Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix a bug in the HTML render's getClosestGlyphInfo implementation #48774

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

LongCatIsLooong
Copy link
Contributor

closestFragmentAtOffset takes a horizontal offset from the start of the line, not the start of the paragraph.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM!

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023
@auto-submit auto-submit bot merged commit 0e7248d into flutter:main Dec 14, 2023
@LongCatIsLooong LongCatIsLooong deleted the fix-getClosestGlyphInfo branch December 14, 2023 21:21
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Dec 14, 2023
…140176)

flutter/engine@2140942...0e7248d

2023-12-14 [email protected] Fix a bug in the HTML render's `getClosestGlyphInfo` implementation (flutter/engine#48774)
2023-12-14 [email protected] Fix `NSPrivacyCollectedDataTypes` array in privacy manifest (flutter/engine#49041)

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 [email protected],[email protected],[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
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 20, 2023
Fixes #131435, #104594, #43400
Needs flutter/engine#48774 (to fix the web test failure).

Currently the method we use for text span hit testing `TextPainter.getPositionForOffset` always returns the closest `TextPosition`, even when the given offset is far away from the text. 

The new TextPaintes method tells you the layout bounds (`width =  letterspacing / 2 + x_advance + letterspacing / 2`, `height = font ascent + font descent`) of a character, the PR changes the hit testing implementation such that a TextSpan is only considered hit if the point-down event landed in one of it's character's layout bounds.

Potential issues:

1. In theory since the text is baseline aligned, we should use the max ascent and max descent of each character to calculate the height of the text span's hit-test region, in case some characters in the span have to fall back to a different font, but that will be slower and it typically doesn't make a huge difference. 

This is a breaking change. It also introduces a new finder and a new method `WidgetTester.tapOnText`: `await tester.tapOnText('string to match')` for ease of migration.
auto-submit bot added a commit to flutter/flutter that referenced this pull request Dec 20, 2023
Reverts #139717
Initiated by: LongCatIsLooong
This change reverts the following previous change:
Original Description:
Fixes #131435, #104594, #43400
Needs flutter/engine#48774 (to fix the web test failure).

Currently the method we use for text span hit testing `TextPainter.getPositionForOffset` always returns the closest `TextPosition`, even when the given offset is far away from the text. 

The new TextPaintes method tells you the layout bounds (`width =  letterspacing / 2 + x_advance + letterspacing / 2`, `height = font ascent + font descent`) of a character, the PR changes the hit testing implementation such that a TextSpan is only considered hit if the point-down event landed in one of it's character's layout bounds.

Potential issues:

1. In theory since the text is baseline aligned, we should use the max ascent and max descent of each character to calculate the height of the text span's hit-test region, in case some characters in the span have to fall back to a different font, but that will be slower and it typically doesn't make a huge difference. 

This is a breaking change. It also introduces a new finder and a new method `WidgetTester.tapOnText`: `await tester.tapOnText('string to match')` for ease of migration.
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
Fixes flutter#131435, flutter#104594, flutter#43400
Needs flutter/engine#48774 (to fix the web test failure).

Currently the method we use for text span hit testing `TextPainter.getPositionForOffset` always returns the closest `TextPosition`, even when the given offset is far away from the text. 

The new TextPaintes method tells you the layout bounds (`width =  letterspacing / 2 + x_advance + letterspacing / 2`, `height = font ascent + font descent`) of a character, the PR changes the hit testing implementation such that a TextSpan is only considered hit if the point-down event landed in one of it's character's layout bounds.

Potential issues:

1. In theory since the text is baseline aligned, we should use the max ascent and max descent of each character to calculate the height of the text span's hit-test region, in case some characters in the span have to fall back to a different font, but that will be slower and it typically doesn't make a huge difference. 

This is a breaking change. It also introduces a new finder and a new method `WidgetTester.tapOnText`: `await tester.tapOnText('string to match')` for ease of migration.
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
Reverts flutter#139717
Initiated by: LongCatIsLooong
This change reverts the following previous change:
Original Description:
Fixes flutter#131435, flutter#104594, flutter#43400
Needs flutter/engine#48774 (to fix the web test failure).

Currently the method we use for text span hit testing `TextPainter.getPositionForOffset` always returns the closest `TextPosition`, even when the given offset is far away from the text. 

The new TextPaintes method tells you the layout bounds (`width =  letterspacing / 2 + x_advance + letterspacing / 2`, `height = font ascent + font descent`) of a character, the PR changes the hit testing implementation such that a TextSpan is only considered hit if the point-down event landed in one of it's character's layout bounds.

Potential issues:

1. In theory since the text is baseline aligned, we should use the max ascent and max descent of each character to calculate the height of the text span's hit-test region, in case some characters in the span have to fall back to a different font, but that will be slower and it typically doesn't make a huge difference. 

This is a breaking change. It also introduces a new finder and a new method `WidgetTester.tapOnText`: `await tester.tapOnText('string to match')` for ease of migration.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants