Fixed render overflow error and implemented native solution of text s… #39624

imlegend19 · 2019-08-31T20:13:50Z

Description

Implemented the native Android solution for text selection.

Related Issues

Fixes #35826

Tests

I added the following tests:

should return true if can copy contents

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.

…election

justinmc

Thanks for doing this! I think this is a step in the direction of a really nice solution that looks native, but I've pointed out a few issues we should work through. This is more complicated that it looks on the surface. Let me know if I can help point you in the right direction for any of this.

The failures seem to be caused by this test:

flutter/packages/flutter/test/material/text_field_test.dart

Line 5776 in 0ed3242

'long press selects word and shows toolbar (Android)',

It's expecting to find 4 FlatButtons in the selection toolbar, but it only finds 3 since this PR hides any buttons beyond 3. The ideal solution here is definitely to move buttons to the submenu only when they physically exceed the width of the screen. I have some ideas about this in the comments below.

Also, on native Android, there is an animation between the menu open/closed states that we should include as well if possible:

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

justinmc · 2019-09-03T17:32:00Z

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

      return Container(width: 0.0, height: 0.0);
    }

+    if (items.length <= 3) {


The ideal solution is to adjust the number of items visible such that the maximum number fit on the screen without overflowing. Hardcoding it to 3 is a breaking change because the many tests that depend on 4 buttons being on the screen will break. It also may still overflow in languages with very long strings in the first 3 buttons.

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

packages/flutter/test/material/text_selection_test.dart

codecov · 2019-09-03T18:15:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e8c42e3). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39624   +/-   ##
=========================================
  Coverage          ?   60.61%           
=========================================
  Files             ?      194           
  Lines             ?    18849           
  Branches          ?        0           
=========================================
  Hits              ?    11426           
  Misses            ?     7423           
  Partials          ?        0

Flag	Coverage Δ
#flutter_tool	`60.61% <0%> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8c42e3...ac3eddf. Read the comment docs.

imlegend19 · 2019-09-18T15:05:25Z

Thank you sir for replying. I will surely make these changes!

justinmc · 2019-09-18T20:00:38Z

Thanks for following up @imlegend19! Let me know if I can help clarify anything!

imlegend19 · 2019-09-25T13:32:18Z

@justinmc can you help me in solving this merge conflict?

justinmc · 2019-09-25T15:34:23Z

I tried to do this myself, but I think I can't commit to your PR. So here's what you should do to fix the conflict.

It seems like there are two conflicts, both in material/text_selection.dart.

For the first conflict, items was changed to use Dart's new UI as code feature where if statements can go in the middle of Lists. So you should keep that format, but include your change of accessing the callbacks on widget:

    final MaterialLocalizations localizations = MaterialLocalizations.of(context);
    final List<Widget> items = <Widget>[
      if (widget.handleCut != null) FlatButton(child: Text(localizations.cutButtonLabel), onPressed: widget.handleCut),
      if (widget.handleCopy != null) FlatButton(child: Text(localizations.copyButtonLabel), onPressed: widget.handleCopy),
      if (widget.handlePaste != null) FlatButton(child: Text(localizations.pasteButtonLabel), onPressed: widget.handlePaste),
      if (widget.handleSelectAll != null) FlatButton(child: Text(localizations.selectAllButtonLabel), onPressed: widget.handleSelectAll),
    ];

For the second conflict you should just accept the changes. It seems like your change was just formatting.

    return delegate.selectAllEnabled &&
           value.text.isNotEmpty &&
           !(value.selection.start == 0 && value.selection.end == value.text.length);

imlegend19 · 2019-10-03T13:22:21Z

@justinmc I have made the changes what you requested, still the tests fail!

justinmc · 2019-10-03T15:33:19Z

