Canvas text and macOS font fixes#27548
Merged
bors-servo merged 2 commits intoservo:masterfrom Aug 10, 2020
Merged
Conversation
|
|
Member
Author
|
r? @pcwalton |
Member
Author
|
@bors-servo try |
Contributor
bors-servo
added a commit
that referenced
this pull request
Aug 7, 2020
Canvas text and macOS font fixes These changes work around a source of panics by skipping glyphs that don't exist when drawing text to the canvas. Ideally we would have font fallback and/or tofu characters, but that's a larger project than I wanted to get into right now. Relatedly, while investigating the non-panic errors observed in #27476, I realized that the underlying source of #24622 could be dealt with because we shouldn't actually need to open the CTFont by a file path to get the bytes, since we already have bytes available that we use to create the CTFont. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24622 and fix #27476 and fix the panic observed in #20445. - [x] There are tests for these changes
Contributor
|
💔 Test failed - status-taskcluster |
Member
Author
|
@bors-servo try=wpt |
Contributor
bors-servo
added a commit
that referenced
this pull request
Aug 7, 2020
Canvas text and macOS font fixes These changes work around a source of panics by skipping glyphs that don't exist when drawing text to the canvas. Ideally we would have font fallback and/or tofu characters, but that's a larger project than I wanted to get into right now. Relatedly, while investigating the non-panic errors observed in #27476, I realized that the underlying source of #24622 could be dealt with because we shouldn't actually need to open the CTFont by a file path to get the bytes, since we already have bytes available that we use to create the CTFont. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24622 and fix #27476 and fix the panic observed in #20445. - [x] There are tests for these changes
Contributor
|
💔 Test failed - status-taskcluster |
Member
Author
|
@bors-servo try=wpt-mac |
Contributor
bors-servo
added a commit
that referenced
this pull request
Aug 7, 2020
Canvas text and macOS font fixes These changes work around a source of panics by skipping glyphs that don't exist when drawing text to the canvas. Ideally we would have font fallback and/or tofu characters, but that's a larger project than I wanted to get into right now. Relatedly, while investigating the non-panic errors observed in #27476, I realized that the underlying source of #24622 could be dealt with because we shouldn't actually need to open the CTFont by a file path to get the bytes, since we already have bytes available that we use to create the CTFont. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24622 and fix #27476 and fix the panic observed in #20445. - [x] There are tests for these changes
Contributor
|
☀️ Test successful - status-taskcluster |
pcwalton
reviewed
Aug 10, 2020
| continue; | ||
| }, | ||
| }; | ||
| start += advance * point_size / 24. / 96.; |
Contributor
There was a problem hiding this comment.
Where do these values come from? Are you sure this isn't the units_per_em for some font?
Member
Author
There was a problem hiding this comment.
This is copied directly from the code we used to call in https://github.com/jrmuizel/raqote/blob/490bff22610a130ad75eb797b6bd6e3e361bbde5/src/draw_target.rs#L742-L761.
Contributor
|
@bors-servo: r+ |
Contributor
|
📌 Commit 1e743a7 has been approved by |
highfive
removed
the
S-awaiting-review
highfive
added
the
S-awaiting-merge
Contributor
Contributor
|
☀️ Test successful - status-taskcluster |
highfive
removed
the
S-awaiting-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes work around a source of panics by skipping glyphs that don't exist when drawing text to the canvas. Ideally we would have font fallback and/or tofu characters, but that's a larger project than I wanted to get into right now.
Relatedly, while investigating the non-panic errors observed in #27476, I realized that the underlying source of #24622 could be dealt with because we shouldn't actually need to open the CTFont by a file path to get the bytes, since we already have bytes available that we use to create the CTFont.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors