-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a few issues rendering text with Skwasm. #51074
Conversation
This fixes flutter/flutter#141001 This also fixes flutter/flutter#143743 * We need to always call `setStrutEnabled(true)` on `StrutStyle`. * `getLineMetricsAt` had reversed ternary logic. * Ported unit tests from CanvasKit over to UI to cover Skwasm and HTML * The HTML renderer should return 0 line metrics for an empty paragraph.
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!
if (ui.ParagraphBuilder.shouldDisableRoundingHack) { | ||
paragraphStyleSetApplyRoundingHack(handle, false); | ||
} |
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.
Not sure I understand this. Why not:
if (ui.ParagraphBuilder.shouldDisableRoundingHack) { | |
paragraphStyleSetApplyRoundingHack(handle, false); | |
} | |
paragraphStyleSetApplyRoundingHack( | |
handle, | |
!ui.ParagraphBuilder.shouldDisableRoundingHack, | |
); |
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.
Because there is no reason to invoke this API when shoudlDisableRoundingHack
is false, since the rounding hack is true by default.
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.
Worth putting a comment above this code?
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
…144419) flutter/engine@bb6c6a0...7e8fefe 2024-02-29 kevmoo@users.noreply.github.com [web] Drop noisy prints from bootstrapping logic (flutter/engine#51097) 2024-02-29 jacksongardner@google.com Fix a few issues rendering text with Skwasm. (flutter/engine#51074) 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 aaclarke@google.com,rmistry@google.com,zra@google.com 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
This fixes flutter/flutter#141001
This also fixes flutter/flutter#143743
setStrutEnabled(true)
onStrutStyle
.getLineMetricsAt
had reversed ternary logic.and HTMLThe HTML renderer should return 0 line metrics for an empty paragraph.