Selection handles position is off by chunhtai · Pull Request #34665 · flutter/flutter

chunhtai · 2019-06-18T19:46:03Z

… non-obscureText

Description

Added a logic to check whether an update to selection overlay is necessary whenever editable text gets rebuilt.

Related Issues

#34607

Tests

I added the following tests:
'selection overlay will update when text grow bigger'

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

breaking change draft: https://docs.google.com/document/d/1JI-5idfRxzqcue3euGeJBuCtsEhnX7_VMeeb6Qp2Bwg/edit

goderbauer · 2019-06-19T16:26:20Z

packages/flutter/lib/src/widgets/text_selection.dart

remove whitespace

goderbauer · 2019-06-19T16:28:31Z

packages/flutter/lib/src/widgets/text_selection.dart

nit: // instead of ///

goderbauer · 2019-06-19T16:29:57Z

packages/flutter/lib/src/widgets/text_selection.dart

The overlay position can change even without changes in selection, for example when the font size changes.

goderbauer · 2019-06-19T16:30:09Z

packages/flutter/lib/src/widgets/text_selection.dart

... if their position ...

goderbauer · 2019-06-19T16:31:06Z

packages/flutter/lib/src/widgets/text_selection.dart

space after if

goderbauer · 2019-06-19T18:21:05Z

packages/flutter/lib/src/widgets/editable_text.dart

Why else if? Don't we need to call _selectionIOverlay.update if the controller and the selection on it changed? (we should add a test to verify that this works)

goderbauer · 2019-06-19T18:26:19Z

packages/flutter/lib/src/widgets/text_selection.dart

Piping this back like this feels strange...

goderbauer · 2019-06-19T18:26:55Z

packages/flutter/test/widgets/editable_text_test.dart

nit: indentation

goderbauer · 2019-06-19T18:30:53Z

packages/flutter/test/widgets/editable_text_test.dart

tester.widgetList(find.byType(...))?

goderbauer · 2019-06-19T18:31:05Z

packages/flutter/test/widgets/editable_text_test.dart

goderbauer · 2019-06-19T19:47:38Z

Could this be solved by having the RenderEditable insert LeaderLayers [1] where the selection handles are supposed to go and the selection handles itself are drawn on a FollowerLayer [2], which is always positioned correctly to be in the right place even when the text size in the RenderEditable changes?

See also the Widgets CompositedTransformTarget [3] and CompositedTransformFollower [4], which insert these kind of layers.

Ian mentioned that @xster was talking about doing this at some point to fix a similar class of problems.

[1] https://master-api.flutter.dev/flutter/rendering/LeaderLayer-class.html
[2] https://master-api.flutter.dev/flutter/rendering/LeaderLayer-class.html
[3] https://master-api.flutter.dev/flutter/widgets/CompositedTransformTarget-class.html
[4] https://master-api.flutter.dev/flutter/widgets/CompositedTransformFollower-class.html

xster · 2019-06-21T00:53:32Z

+1. @matthew-carroll had a prototype implementing the iOS text selection magnifier using this approach.

chunhtai · 2019-06-24T21:46:16Z

finished refactoring ready for review

justinmc · 2019-06-25T17:40:40Z

This made the Cupertino handles appear on the next line when I tried it:

chunhtai · 2019-06-25T22:57:18Z

This made the Cupertino handles appear on the next line when I tried it:

thanks for catching this bug, I refined the offset calculation.

justinmc · 2019-06-26T16:15:23Z

Thanks for the fix! The handles look right to me now.

justinmc · 2019-06-26T16:24:49Z

Another thing I'm noticing is that the interactivity improvements that I made in #31852 seem to be gone here. The handles are very hard to touch for both Material and Cupertino.

chunhtai · 2019-06-26T18:23:54Z

Another thing I'm noticing is that the interactivity improvements that I made in #31852 seem to be gone here. The handles are very hard to touch for both Material and Cupertino.

It turns out I removed a bunch of them when i was experimenting the offset calculation. Added back

justinmc · 2019-06-27T16:08:08Z

Thanks, the handles feel great now.

I've got one more "QA" bug for you, sorry for the bombardment haha. The handle sometimes appears outside the input for both TextField and CupertinoTextField:

justinmc

See my concern about breaking changes. Otherwise this looks pretty good and seems to work great now. I like the LayerLink approach.

justinmc · 2019-07-01T15:57:08Z

packages/flutter/lib/src/widgets/text_selection.dart

justinmc · 2019-07-01T16:02:19Z

packages/flutter/lib/src/rendering/editable.dart

Is this a breaking change?

justinmc · 2019-07-01T16:06:11Z

packages/flutter/lib/src/widgets/text_selection.dart

More potential breaking changes.

goderbauer

Please update the PR with the breaking changes.

goderbauer · 2019-07-01T16:58:37Z

packages/flutter/lib/src/rendering/editable.dart

Update the documentation to describe what this parameter is?

goderbauer · 2019-07-01T16:59:05Z

packages/flutter/lib/src/rendering/editable.dart

the -> The

Also, this probably deserves a slightly more detailed documentation.

goderbauer · 2019-07-01T16:59:31Z

packages/flutter/lib/src/rendering/editable.dart

capital H for Handle?

goderbauer · 2019-07-01T17:00:27Z

packages/flutter/lib/src/rendering/editable.dart

nit: add trailing comma and indent by two spaces.

