Skip to content

[vector_graphics_compiler] Fix em units in text positions - #13388

Open
rohanjariwala03 wants to merge 6 commits into
flutter:mainfrom
rohanjariwala03:vgc-em-text-position
Open

rohanjariwala03 wants to merge 6 commits into
flutter:mainfrom
rohanjariwala03:vgc-em-text-position

Conversation

@rohanjariwala03

@rohanjariwala03 rohanjariwala03 commented Oct 22, 2026 •

Copy link
Copy Markdown

Text x, y, dx and dy values in em were treated as pixels. For example, dy=".35em" with font-size="425px" moved the text down by 0.35 pixels instead of about 149, so text centered this way was drawn too high.

DoubleOrPercentage.fromString stripped the unit with parseDouble, so an em value ended up the same as a plain number. Now it remembers that the value is in em, and computeTextPosition multiplies it by the element's font size, including one inherited from a parent. Without a font size it uses kDefaultFontSize (16), the same default computeTextConfig uses when drawing the text. The bug is in vector_graphics_compiler, which flutter_svg uses to parse SVGs.

Fixes flutter/flutter#158758

Pre-Review Checklist

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-assist bot 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

  1. 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

Text `x`, `y`, `dx` and `dy` values in `em` were parsed with the unit
stripped, so `dy=".35em"` moved the text by 0.35 pixels. They are now
multiplied by the element's font size, falling back to the same default
of 16 used when drawing the text.

Fixes flutter/flutter#158758
@github-actions github-actions Bot added p: vector_graphics triage-engine Should be looked at in engine triage labels Oct 22, 2026
@rohanjariwala03
rohanjariwala03 marked this pull request as ready for review October 22, 2026 18:39

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for 'em' units in SVG text positions (x, y, dx, dy) by updating the DoubleOrPercentage class to parse and calculate values relative to the font size. Feedback suggests converting the parsed string to lowercase to ensure case-insensitive matching of the 'em' unit.

Comment thread packages/vector_graphics_compiler/lib/src/svg/parser.dart Outdated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@gaaclarke
gaaclarke self-requested a review October 26, 2026 18:22
Comment thread packages/vector_graphics_compiler/lib/src/svg/parser.dart Outdated
Comment thread packages/vector_graphics_compiler/lib/src/svg/parser.dart Outdated
@rohanjariwala03

Copy link
Copy Markdown
Author

@gaaclarke Thanks for the review! Merged upstream. Since 1.3.3 is now the coordinate list change, I moved this entry to 1.3.4.

@gaaclarke gaaclarke added the CICD Run CI/CD label Oct 28, 2026
@gaaclarke
gaaclarke requested a review from flar October 28, 2026 15:35
@gaaclarke

Copy link
Copy Markdown
Member

@flar can you be a second review on this please?

@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Oct 28, 2026

@flar flar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, but there's a nit that I'd like to hear back about...

}
return DoubleOrPercentage._(parseDouble(raw)!, false);
final String trimmed = raw.trim().toLowerCase();
final bool isEm = trimmed.endsWith('em') && !trimmed.endsWith('rem');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weak in that it seems to need to exclude rem but it doesn't otherwise ensure that the value is #####em rather than #####somethingsomethingelseem. It leaves me wondering why rem was specifically chosen to be excluded, but nothing else was done to make sure the em was just an em...?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there appears to be an isPercentage method somewhere, should there be something like an isEm method?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, rem was only excluded because it's the other unit ending in "em". I added isEm/parseEm next to the percentage helpers. isEm only matches a number followed by em, so the special case is gone. Added tests too.

@rohanjariwala03
rohanjariwala03 requested a review from flar October 29, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p: vector_graphics triage-engine Should be looked at in engine triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flutter_svg] Text offset in y direction

3 participants