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

Conversation

@photon3108
Copy link
Contributor

@photon3108 photon3108 commented Dec 15, 2019

Description

If you tap the nested TextSpan, it doesn't received the tap event by TapGestureRecognizer. That's because DOM-based implementation of ParagraphRuler.hitTest() doesn't pick the inner-most TextSpan up. I fixed it by arranging the search path with breadth-first traversal. There are more explanations in the comments of test-case.

Issues

Fixed: flutter/flutter#46975
Related: flutter/flutter#33523

Tests

I added a test-case for general implementation and that covers both cases of DOM-based and Canvas-based implementation.

@auto-assign auto-assign bot requested a review from jason-simmons December 15, 2019 08:28
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@photon3108
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@mdebbar mdebbar requested review from ferhatb and mdebbar December 16, 2019 19:06
@mdebbar mdebbar added the platform-web Code specifically for the web engine label Dec 16, 2019
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.

Thanks for fixing this bug and writing great tests and explanations!

LGTM

@photon3108
Copy link
Contributor Author

@jason-simmons Hi Jason. You were assigned to the default reviewer by Github. Are there anything I can improve this PR?

@mdebbar
Copy link
Contributor

mdebbar commented Dec 18, 2019

@photon3108 I don't think @jason-simmons will be reviewing this PR as it's a web-only change. @ferhatb and my approvals should be good enough. Please wait until all the checkmarks become green then merge the PR.

Thanks again for helping us improve Flutter for web!

@photon3108
Copy link
Contributor Author

@mdebbar Github doesn't authorize me to merge this issue. All are green but there is no merge button. I thought you were waiting for @jason-simmons' review. Could you help me merge it? Thanks.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 20, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 20, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 20, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 20, 2019
flar pushed a commit to flutter/flutter that referenced this pull request Dec 20, 2019
* bb118c6 Roll src/third_party/dart 8d11c1dce64a..642f8d052fd7 (1 commits) (flutter/engine#14574)

* 09c434d Use ELF for Dart AOT snapshots on Fuchsia. (flutter/engine#13896)

* e1e7851 Roll fuchsia/sdk/core/linux-amd64 from VdBKA... to uFFWW... (flutter/engine#14575)

* f317f8f Roll src/third_party/skia c76ac8e325c7..77742c350371 (1 commits) (flutter/engine#14576)

* 2ba5633 instructions for running firefox/safari tests (flutter/engine#14562)

* 9cf1e46 Roll src/third_party/dart 642f8d052fd7..7113fc79a83c (3 commits) (flutter/engine#14578)

* f5b877a [web] Run engine tests on Safari locally by launching safari installed on MacOS (flutter/engine#14555)

* 68d9196 Fix DOM-based ParagraphRuler.hitTest() (flutter/engine#14504)

* bb65df8 Roll src/third_party/skia 77742c350371..a8352ccaae37 (8 commits) (flutter/engine#14579)

* ad1ab56 Roll src/third_party/dart 7113fc79a83c..e50d98cd5651 (8 commits) (flutter/engine#14580)

* 22413ef Update formatting in web_ui scene bulder to match flutter style and dartfmt. (flutter/engine#14539)

* 6e825e7 Roll fuchsia/sdk/core/mac-amd64 from Ykb4b... to f51Q_... (flutter/engine#14584)

* 1d3bb8c Fix message_loop_fuchsia and thus enable fml_tests and flow_tests for Fuchsia (flutter/engine#14583)

* 1f7bb9d Wire up OpacityLayer to Scenic (flutter/engine#14577)

* 11db035 Roll src/third_party/skia a8352ccaae37..87e9ddb675b6 (11 commits) (flutter/engine#14585)

* bd58af7 Roll src/third_party/dart e50d98cd5651..141fcfa61092 (3 commits) (flutter/engine#14586)

* 929b1ed Engine support for ImageFiltered widget (flutter/engine#14491)

* 40b84fc Fix lint warnings across web_ui, add missing browserEngine case in text field. (flutter/engine#14535)

* ea1d330 Roll fuchsia/sdk/core/linux-amd64 from uFFWW... to 25LzW... (flutter/engine#14587)

* 854d5f8 Roll src/third_party/skia 87e9ddb675b6..7e2dea568299 (1 commits) (flutter/engine#14589)
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
* [web] Fix ParagraphRuler.hitTest

* [web] Add a test-case for paragraph.getPositionForOffset and nested TextSpans

* [web] Add a test-case for paragraph.getPositionForOffset and nested TextSpans

* [web] remove trailing spaces

* [web] avoid differences of overflow-wrap between chrome and firefox
@jlubeck
Copy link

jlubeck commented May 11, 2020

This is still broken on Flutter v1.17.0-3.4.pre

@jlubeck
Copy link

jlubeck commented May 11, 2020

On further investigating, looks like it breaks when the TextAlign used is something other than left.
If you actually tap on the area where the text WOULD be located if left was used, it detects it there. So in my specific case, I need to tap on an empty area next to the centered paragraph to have it detect it.

@photon3108 I'm tagging you since you are the contributor.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TapGestureRecognizer doesn't work on web, does work on mobile.

5 participants