Skip to content

Text selection handles are sometimes not interactive - #31852

Merged
justinmc merged 32 commits into
flutter:masterfrom
justinmc:immovable-selection-handles
May 30, 2019
Merged

justinmc merged 32 commits into
flutter:masterfrom
justinmc:immovable-selection-handles

Conversation

@justinmc

@justinmc justinmc commented Apr 30, 2019 •

Copy link
Copy Markdown
Contributor

Closes #31286 #23432

Text selection handles are being moved outside of their enclosing GestureDetector, making them not interactive. I have seen this for the left Material handle at the start of a text field and for the left Cupertino handle at any position.

@justinmc justinmc self-assigned this Apr 30, 2019
@goderbauer goderbauer added p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Apr 30, 2019
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 03107ef to 0bf58c0 Compare April 30, 2019 23:18
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 7002a32 to aa6c887 Compare May 2, 2019 22:21
@justinmc

justinmc commented May 2, 2019 •

Copy link
Copy Markdown
Contributor Author

I've gotten the GestureDetectors to be the correct size and position for all of the different text selection handles that we have.

However, despite the GestureDetectors being the right size and in the right place, there are parts of them that are sometimes still not interactive. I think that's because of the CompositedTransformFollower that we use to reset the coordinate space.

From the docs, "Hit testing on descendants of this widget will only work if the target position is within the box that this widget's parent considers to be hitable." I can't quite get a perfect visual on exactly where the widget's boundary is and if it lines up perfectly with the non-interactive regions I'm seeing, though.

Update: Hit detection works in the places we need it now as of dcf30cf.

@justinmc
justinmc force-pushed the immovable-selection-handles branch from aa6c887 to da9aa08 Compare May 2, 2019 22:33
@justinmc
justinmc force-pushed the immovable-selection-handles branch from da9aa08 to 056f5e1 Compare May 3, 2019 16:53
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 363f071 to 49b8d57 Compare May 7, 2019 21:59
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 49b8d57 to c66739c Compare May 10, 2019 23:25
@justinmc

Copy link
Copy Markdown
Contributor Author

The last problem that I'm fighting with is that the GestureDetector swallows tap events from the text field behind it. This manifests as double tapping to select text being broken (the first tap shows the handle, which blocks the second tap).

@HansMuller I've got a working solution by using Listener instead of GestureDetector, like we talked about. I was able to reuse the dragging logic from GestureDetector, but I reimplemented tap myself (poorly) because reusing any code caused the event to be swallowed. I'll talk to you on Monday about it, and hopefully I can bypass having to write my own tap if I understand a bit more about the event arena.

@justinmc
justinmc force-pushed the immovable-selection-handles branch from 4d1e0c9 to 3a848ef Compare May 15, 2019 17:28
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 1f2364c to e0be739 Compare May 16, 2019 16:40
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 386ab39 to 80382f0 Compare May 28, 2019 20:09
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 80382f0 to 43241c3 Compare May 28, 2019 20:10
…enu and expanding selection, so just document it
@justinmc
justinmc force-pushed the immovable-selection-handles branch from 19b0ae3 to 3a060b7 Compare May 29, 2019 17:12
@justinmc
justinmc requested a review from HansMuller May 29, 2019 17:46
@justinmc justinmc changed the title WIP Text selection handles are sometimes not interactive Text selection handles are sometimes not interactive May 29, 2019
@justinmc

justinmc commented May 29, 2019 •

Copy link
Copy Markdown
Contributor Author

@HansMuller This is ready for review. The main things this PR does to achieve its goal of making the handles nicely interactive are:

  1. Refactor how handles are drawn so that they can be easily padded to a nice 48x48 interactive size. TextSelectionControls renders the handles in an encapsulated way, and the parent TextSelectionHandleOverlayState handles padding them and putting them in the right place wrapped in a GestureDetector.
  2. Changed how the selection overlay uses CompositedTransformFollower and the gesture arena to get interactions to work on both handles and text field at the same time. This was the big time sink as there were many gestures that I needed to get right while still allowing 48x48 interaction with and without overlap of the underlying text field. See my big list of sneaky gestures above.

Also, do you have any idea what is going on with the tests?

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great, just a few small suggestions for simplifications, etc.

Comment thread packages/flutter/lib/src/material/text_selection.dart Outdated
Comment thread packages/flutter/lib/src/material/text_selection.dart Outdated
Comment thread packages/flutter/lib/src/widgets/text_selection.dart
Comment thread packages/flutter/lib/src/widgets/text_selection.dart Outdated
Comment thread packages/flutter/lib/src/widgets/text_selection.dart Outdated
Comment thread packages/flutter/lib/src/widgets/text_selection.dart Outdated
Comment thread packages/flutter/lib/src/widgets/text_selection.dart Outdated

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@justinmc
justinmc force-pushed the immovable-selection-handles branch from fd47bef to b8870ac Compare May 30, 2019 16:29
@justinmc
justinmc force-pushed the immovable-selection-handles branch from b8870ac to d704bec Compare May 30, 2019 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iOS Textfield caret "start handle"'s top balloon does not allow drag

4 participants