Skip to content

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

Merged
auto-submit[bot] merged 7 commits into
flutter:mainfrom
rohanjariwala03:vgc-em-text-position
Oct 29, 2026
Merged

auto-submit[bot] merged 7 commits into
flutter:mainfrom
rohanjariwala03:vgc-em-text-position

Conversation

@rohanjariwala03

@rohanjariwala03 rohanjariwala03 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

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.

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
@rohanjariwala03

Copy link
Copy Markdown
Contributor 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...

@rohanjariwala03
rohanjariwala03 requested a review from flar October 29, 2026 00:15

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

LGTM

@flar flar added the CICD Run CI/CD label Oct 29, 2026
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Oct 29, 2026
@rohanjariwala03

Copy link
Copy Markdown
Contributor Author

@gaaclarke @flar I've merged the main branch and updated the PR. Can you merge this?

@gaaclarke gaaclarke added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Oct 29, 2026
@auto-submit
auto-submit Bot merged commit 10f15b3 into flutter:main Oct 29, 2026
14 checks passed
@rohanjariwala03
rohanjariwala03 deleted the vgc-em-text-position branch October 29, 2026 19:50
flutter-zl pushed a commit to flutter-zl/flutter that referenced this pull request Oct 30, 2026
flutter/packages@0b72e92...3742a4e

2026-10-30 [email protected] [pigeon] Allow event channel
unregistration (flutter/packages#13438)
2026-10-30 [email protected] [material_ui] Fix Escape key not
closing MenuAnchor opened by tap or click (flutter/packages#13430)
2026-10-30 [email protected] manual roll and manual build and copy
(flutter/packages#13495)
2026-10-29 [email protected] [pigeon] Separate native interop Dart
bindings into a conditionally imported companion file
(flutter/packages#13393)
2026-10-29 [email protected] [image_picker_ios] PR 4/5 Migrate
the view provider, PHPicker save operation, and image utilities to Swift
(flutter/packages#13394)
2026-10-29 [email protected] Sync release-material_ui-1.9.0 to
main (flutter/packages#13479)
2026-10-29 [email protected] [material_ui]
Unskip web tests that pass on CanvasKit and Skwasm: time_picker_test
(flutter/packages#13471)
2026-10-29 [email protected] [vector_graphics]
Dispose pictures decoded after their VectorGraphic is removed
(flutter/packages#13435)
2026-10-29 [email protected]
[camera_android_x] implement CameraX RecordingStats and VideoRecordEvent
APIs (flutter/packages#13330)
2026-10-29 [email protected] Sync release-go_router-18.1.2 to
main (flutter/packages#13480)
2026-10-29 [email protected] Sync release-cupertino_ui-1.1.5 to
main (flutter/packages#13481)
2026-10-29 [email protected]
[vector_graphics_compiler] Fix em units in text positions
(flutter/packages#13388)

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
gmackall pushed a commit to gmackall/flutter that referenced this pull request Nov 3, 2026
flutter/packages@0b72e92...3742a4e

2026-10-30 [email protected] [pigeon] Allow event channel
unregistration (flutter/packages#13438)
2026-10-30 [email protected] [material_ui] Fix Escape key not
closing MenuAnchor opened by tap or click (flutter/packages#13430)
2026-10-30 [email protected] manual roll and manual build and copy
(flutter/packages#13495)
2026-10-29 [email protected] [pigeon] Separate native interop Dart
bindings into a conditionally imported companion file
(flutter/packages#13393)
2026-10-29 [email protected] [image_picker_ios] PR 4/5 Migrate
the view provider, PHPicker save operation, and image utilities to Swift
(flutter/packages#13394)
2026-10-29 [email protected] Sync release-material_ui-1.9.0 to
main (flutter/packages#13479)
2026-10-29 [email protected] [material_ui]
Unskip web tests that pass on CanvasKit and Skwasm: time_picker_test
(flutter/packages#13471)
2026-10-29 [email protected] [vector_graphics]
Dispose pictures decoded after their VectorGraphic is removed
(flutter/packages#13435)
2026-10-29 [email protected]
[camera_android_x] implement CameraX RecordingStats and VideoRecordEvent
APIs (flutter/packages#13330)
2026-10-29 [email protected] Sync release-go_router-18.1.2 to
main (flutter/packages#13480)
2026-10-29 [email protected] Sync release-cupertino_ui-1.1.5 to
main (flutter/packages#13481)
2026-10-29 [email protected]
[vector_graphics_compiler] Fix em units in text positions
(flutter/packages#13388)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD 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