goderbauer · 2019-07-01T17:00:37Z

packages/flutter/lib/src/rendering/editable.dart

goderbauer · 2019-07-01T17:01:45Z

packages/flutter/lib/src/rendering/editable.dart

Since we always require two: would it be more meaningful to just have to separate parameters with meaningful names instead of a list?

(e.g. leading/trailingHandleLayerLink)?

same elsewhere where the list is used.

goderbauer · 2019-07-01T17:03:31Z

packages/flutter/lib/src/widgets/editable_text.dart

nit: remove space before :

goderbauer · 2019-07-01T17:05:10Z

packages/flutter/lib/src/widgets/text_selection.dart

This is surprising, why does the textDirection determine the handle type?

textdirection will change the order of handles
ltr:
start -> left handle, end -> right handle
rtl:
start -> right handle, end -> left handle

but I think i can avoid this parameter, I was thinking of getting rid of renderObject dependency from this class, but it seems I will still need it to calculate the handlesize

chunhtai · 2019-07-01T21:15:11Z

Addressed comment, attached break change email draft. Ready for review

justinmc

LGTM. I requested access to the breaking change email, so I'll take a look at that later.

Update: Got access and it looks good too!

goderbauer

LGTM

goderbauer · 2019-07-01T22:46:40Z

packages/flutter/lib/src/rendering/editable.dart

nit [LayerLink]

goderbauer · 2019-07-01T22:46:51Z

packages/flutter/lib/src/rendering/editable.dart

goderbauer · 2019-07-01T22:47:45Z

packages/flutter/lib/src/widgets/editable_text.dart

nit: Just group these three together without blank lines in between since they are logically similar.

* commit 'a0c47e2216a2b50b70c478001cf5652f31a783af': (187 commits) Do not use ideographic baseline for RenderPargraph baseline (flutter#35493) Add type to StreamChannel in generated test code. (flutter#35367) Fix RenderFittedBox when child.size.isEmpty (flutter#35487) add APK build time benchmarks (flutter#35481) fix Selection handles position is off (flutter#34665) Move usage flutter create tests into memory filesystem. (flutter#35160) Include tags in SemanticsNode debug properties (flutter#35491) Re-apply 'Add currentSystemFrameTimeStamp to SchedulerBinding' (flutter#35492) CupertinoTextField vertical alignment (flutter#34723) Mark update-packages as non-experimental (flutter#35467) fix default artifacts to exclude ios and android (flutter#35303) Roll engine ffba2f6..7d3e722 (59 commits) (flutter#35489) Update macrobenchmarks README and app name (flutter#35477) mark windows and macos chrome dev mode as flaky (flutter#35495) Use the new service protocol message names (flutter#35482) Manual roll of engine 45b66b7...ffba2f6 (flutter#35464) more ui-as-code (flutter#35393) update reassemble doc (flutter#35164) New parameter for RawGestureDetector to customize semantics mapping (flutter#33936) Add --target support for Windows and Linux (flutter#34660) ...

googlebot added the cla: yes label Jun 18, 2019

chunhtai requested review from goderbauer and justinmc June 18, 2019 19:46

chunhtai added the framework flutter/packages/flutter repository. See also f: labels. label Jun 18, 2019

goderbauer reviewed Jun 19, 2019

View reviewed changes

chunhtai changed the title ~~Selection handles position is off when switch between obscureText and…~~ WIP Selection handles position is off - big refactor incoming Jun 20, 2019

chunhtai added the work in progress; do not review label Jun 20, 2019

chunhtai force-pushed the issues/34607 branch 2 times, most recently from b0d3a45 to 7ef13d0 Compare June 24, 2019 20:02

chunhtai removed the work in progress; do not review label Jun 24, 2019

chunhtai changed the title ~~WIP Selection handles position is off - big refactor incoming~~ Selection handles position is off Jun 24, 2019

justinmc reviewed Jul 1, 2019

View reviewed changes

goderbauer reviewed Jul 1, 2019

View reviewed changes

chunhtai added the c: API break Backwards-incompatible API changes label Jul 1, 2019

Piinks added the a: text input Entering text in a text field or keyboard related problems label Jul 1, 2019

justinmc approved these changes Jul 1, 2019

View reviewed changes

goderbauer approved these changes Jul 1, 2019

View reviewed changes

fix Selection handles position is off

44fc7a1

chunhtai force-pushed the issues/34607 branch from 366c3fc to 44fc7a1 Compare July 2, 2019 17:39

chunhtai merged commit f143fd6 into flutter:master Jul 3, 2019

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

fix Selection handles position is off (flutter#34665)

45bed27

justinmc mentioned this pull request Aug 6, 2019

Text selection handles display outside when toggle obscureText value in TextField #37548

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021

Conversation

chunhtai commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jun 19, 2019

Uh oh!

xster commented Jun 21, 2019

Uh oh!

chunhtai commented Jun 24, 2019

Uh oh!

justinmc commented Jun 25, 2019

Uh oh!

chunhtai commented Jun 25, 2019

Uh oh!

justinmc commented Jun 26, 2019

Uh oh!

justinmc commented Jun 26, 2019

Uh oh!

chunhtai commented Jun 26, 2019

Uh oh!

justinmc commented Jun 27, 2019

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

chunhtai commented Jun 18, 2019 •

edited

Loading

justinmc left a comment •

edited

Loading