Add new test font #39809
Add new test font #39809
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
cc @Hixie |
|
(Still trying to figure out how web tests load fonts but other than that should be ready for review). |
LongCatIsLooong
force-pushed
the
test-font
branch
from
eacc77c to
3ecb9b0
Compare
|
@LongCatIsLooong I'm thinking maybe adding some code points in the U+10000+ range could be useful to test behaviour with surrogate pairs? Say, Musical Symbols Range: U+1D100–U+1D1FF or Mathematical Alphanumeric Symbols Range: U+1D400–U+1D7FF. Other interesting items for tests (thinking of some of the existing issues around complex scripts in text fields):
|
LongCatIsLooong
force-pushed
the
test-font
branch
2 times, most recently
from
5992051 to
d7da67c
Compare
LongCatIsLooong
force-pushed
the
test-font
branch
3 times, most recently
from
1411e77 to
4761add
Compare
LongCatIsLooong
force-pushed
the
test-font
branch
from
4761add to
0a7da1f
Compare
|
This font is meant for framework tests. In the framework we can tell whether a codepoint is in the supplementary planes by looking at the range and don't have to rely on the shaper so I think it's rarely needed to have glyph mappings for that. If you have specific needs feel free to update the script and regenerate the font (but Skia is likely a better place to write tests to verify the shapers are working as expected, the framework currently doesn't direct configure shapers AFAIK), or use |
|
@LongCatIsLooong yes I meant framework tests for issues like For this one perhaps we could just add some > U+10000 characters to CMAP as that should be cheap. |
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
Web changes look good to me with some minor suggestions!
| final FontManager fontManager = _testFontManager = FontManager() | ||
| ..downloadAsset( | ||
| ahemFontFamily, 'url($ahemFontUrl)', const <String, String>{}) | ||
| ..downloadAsset( | ||
| flutterTestFontFamily, 'url($flutterTestFontUrl)', const <String, String>{}) | ||
| ..downloadAsset( | ||
| robotoFontFamily, 'url($robotoTestFontUrl)', const <String, String>{}) | ||
| ..downloadAsset( | ||
| robotoVariableFontFamily, 'url($robotoVariableTestFontUrl)', const <String, String>{}); |
There was a problem hiding this comment.
Suggestion: we could put these fonts in a Map and loop through them instead of calling one by one. Like:
final Map<String, String> testFonts = <String, String>{
ahemFontFamily: ahemFontUrl,
flutterTestFontFamily: flutterTestFontUrl,
robotoFontFamily: robotoFontUrl,
robotoVariableFontFamily: robotoVariableFontUrl,
};| } | ||
| return fontFamily; | ||
| final String fontFamily = this.fontFamily.isEmpty ? FlutterViewEmbedder.defaultFontFamily : this.fontFamily; | ||
| // In the flutter tester environment, we use a predictable-size test fonts. |
There was a problem hiding this comment.
| // In the flutter tester environment, we use a predictable-size test fonts. | |
| // In the flutter tester environment, we use predictable-size test fonts. |
|
Overall approach LGTM The tests are segfaulting on CI. In particular, the runs of flutter_tester with ASAN are reporting a null pointer dereference in |
| api_file = "//flutter/third_party/web_test_fonts/lib/web_test_fonts.dart" | ||
| input_dir = "//flutter/third_party/web_test_fonts/lib/web_test_fonts/" |
There was a problem hiding this comment.
This package needs to exist. For example, look at:
- https://github.com/flutter/engine/tree/main/third_party/web_unicode
- https://github.com/flutter/engine/tree/main/third_party/web_locale_keymap
Perhaps you forgot to include them in the commit?
There was a problem hiding this comment.
Sorry forgot to track those files. The tests are passing now.
LongCatIsLooong
force-pushed
the
test-font
branch
from
445bc06 to
c3ea797
Compare
|
@tgucio I think the issues you mentioned are likely skparagraph issues and it's better to add tests to skia, as in those cases it seems the framework isn't doing anything particularly wrong, and it's going to fail faster so should there be any regressions skia maintainers will notice that without having to roll the changes into flutter. Also for skparagraph it's easy to include/load font assets in their tests so you could use existing fonts. |
|
I didn't realize we were running |
|
Looks like the tests are passing. Could you take another look at the new approach? |
LongCatIsLooong
added
the
autosubmit
5 participants
I'm planning to keep Ahem the default until the framework tests are migrated (flutter/flutter#121306).
Add a new test font that provides accurate font & glyph metrics
and some Chinese character codepoint coverage(not sure if that's still needed).Ahem's 1000 upem (not a power of 2) causes FreeType to truncate numbers when computing metrics, since it will be used as a divisor and calculations are done using 26.6 fixed-point numbers. For example when
point size = 1, FreeType would report an x advance of0.99998instead of1. Currently we round most of the horizontal metrics so there are tons ofexpect(text.width, 100)in framework tests. Those tests are going to fail once the rounding is removed.The new font covers most codepoint ranges Ahem covers, with glyphs that look almost identical to glyphs from Ahem, except with a different baseline location (
ASCENT : DESCENT = 3 : 1, since Ahem's ratioASCENT : DESCENT = 4 : 1can't be preserved whenupemis a power of 2).Some font metrics:
ascent = 768,descent = 256,units per em = 1024Glyph Mappings by Scripts
Glyph
character(s):
!"#$%&()*+,-./0123456789:;<=>?@[\]^_`{|}~¡¢£¤¥¦§¨©«¬<SOFT HYPHEN>®¯°±²³´µ¶·¸¹»¼½¾¿×÷ˆˇˉ˘˙˚˛˜˝–—‘’‚“”„†‡•…‰‹›⁄™∂∆∏∑−∙√∞∫≈≠≤≥⋲◊<0xf000><0xf001><0xf002>character(s):
ΔΥΧΩμπΩcharacter(s):
〇一七三中九二五八六十否囗四土文文是木本正水测火确试金character(s):
ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyzªºÀÁÂÃÄÅÆÇÈÊËÌÍÎÏÐÑÒÓÔÕÖØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõöøùúûüýþÿıŒœŸƒcharacter(s):
pcharacter(s):
Écharacter(s):
<SPACE>character(s):
<EN SPACE>character(s):
<THREE-PER-EM SPACE>character(s):
<FOUR-PER-EM SPACE>character(s):
<SIX-PER-EM SPACE>character(s):
<THIN SPACE>character(s):
<HAIR SPACE>character(s):
<ZERO WIDTH NO-BREAK SPACE>Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.