Repository navigation
[vector_graphics] Use font-family lists as a family with fallbacks - #13552
Conversation
github-actions
Bot
added
p: vector_graphics
triage-engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the vector_graphics package to version 1.2.7. It introduces support for parsing a comma-separated font-family list from SVG, extracting the first family name as the primary font family and the remaining names as fallbacks. The _TextConfig class and FlutterVectorGraphicsListener have been updated to handle fontFamilyFallback. Additionally, a new test file font_family_test.dart has been added to verify this behavior. There are no review comments, and I have no feedback to provide.
b-luk
left a comment
b-luk
left a comment
There was a problem hiding this comment.
Thanks for the PR!
font_family_test.dart has a few issues:
- Relying on
FLUTTER_ROOTand the test tool caching the fonts is fragile. - It uses
dart:ioand can't be run on Chrome. - As you mention in the description, the test does not actually test the fallback font.
What do you think about making parseFontFamilies be @visibleForTesting and testing it in listener_test.dart, and getting rid of font_family_test.dart? That lets us avoid those issues. The downside is that it doesn't test the end-to-end rendered result.
|
Makes sense, thanks! Made |
| /// | ||
| /// Flutter takes the first name as the family and the rest as fallbacks. | ||
| @visibleForTesting | ||
| List<String> parseFontFamilies(String? fontFamily) { |
There was a problem hiding this comment.
This is a simplified parser which doesn't handle corner cases like commas inside quotes, backslash escapes, and probably other special case scenarios. I think that's okay, because these corner cases should be exceedingly rare and trying to match the full css parsing spec is a rabbit hole I don't think we need to go down. This new behavior is a strict improvement over the current behavior for the common cases, so this LGTM.
nonblocking nit: Maybe add a short one-line comment to note that this isn't a full parser and doesn't handle those corner cases.
# Conflicts: # packages/vector_graphics/CHANGELOG.md
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
More conflicts =( I thought we had tooling to handle versioning at least?
# Conflicts: # packages/vector_graphics/CHANGELOG.md # packages/vector_graphics/pubspec.yaml # packages/vector_graphics/test/listener_test.dart
|
Sorry about that! Fixed the conflicts with the filters PR, this is now 1.3.1. As far as I can tell vector_graphics isn't set up for batch release yet (no |
gaaclarke
added
the
autosubmit
…r#196450) flutter/packages@653628e...14f9933 2026-11-12 [email protected] [material_ui] Fix SnackBarThemeData.lerp dropping showCloseIcon (flutter/packages#13449) 2026-11-11 [email protected] [ci] Update repo for 3.50 stable release (flutter/packages#13628) 2026-11-11 [email protected] [vector_graphics] Use font-family lists as a family with fallbacks (flutter/packages#13552) 2026-11-11 [email protected] Roll Flutter (stable) from c197ca5 to ab29120 (1647 revisions) (flutter/packages#13625) 2026-11-11 [email protected] [shared_prferences] Adopts mockito-kotlin in tests (flutter/packages#13624) 2026-11-11 [email protected] [cupertino_ui] Dim disabled CupertinoSlider controls (flutter/packages#13419) 2026-11-11 [email protected] [flutter_svg] Invalidate pending cache entries on clear and evict (flutter/packages#13595) 2026-11-11 [email protected] [cupertino_ui] Fix the tinted button's fill opacity (flutter/packages#13436) 2026-11-11 [email protected] [various] Support SVG filters (blur, offset, feMerge) (flutter/packages#11909) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [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
SVG
font-familycan be a list like"Roboto, sans-serif", but the whole string was passed to Flutter as one font name, so it never matched and you always got the default font.Now the list is split. The first name is used as the font and the rest go into
fontFamilyFallback. Quotes around names are removed too. A single name works the same as before, and since this is only in the listener, SVGs that were already compiled get the fix as well.parseFontFamiliesis tested directly inlistener_test.dart, including the fallback order, so the tests also run on web.Fixes flutter/flutter#187087
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2