Here's what the failure is:

  Expected: exactly 4 matching nodes in the widget tree
  Actual: ?:<3 widgets with type "FlatButton" (ignoring offstage widgets): [FlatButton(dependencies:
[_LocalizationsScope-[GlobalKey#aaf34], _InheritedTheme]), FlatButton(dependencies:
[_LocalizationsScope-[GlobalKey#aaf34], _InheritedTheme]), FlatButton(dependencies:
[_LocalizationsScope-[GlobalKey#aaf34], _InheritedTheme])]>
   Which: is not enough
When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///tmp/flutter%20sdk/packages/flutter/test/material/text_field_test.dart:5387:7)

Looks like it is probably a legitimate failure. You should be able to run material/text_field_test.dart and debug the failure locally.

imlegend19

I have made your changes but it still does not solve the issue.

imlegend19 · 2019-10-29T15:47:59Z

Here's what the failure is:

  Expected: exactly 4 matching nodes in the widget tree
  Actual: ?:<3 widgets with type "FlatButton" (ignoring offstage widgets): [FlatButton(dependencies:
[_LocalizationsScope-[GlobalKey#aaf34], _InheritedTheme]), FlatButton(dependencies:
[_LocalizationsScope-[GlobalKey#aaf34], _InheritedTheme]), FlatButton(dependencies:
[_LocalizationsScope-[GlobalKey#aaf34], _InheritedTheme])]>
   Which: is not enough
When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///tmp/flutter%20sdk/packages/flutter/test/material/text_field_test.dart:5387:7)

Looks like it is probably a legitimate failure. You should be able to run material/text_field_test.dart and debug the failure locally.

Here is the stacktrace:

Testing started at 9:11 PM ...
/home/user/AndroidStudioProjects/flutter/bin/flutter --no-color test --machine --start-paused test/material/text_field_test.dart
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure object was thrown running a test:
  Expected: one widget whose rasterized image matches golden image "text_field_opacity_test.0.3.png"
  Actual: ?:<exactly one widget with type "Overlay" (ignoring offstage widgets):
Overlay-[LabeledGlobalKey<OverlayState>#d7160](state: OverlayState#a240f(tickers: tracking 2
tickers, entries: [OverlayEntry#7e483(opaque: false; maintainState: false),
OverlayEntry#62d73(opaque: false; maintainState: true), OverlayEntry#be481(opaque: false;
maintainState: false), OverlayEntry#6f1f6(opaque: false; maintainState: false),
OverlayEntry#89204(opaque: false; maintainState: false)]))>
   Which: does not match

When the exception was thrown, this was the stack:
#0      fail (package:test_api/src/frontend/expect.dart:152:30)
#1      _expect.<anonymous closure> (package:test_api/src/frontend/expect.dart:127:9)
#13     _MatchesGoldenFile.matchAsync.<anonymous closure> (package:flutter_test/src/matchers.dart)
<asynchronous suspension>
#14     AutomatedTestWidgetsFlutterBinding.runAsync.<anonymous closure> (package:flutter_test/src/binding.dart:891:22)
#17     AutomatedTestWidgetsFlutterBinding.runAsync (package:flutter_test/src/binding.dart:889:26)
#18     _MatchesGoldenFile.matchAsync (package:flutter_test/src/matchers.dart:1721:20)
#20     _MatchesGoldenFile.matchAsync (package:flutter_test/src/matchers.dart:1701:28)
#21     _expect (package:test_api/src/frontend/expect.dart:116:26)
#22     expectLater (package:test_api/src/frontend/expect.dart:75:5)
#23     expectLater (package:flutter_test/src/widget_tester.dart:266:10)
#24     main.<anonymous closure> (file:///home/user/AndroidStudioProjects/flutter/packages/flutter/test/material/text_field_test.dart:497:11)
#38     AutomatedTestWidgetsFlutterBinding.runTest.<anonymous closure> (package:flutter_test/src/binding.dart:1028:17)
#40     AutomatedTestWidgetsFlutterBinding.runTest.<anonymous closure> (package:flutter_test/src/binding.dart:1016:35)
(elided 43 frames from class _FakeAsync, package dart:async, package dart:async-patch, and package stack_trace)

The test description was:
  text field selection toolbar renders correctly inside opacity
════════════════════════════════════════════════════════════════════════════════════════════════════

…election

# Conflicts: # packages/flutter/lib/src/material/text_selection.dart

googlebot · 2019-10-29T21:17:05Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Piinks · 2019-10-29T21:20:46Z

Hi @imlegend19,
Thank you for your work on this contribution! It looks like you may have rebased your branch and pulled in a bunch of unrelated commits. GitHub has a hard time reconciling when using
git rebase....
To fix this, you can use
git reflog...
to undo the rebasing, and then use
git pull upstream master...
instead. That should fix the commit issues.

googlebot · 2019-10-29T21:39:08Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

imlegend19 · 2019-10-29T21:40:13Z

Hi @imlegend19,
Thank you for your work on this contribution! It looks like you may have rebased your branch and pulled in a bunch of unrelated commits. GitHub has a hard time reconciling when using
git rebase....
To fix this, you can use
git reflog...
to undo the rebasing, and then use
git pull upstream master...
instead. That should fix the commit issues.

Thank you @Piinks for your help.

justinmc · 2019-10-30T15:56:50Z

It looks like that failure is due to breaking a goldens test, which is to be expected for this PR. Goldens tests are visual diff tests, so because the selection menu looks different in this PR, it will break any visual diff test that covers the selection menu. See the image from your stack trace, which does include the selection menu: https://github.com/flutter/goldens/blob/master/packages/flutter/test/material/text_field_opacity_test.0.4.png

I added the "breaking change" tag, and we'll need to follow the instructions for breaking changes when we're ready. We'll also need to update the golden tests when we're sure that this looks exactly how we want it to.

imlegend19 · 2019-10-30T17:02:55Z

How should we proceed in modifying the golden tests?

Piinks · 2019-10-30T17:09:01Z

I can help with the golden file updates when this is ready to land. :)
I would start by following the breaking changes procedures, as it will take a few days for that to be completed.

justinmc · 2020-01-16T01:10:30Z

@imlegend19 Are you still available for working on this PR? I might need to take over this work soon since this is a major bug for people.

justinmc · 2020-01-23T23:55:15Z

I've started investigating my own solution in #49391. I'll close this PR if I am able to merge that one.

imlegend19 · 2020-01-29T04:40:35Z

Hi, I am working on this PR but still stuck in the same error!

myriky · 2020-01-29T06:34:33Z

any updates?

Piinks · 2020-01-29T21:58:44Z

Hi, I am working on this PR but still stuck in the same error!

Hi @imlegend19 can you update your branch? It looks like this PR has aged a bit and now has merge conflicts with the master branch.

justinmc · 2020-01-29T22:57:37Z

@imlegend19 I'm getting close to finishing my solution in #49391. It will make this PR redundant, unfortunately. Sorry about the overlapping work.

I recommend we close this PR if you're alright with that. If you still want to help out on this problem, we still need to do a similar solution for iOS, and no one has started that yet that I know of.

justinmc · 2020-02-07T23:45:33Z

I'm closing this in favor of #49391. If anyone sees any problems that that PR doesn't cover, please let me know!

Fixed render overflow error and implemented native solution of text s…

e5055b0

…election

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 31, 2019

googlebot added the cla: yes label Aug 31, 2019

imlegend19 mentioned this pull request Aug 31, 2019

Fixed Render Overflow error on text selection #38561

Closed

9 tasks

imlegend19 added 2 commits September 1, 2019 02:03

modified test

9b3bdbd

fixed flutter analyzer issue

86e1944

justinmc requested changes Sep 3, 2019

View reviewed changes

justinmc added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Sep 6, 2019

made the requested changes in the code and added todos

88769f2

justinmc mentioned this pull request Sep 23, 2019

TextField's toolbar options look very ugly #41047

Closed

made the requested changes in the code

4522b37

HansMuller mentioned this pull request Oct 18, 2019

Upcoming Material Library Features #43035

Closed

4 tasks

imlegend19 commented Oct 29, 2019

View reviewed changes

modified some tests

cc6accc

imlegend19 added 4 commits October 29, 2019 21:30

Fixed render overflow error and implemented native solution of text s…

83c4bd6

…election

modified test

c92dd70

modified some tests

eadebfc

Merge remote-tracking branch 'origin/master'

e8c42e3

# Conflicts: # packages/flutter/lib/src/material/text_selection.dart

imlegend19 requested review from Piinks and jmagman as code owners October 29, 2019 21:15

googlebot added cla: no and removed cla: yes labels Oct 29, 2019

Merge branch 'master' of github.com:flutter/flutter

ac3eddf

googlebot added cla: yes and removed cla: no labels Oct 29, 2019

justinmc added the c: API break Backwards-incompatible API changes label Oct 30, 2019

justinmc mentioned this pull request Nov 13, 2019

TextSelection could misbehave inside a Nested Overlay #43787

Closed

justinmc closed this Feb 7, 2020

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

Fixed render overflow error and implemented native solution of text s… #39624

Fixed render overflow error and implemented native solution of text s… #39624

Uh oh!

Conversation

imlegend19 commented Aug 31, 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!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinmc Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

imlegend19 commented Sep 18, 2019

Uh oh!

justinmc commented Sep 18, 2019

Uh oh!

imlegend19 commented Sep 25, 2019

Uh oh!

justinmc commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imlegend19 commented Oct 3, 2019

Uh oh!

justinmc commented Oct 3, 2019

Uh oh!

imlegend19 left a comment

Choose a reason for hiding this comment

Uh oh!

imlegend19 commented Oct 29, 2019

Uh oh!

googlebot commented Oct 29, 2019

Uh oh!

Piinks commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 29, 2019

Uh oh!

imlegend19 commented Oct 29, 2019

Uh oh!

justinmc commented Oct 30, 2019

Uh oh!

imlegend19 commented Oct 30, 2019

Uh oh!

Piinks commented Oct 30, 2019

Uh oh!

justinmc commented Jan 16, 2020

Uh oh!

justinmc commented Jan 23, 2020

Uh oh!

imlegend19 commented Jan 29, 2020

Uh oh!

myriky commented Jan 29, 2020

Uh oh!

Piinks commented Jan 29, 2020

Uh oh!

justinmc commented Jan 29, 2020

Uh oh!

justinmc commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

imlegend19 commented Aug 31, 2019 •

edited

Loading

codecov bot commented Sep 3, 2019 •

edited

Loading

justinmc commented Sep 25, 2019 •

edited

Loading

Piinks commented Oct 29, 2019 •

edited

Loading