-
Notifications
You must be signed in to change notification settings - Fork 6k
[iOS] Load additional Cupertino system fonts #46857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits
| import 'channel_util.dart'; | ||
| import 'scenario.dart'; | ||
|
|
||
| /// Tries to draw darwin system font: Cupertino-System-Pro, Cupertino-System-Text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of Cupertino-System-Pro, should this be Cupertino-System-Display?
| final PictureRecorder recorder = PictureRecorder(); | ||
| final Canvas canvas = Canvas(recorder); | ||
|
|
||
| final ParagraphBuilder paragraphBuilderPro = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should this be paragraphBuilderDisplay? Technically both fonts can be referred to as "Pro" fonts.
|
We are waiting for feedbacks on the font names before landing this. |
|
auto label is removed for flutter/engine/46857, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
auto label is removed for flutter/engine/46857, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…136910) flutter/engine@9d49175...bfd2ffb 2023-10-19 [email protected] [iOS] Load additional Cupertino system fonts (flutter/engine#46857) 2023-10-19 [email protected] Add missing import (flutter/engine#47083) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Manually loads darwin system fonts. Darwin system fonts returns different typeface in different sizes. This PR loads the font in different sizes then register them as different fonts. These new fonts can be access through the framework. Fixes flutter/flutter#63507 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Addresses #63507, and is a follow up to the engine PR flutter/engine#46857 Changes the font family string when attempting to use Apple system fonts to the new proxies added by the engine. For the "Text" font this will be more secure in the future against possible changes to Apple's API. For the "Display" font, this will now work correctly when it didn't before. I checked the letter spacing values against a native app for all font sizes between 17-28. I made a few adjustments to better match native, but especially for the "Text" font we were either really close, or close enough to not make a large breaking change to default fonts worth it. | Before | After | | ------------- | ------------- | | <img width="466" alt="Screenshot 2023-11-02 at 11 45 12�AM" src="https://github.com/flutter/flutter/assets/58190796/627ed8ac-d848-4f71-aa62-a467b8aac62d"> | <img width="383" alt="Screenshot 2023-11-02 at 11 46 25�AM" src="https://github.com/flutter/flutter/assets/58190796/9a502021-7d2b-4e14-98f1-86971b3830a5"> | The smaller text in both the before and after should be the same. The large system font that Flutter used before was incorrect, which caused it to look more spread out. Now we use the correct font.
Addresses flutter#63507, and is a follow up to the engine PR flutter/engine#46857 Changes the font family string when attempting to use Apple system fonts to the new proxies added by the engine. For the "Text" font this will be more secure in the future against possible changes to Apple's API. For the "Display" font, this will now work correctly when it didn't before. I checked the letter spacing values against a native app for all font sizes between 17-28. I made a few adjustments to better match native, but especially for the "Text" font we were either really close, or close enough to not make a large breaking change to default fonts worth it. | Before | After | | ------------- | ------------- | | <img width="466" alt="Screenshot 2023-11-02 at 11 45 12�AM" src="https://github.com/flutter/flutter/assets/58190796/627ed8ac-d848-4f71-aa62-a467b8aac62d"> | <img width="383" alt="Screenshot 2023-11-02 at 11 46 25�AM" src="https://github.com/flutter/flutter/assets/58190796/9a502021-7d2b-4e14-98f1-86971b3830a5"> | The smaller text in both the before and after should be the same. The large system font that Flutter used before was incorrect, which caused it to look more spread out. Now we use the correct font.
Manually loads darwin system fonts. Darwin system fonts returns different typeface in different sizes. This PR loads the font in different sizes then register them as different fonts. These new fonts can be access through the framework.
Fixes flutter/flutter#63507
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.