Text selection located wrong position when selecting multiple lines over max lines by takassh · Pull Request #102747 · flutter/flutter

takassh · 2022-04-28T11:58:20Z

This PR fixes bugs that text selection located wrong position when selecting multiple lines over max lines.

related closed pr #96471

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

This reverts commit 201b368.

takassh · 2022-04-28T12:59:37Z

hi
test is failed because of When renders below a block of text, menu appears below bottom endpoint.
I want to confirm whether it is expected behavior.

master	pr

takassh · 2022-05-10T06:31:25Z

cc @chunhtai

chunhtai

logic looks good, left some comments

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

packages/flutter/test/material/text_selection_test.dart

takassh · 2022-05-11T10:41:29Z

thanks for quick review.
I fixed them and added a new test for CupertinoTextField.
@chunhtai

chunhtai

LGTM

takassh · 2022-05-17T02:32:22Z

@justinmc
Hi, what should I do next?

justinmc

Thanks for fixing this! FYI It looks like there is a legitimate test failure in text_selection_test.dart.

Also, about your screenshots in #102747 (comment). I'm not convinced that the new menu location is correct. Shouldn't it be below? On native Android I see the selection color expand for the full width of the input, which Flutter might also be getting wrong unrelated to this PR, and the toolbar goes below.

justinmc · 2022-05-17T20:32:28Z

packages/flutter/lib/src/cupertino/text_selection_toolbar.dart

        delegate: TextSelectionToolbarLayoutDelegate(
          anchorAbove: anchorAbove - localAdjustment - contentPaddingAdjustment,
          anchorBelow: anchorBelow - localAdjustment + contentPaddingAdjustment,
+          fitsAbove: fitsAbove,


Previously I wasn't passing this because TextSelectionToolbarLayoutDelegate could calculate it itself (see the docs), but I suppose we can pass it in here to avoid calculating it twice.

takassh · 2022-05-19T11:12:22Z

@justinmc
ok, then I put anchorBelow back.

I can pass all tests on local, but it doesn't pass on github actions. can you take a look?

takassh · 2022-05-27T04:22:04Z

hi @justinmc

I'm not sure why Linux web_long_running_tests_1_5 failed.
please tell me what to do?
(sorry for bothering you)

takassh · 2022-05-31T12:30:13Z

@justinmc
finally, I could pass all tests.
please review it.

justinmc

LGTM 👍

Thanks for fixing this. I tried running your branch with the sample in the issue and it worked correctly for me. Glad to see you got the tests passing.

justinmc · 2022-05-31T18:02:50Z

packages/flutter/test/cupertino/text_selection_test.dart

    );
+
+    testWidgets(
+      'When select multiple lines over max lines',


Nit: "select" => "selecting" Here and in the other test.

justinmc · 2022-05-31T18:03:39Z

packages/flutter/test/cupertino/text_selection_test.dart

+        expect(find.text('Select All'), findsNothing);
+        expect(find.text('◀'), findsNothing);
+        expect(find.text('▶'), findsNothing);
+


Nit: Maybe add a comment like this? "The menu appears at the top of the visible selection."

And same for the other test below.

justinmc · 2022-05-31T18:05:04Z

packages/flutter/test/cupertino/text_selection_test.dart

+        final Offset cutOffset = tester.getTopLeft(find.text('Cut'));
+        final Offset textFieldOffset =
+            tester.getTopLeft(find.byType(CupertinoTextField));
+        expect(cutOffset.dy + 36, equals(textFieldOffset.dy));


Nit: Is there any way to explain where this 36 number comes from?

justinmc · 2022-05-31T18:05:15Z

packages/flutter/test/material/text_selection_test.dart

+        final Offset cutOffset = tester.getTopLeft(find.text('Cut'));
+        final Offset textFieldOffset =
+            tester.getTopLeft(find.byType(TextField));
+        expect(cutOffset.dy + 25, equals(textFieldOffset.dy));


Same question about explaining this 25 value.

takassh · 2022-06-02T05:59:24Z

@justinmc
thanks for review
added comments

justinmc · 2022-06-04T00:15:51Z

This is good to go. I'm writing myself a note to merge this on Monday.

takassh · 2022-06-04T02:41:23Z

@justinmc
thank you!

btw, you mentioned it before

the selection color expand for the full width of the input, which Flutter might also be getting wrong unrelated to this PR

#102747 (review)

this issue has already been reported? I wanna contribute it next if I can.

justinmc · 2022-06-07T17:14:56Z

@takassh No I have not seen an issue for the width of the selection highlight. That's great if you want to work on it! Could you create one and mention me?

… lines over max lines (flutter/flutter#102747)

…ver max lines (flutter#102747) Fix for text selection toolbar position in cases with overflowing text.

… lines over max lines (flutter/flutter#102747)

takassh added 3 commits April 28, 2022 15:31

Update text_selection.dart

201b368

Revert "Update text_selection.dart"

21a12f3

This reverts commit 201b368.

Update text_selection

713536c

Remove trailing U+0020 space character

f498bc5

chunhtai reviewed May 10, 2022

View reviewed changes

takassh added 2 commits May 11, 2022 19:06

Add test for cupertino

77b6983

remove space

73fcead

chunhtai approved these changes May 11, 2022

View reviewed changes

chunhtai requested a review from justinmc May 11, 2022 16:25

justinmc reviewed May 17, 2022

View reviewed changes

takassh and others added 2 commits May 19, 2022 19:32

Remove anchorBottom

3dd7066

Merge branch 'flutter:master' into text_selection

07cb984

huycozy mentioned this pull request May 30, 2022

Text Editing Toolbar is displayed in the wrong position for single line editable text when EditableText is overflown #104945

Closed

Merge branch 'upstream/master' into text_selection

1e79d87

justinmc approved these changes May 31, 2022

View reviewed changes

Add comments

348e546

justinmc merged commit 4b4c876 into flutter:master Jun 7, 2022

engine-flutter-autoroll mentioned this pull request Jun 7, 2022

Roll Flutter from 36fe51524f72 to 4b4c876fb9e9 (218 revisions) flutter/packages#2220

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022

4b4c876 Text selection located wrong position when selecting multiple…

5a4ef34

… lines over max lines (flutter/flutter#102747)

engine-flutter-autoroll mentioned this pull request Jun 7, 2022

Roll Flutter from 36fe51524f72 to 5ee59fa9ec15 (219 revisions) flutter/packages#2221

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022

4b4c876 Text selection located wrong position when selecting multiple…

77a11a3

… lines over max lines (flutter/flutter#102747)

engine-flutter-autoroll mentioned this pull request Jun 8, 2022

Roll Flutter from be0c1bd0f0ce to 4ec296513cb2 (24 revisions) flutter/plugins#5935

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 8, 2022

4b4c876 Text selection located wrong position when selecting multiple…

023038c

… lines over max lines (flutter/flutter#102747)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Text selection located wrong position when selecting multiple lines o…

68d83c5

…ver max lines (flutter#102747) Fix for text selection toolbar position in cases with overflowing text.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

4b4c876 Text selection located wrong position when selecting multiple…

e3deebd

… lines over max lines (flutter/flutter#102747)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

4b4c876 Text selection located wrong position when selecting multiple…

b0b31ca

… lines over max lines (flutter/flutter#102747)

Conversation

takassh commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

takassh commented Apr 28, 2022

Uh oh!

takassh commented May 10, 2022

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

takassh commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

takassh commented May 17, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc May 17, 2022

Choose a reason for hiding this comment

Uh oh!

takassh commented May 19, 2022

Uh oh!

takassh commented May 27, 2022

Uh oh!

takassh commented May 31, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc May 31, 2022

Choose a reason for hiding this comment

Uh oh!

justinmc May 31, 2022

Choose a reason for hiding this comment

Uh oh!

justinmc May 31, 2022

Choose a reason for hiding this comment

Uh oh!

justinmc May 31, 2022

Choose a reason for hiding this comment

Uh oh!

justinmc May 31, 2022

Choose a reason for hiding this comment

Uh oh!

takassh commented Jun 2, 2022

Uh oh!

justinmc commented Jun 4, 2022

Uh oh!

takassh commented Jun 4, 2022

Uh oh!

justinmc commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

takassh commented Apr 28, 2022 •

edited

Loading

takassh commented May 11, 2022 •

edited

Loading