Skip to content

Tappable links' touch target can extend through whitespace #214

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

Closed
chrisbobbe opened this issue Jul 7, 2023 · 3 comments · Fixed by #476
Closed

Tappable links' touch target can extend through whitespace #214

chrisbobbe opened this issue Jul 7, 2023 · 3 comments · Fixed by #476
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents

Comments

@chrisbobbe
Copy link
Collaborator

If you have a short paragraph ending in a link, the space between the link and the end margin is also tappable. Shown here in the app as built for macOS, so you can see the tappable area when the cursor changes on hover:

Jul-07-2023 13-32-51

@chrisbobbe chrisbobbe added the a-content Parsing and rendering Zulip HTML content, notably message contents label Jul 7, 2023
@gnprice
Copy link
Member

gnprice commented Jul 7, 2023

Wacky! Thanks for the report. Copying what I said at #204 (comment) :

A variation on that scenario would be when the paragraph wraps over multiple lines, and the last line is short and ends with a link. I'd guess the same bug would reproduce there.

It sounds like the last TextSpan's recognizer is inappropriately being applied through the end of the line when the TextSpan visually doesn't extend that far. So this may be an upstream Flutter bug.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Jul 7, 2023

Ah indeed; the "short" part of "[i]f you have a short paragraph ending in a link" is not required. 🙂 (I observed the case you described in my testing and meant to include it in the report.)

@gnprice gnprice added this to the Launch milestone Sep 14, 2023
@gnprice
Copy link
Member

gnprice commented Jan 4, 2024

It looks like this was the following upstream issue (which appear to be dupes unless I'm missing something):

Those were closed by flutter/flutter#140621 (the same upstream change that induced #475, which is why I came across them). So it's likely this issue no longer reproduces. If not, we can close it.

gnprice added a commit to gnprice/zulip-flutter that referenced this issue 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 issue 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 that referenced this issue 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 that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants