Skip to content
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

[web] Use DOM to render paragraphs with letter spacing #45651

Merged
merged 3 commits into from Sep 13, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Sep 11, 2023

<Only applies to the HTML renderer>

When a paragraph has letter spacing that's greater than 0, we attempt to render each character individually on the canvas, while adding extra gaps between the characters (because the browser's canvas API doesn't support letter spacing).

This works well-ish in English but causes issues with international text. It also makes text rendering very slow.

With this PR, the idea is to force all paragraphs with letter spacing to be rendered to a DOM element.

Fixes flutter/flutter#51234
Fixes flutter/flutter#71220

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Sep 11, 2023
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

LGTM! Prepare for Gold/Skuba failures.

@mdebbar
Copy link
Contributor Author

mdebbar commented Sep 13, 2023

LGTM! Prepare for Gold/Skuba failures.

Yeah, I was expecting to see Gold changes, but there's none. So I'm adding a new golden test to cover the issue that this PR is fixing. Test coming soon..

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 13, 2023
@auto-submit auto-submit bot merged commit b94b83b into flutter:main Sep 13, 2023
25 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 14, 2023
…134695)

flutter/engine@cd90cc8...4160eba

2023-09-13 skia-flutter-autoroll@skia.org Roll Skia from b38989859b81 to a30fbf83f2e9 (3 revisions) (flutter/engine#45796)
2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump quiver from 3.0.0 to 3.2.1 in /lib/web_ui (flutter/engine#45792)
2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump http from 0.13.5 to 1.1.0 in /lib/web_ui (flutter/engine#45791)
2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump archive from 3.1.2 to 3.3.9 in /lib/web_ui (flutter/engine#45795)
2023-09-13 mdebbar@google.com [web] Use DOM to render paragraphs with letter spacing (flutter/engine#45651)

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 bdero@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
…lutter#134695)

flutter/engine@cd90cc8...4160eba

2023-09-13 skia-flutter-autoroll@skia.org Roll Skia from b38989859b81 to a30fbf83f2e9 (3 revisions) (flutter/engine#45796)
2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump quiver from 3.0.0 to 3.2.1 in /lib/web_ui (flutter/engine#45792)
2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump http from 0.13.5 to 1.1.0 in /lib/web_ui (flutter/engine#45791)
2023-09-13 49699333+dependabot[bot]@users.noreply.github.com Bump archive from 3.1.2 to 3.3.9 in /lib/web_ui (flutter/engine#45795)
2023-09-13 mdebbar@google.com [web] Use DOM to render paragraphs with letter spacing (flutter/engine#45651)

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 bdero@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023
<**_Only applies to the HTML renderer_**>

When a paragraph has letter spacing that's greater than 0, we attempt to render each character individually on the canvas, while adding extra gaps between the characters (because the browser's canvas API doesn't support letter spacing).

This works well-ish in English but causes issues with international text. It also makes text rendering very slow.

With this PR, the idea is to force all paragraphs with letter spacing to be rendered to a DOM element.

Fixes flutter/flutter#51234
Fixes flutter/flutter#71220
@mdebbar mdebbar deleted the canvas_letter_spacing branch December 11, 2023 21:01
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 platform-web Code specifically for the web engine
Projects
None yet
2 participants