Reland "Make TextSpan hit testing precise." (#140468)
#140621
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 9003f13.
github-actions
bot
added
framework
Contributor
Author
|
G3 migration cls have landed and this shouldn't be breaking any internal tests now. |
LongCatIsLooong
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 3, 2024
This was referenced Jan 4, 2024
Member
|
(I just closed the latter two of these issues because they hadn't been automatically — FYI @LongCatIsLooong. GitHub's magic auto-close regexps don't recognize this sort of distributive syntax, which is why the usual convention here is like "Fixes #131435. Fixes #104594. Fixes #43400." Thanks for the fix!) |
gnprice
added a commit
to gnprice/zulip-flutter
that referenced
this pull request
Jan 4, 2024
By saying `tester.tapAt(find.text('hello'))`, we had been aiming
at the center of the Text widgets, and expecting that to hit the
recognizer we've put on the span, even though the widget is much
wider than the span and the latter doesn't reach the former's center.
Effectively we were relying on the presence of issue zulip#214.
But with an upstream change yesterday:
flutter/flutter#140621
such a tap no longer hits the span. That broke these tests.
To fix them, aim the taps near the start of the widget instead.
Fixes: zulip#475
gnprice
added a commit
to gnprice/zulip-flutter
that referenced
this pull request
Jan 4, 2024
And update Flutter's supporting libraries to match. In particular this pulls in the following upstream change: flutter/flutter#140621 which fixes zulip#214. Fixes: zulip#214
chrisbobbe
pushed a commit
to zulip/zulip-flutter
that referenced
this pull request
Jan 4, 2024
By saying `tester.tapAt(find.text('hello'))`, we had been aiming
at the center of the Text widgets, and expecting that to hit the
recognizer we've put on the span, even though the widget is much
wider than the span and the latter doesn't reach the former's center.
Effectively we were relying on the presence of issue #214.
But with an upstream change yesterday:
flutter/flutter#140621
such a tap no longer hits the span. That broke these tests.
To fix them, aim the taps near the start of the widget instead.
Fixes: #475
chrisbobbe
pushed a commit
to zulip/zulip-flutter
that referenced
this pull request
Jan 4, 2024
And update Flutter's supporting libraries to match. In particular this pulls in the following upstream change: flutter/flutter#140621 which fixes #214. Fixes: #214
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 5, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 16, 2024
gnprice
added a commit
to gnprice/flutter_customer_testing
that referenced
this pull request
May 22, 2024
Zulip is an open-source team chat app, with a new Flutter-based mobile client now in beta. This is that client's test suite. I believe these will be the only tests currently in this registry for an app, rather than a library. That should naturally give them a different mix of use cases and types of tests. Concretely, we've seen a handful of breaking changes over the past year that weren't caught by any of Flutter's existing test suites. These were in text hit-testing: zulip/zulip-flutter@ba7a2bf flutter/flutter#140621 and Material menu behavior: zulip/zulip-flutter@38ed6c8 flutter/flutter#130536 and SlottedContainerRenderObjectMixin gaining a type parameter: zulip/zulip-flutter@2f0f469 flutter/flutter#126108 I'm not complaining, to be clear, and none of these were particularly onerous for us to adapt to. By registering these tests, I'm hoping to provide feedback on future such breakages at a point where it's actionable. Omitted here are several tests that re-generate generated files and check they match what's in the tree. Those are pretty slow, and I think they're pretty insensitive to changes in the Flutter tree anyway; rather they depend on pigeon, json_serializable, build_runner, and drift_dev.
gnprice
added a commit
to flutter/tests
that referenced
this pull request
May 23, 2024
Zulip is an open-source team chat app, with a new Flutter-based mobile client now in beta. This is that client's test suite. I believe these will be the only tests currently in this registry for an app, rather than a library. That should naturally give them a different mix of use cases and types of tests. Concretely, we've seen a handful of breaking changes over the past year that weren't caught by any of Flutter's existing test suites. These were in text hit-testing: zulip/zulip-flutter@ba7a2bf flutter/flutter#140621 and Material menu behavior: zulip/zulip-flutter@38ed6c8 flutter/flutter#130536 and SlottedContainerRenderObjectMixin gaining a type parameter: zulip/zulip-flutter@2f0f469 flutter/flutter#126108 I'm not complaining, to be clear, and none of these were particularly onerous for us to adapt to. By registering these tests, I'm hoping to provide feedback on future such breakages at a point where it's actionable. Omitted here are several tests that re-generate generated files and check they match what's in the tree. Those are pretty slow, and I think they're pretty insensitive to changes in the Flutter tree anyway; rather they depend on pigeon, json_serializable, build_runner, and drift_dev.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #131435, #104594, #43400
Currently the method we use for text span hit testing
TextPainter.getPositionForOffsetalways returns the closestTextPosition, 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 its character's layout bounds.Potential issues:
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.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.