Implement font fallback#187520
Open
Rusino wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds support for fallback font families in WebTextStyle and updates WebStrutStyle to apply styles using WebTextStyle. It also adds a test verifying fallback font rendering. Feedback on these changes highlights that unconditionally quoting the font family and bypassing canonicalizeFontFamily can break generic font families, and that fallback fonts themselves should be canonicalized. Additionally, the reviewer noted that fontFamilyFallback was omitted in WebStrutStyle's usage of WebTextStyle, and pointed out leftover commented-out code.
mdebbar
previously approved these changes
Jun 3, 2026
mdebbar
reviewed
Jun 4, 2026
Comment on lines
+557
to
+596
| List<String> get _fallbackFontFamilies { | ||
| if (isIOS15) { | ||
| // Remove the "-apple-system" fallback font because it causes a crash in | ||
| // iOS 15. | ||
| // | ||
| // See github issue: https://github.com/flutter/flutter/issues/90705 | ||
| // See webkit bug: https://bugs.webkit.org/show_bug.cgi?id=231686 | ||
| return <String>['BlinkMacSystemFont']; | ||
| } | ||
| if (isMacOrIOS) { | ||
| return <String>['-apple-system', 'BlinkMacSystemFont']; | ||
| } | ||
| return <String>['Arial']; | ||
| } | ||
|
|
||
| String _canonicalizeFontFamily(String? fontFamily, [List<String>? fontFamilyFallback]) { | ||
| final processedFontFamilyFallback = fontFamilyFallback == null || fontFamilyFallback.isEmpty | ||
| ? '' | ||
| : '${fontFamilyFallback.map(_processFontFamily).join(', ')}, '; | ||
| return '${_processFontFamily(fontFamily)}, $processedFontFamilyFallback ${_fallbackFontFamilies.join(', ')}, sans-serif'; | ||
| } | ||
|
|
||
| String? _processFontFamily(String? fontFamily) { | ||
| if (genericFontFamilies.contains(fontFamily)) { | ||
| return fontFamily; | ||
| } | ||
| if (isMacOrIOS) { | ||
| // Unlike Safari, Chrome on iOS does not correctly fallback to cupertino | ||
| // on sans-serif. | ||
| // Map to San Francisco Text/Display fonts, use -apple-system, | ||
| // BlinkMacSystemFont. | ||
| if (fontFamily == '.SF Pro Text' || | ||
| fontFamily == '.SF Pro Display' || | ||
| fontFamily == '.SF UI Text' || | ||
| fontFamily == '.SF UI Display') { | ||
| return _fallbackFontFamilies.join(', '); | ||
| } | ||
| } | ||
| return '"$fontFamily"'; | ||
| } |
Contributor
There was a problem hiding this comment.
Is there a reason that requires duplicating this code here instead of refactoring the one in util.dart?
Comment on lines
+546
to
+555
| String? get sharedFontFamily; | ||
| List<String>? get sharedFontFamilyFallback; | ||
| double? get sharedFontSize; | ||
| ui.FontWeight? get sharedFontWeight; | ||
| ui.FontStyle? get sharedFontStyle; | ||
| ui.Locale? get sharedLocale => null; | ||
| List<ui.FontFeature>? get sharedFontFeature => null; | ||
| List<ui.FontVariation>? get sharedFontVariations => null; | ||
| double? get sharedLetterSpacing => null; | ||
| double? get sharedWordSpacing => null; |
Contributor
There was a problem hiding this comment.
Can we use the original property names instead of sharedXYZ?
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
2 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.
Pass the correct font line to Canvas2D
I was only able to demonstrate it working on text fonts (Sans vs Serif).
Part of #172561