Fix font sorting problem due to iOS 14 fonts being broader #20557
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
|
|
Can we add a golden test on a simulator for this? I don't think we can run a 14.0 simulator on CI, but at least we can catch if the text regresses. |
|
I'm gonna wait a few days on this for https://skia-review.googlesource.com/c/skia/+/311596 to go through so I can test. |
xster
force-pushed
the
ios-14-font-sorting
branch
from
0b230e7 to
9fced26
Compare
|
Ok, Skia patch is in. Added some tests. @dnfield yes and no. Yes because we already have a golden test which should fail once we move the CI to iOS 14. (Food for thought for @jmagman. This might be our opening gambit for future go/flutter-apple-release-cadence to flush these out). No because those tests should be in the framework since there is no public API here flutter.dev/go/flutter-skia-ios-font-problem. |
|
lol, at a quick glance, it seems like we may not have a single non-ahem golden test in https://flutter-gold.skia.org/list. Actually I just thought there might one thing we can do, somewhat tangentially to this PR. We can test in a scenario test that when you ask for a bogus font on iOS, it should always draw in SF. I'll send a separate PR. |
|
Beh, I didn't feel like writing perf. Here's the test #20687. It'll fail until this one's merged. |
|
Actually that one probably won't fail since we're not running on iOS 14. Both can merge independently. |
| // Flutter APIs have the weight specified to narrow it down | ||
| // later. The width ordering here is more consequential since | ||
| // TextStyle doesn't have letter width APIs. | ||
| return std::abs(a_style.width() - SkFontStyle::kNormal_Width) < |
There was a problem hiding this comment.
Is it possible for a_style.width() to be kNormal_Width - delta and b_style.width() to be kNormal_Width + delta?
If so, then add something that prefers either a or b in that case in order to ensure consistency in the sort results.
Minikin's FontCollection will choose the first font in the list among the fonts that have the highest value in Minikin's scoring algorithm. The sort should ensure that the first such font is always the same in each run of the app.
There was a problem hiding this comment.
Did you mean 2 widths equidistant from the middle in opposite directions? Ya, very good point. We can make that case deterministic.
xster
force-pushed
the
ios-14-font-sorting
branch
from
053ae74 to
6959e74
Compare
4 participants
Fixes flutter/flutter#60013
The problem turned out way simpler than I expected. Despite it being officially deprecated, searching for font by font family name string is still working on iOS 14. We still have to fix it at some point but for now, the thing that got borked was searching for the broad family name such as
.AppleSystemUIFont/.SFUI-Regularactually returns both condensed and normal widths of the fonts (thekCTFontWidthTrait). We weren't accounting for it in our sorting so we can end up with the narrower font.Didn't write tests yet. Would love a pointer for which file this might fit in (or whether I'd have to start something new)