Fix text field selection toolbar under Opacity #31097

liyuqian · 2019-04-15T23:52:22Z

Description

As we've introduced offset to the Opacity layer, we have to override
applyTransform to make Leader/FollowerLayer work correctly.

Related Issues

Fixes #30587

Together with flutter/engine#8585,
this test will also exercise test against
#30586.

Tests

I added the following tests:

text field selection toolbar renders correctly inside opacity

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.

liyuqian · 2019-04-15T23:53:02Z

WIP, the test won't pass until

We fixed TextField inside Opacity has wrong selection Leader/Follower transformation #30587
We uploaded the golden file.

Hixie · 2019-04-17T03:10:13Z

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

there's a Matrix4.translate method, which would make this more efficient

Hixie · 2019-04-17T03:10:56Z

packages/flutter/test/material/text_field_test.dart

it's more efficient to just pump once with the required amount of time to skip the animation

I've removed all the previous pumps. However, I couldn't change this pumpAndSettle to even pump(Duration(seconds: 100)) to pass the test. It'll pass with await tester.pump(); await tester.pump(Duration(seconds: 1)); though. Is this a bug or an expected behavior?

I've also added the first pump back in the newest patch. Otherwise, the "COPY/CUT/PASTE" will appear in a different order which causes a golden image mismatch (see attached images). Is this also an expected behavior?

No idea why the order would change...

looks like you still have pumpAndSettle?

it's significantly preferred to use pump than pumpAndSettle, because pumpAndSettle will mask regressions.

Changed to pump.

One suggestion is to capture the return value of pumpAndSettle and expect the result of it (or expect that it's less than some sane value).

I guess the regression that @Hixie mentioned is about the regression in seconds (e.g., the animation should finish in 1 second but a regression made it 10 seconds). I'm not sure if the return value of pumpAndSettle would catch that. What's your thought, @Hixie ?

The return value would catch it but it's more likely that someone fixing it will just change the number and it's more likely that a reviewer will fail to notice that.

To catch issues like flutter/flutter#30586 flutter/flutter#31097 will trigger this CHECK if #8467 were reverted and the transform_ were not initialized in this PR.

liyuqian · 2019-04-18T17:17:51Z

Ping...

As we've introduced offset to the Opacity layer, we have to override `applyTransform` to make Leader/FollowerLayer work correctly. Fixes flutter#30587 Together with flutter/engine#8585, this test will also exercise test against flutter#30586.

Otherwise, the order of "COPY/CUT/PASTE" will be somehow different and it'll cause a golden image mismatch.

Hixie · 2019-04-23T23:31:30Z

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

+    assert(transform != null);
+    transform.translate(offset.dx, offset.dy);
+  }
+


fyi @goderbauer

goderbauer

LGTM

goderbauer · 2019-04-29T10:37:27Z

packages/flutter/test/material/text_field_test.dart

+      find.byType(Overlay),
+      matchesGoldenFile('text_field_opacity_test.0.0.png'),
+    );
+  }, skip: !Platform.isLinux);


Do you really want to skip the entire test on all non-linux platforms? Or just the golden check? In the later, I'd recommend putting the skip on "expectLater". (It looks like everything else should also work on other platforms?)

Good catch! Fixed.

jonahwilliams · 2019-04-29T19:28:20Z

This is breaking several devicelab tasks, and failing a golden check. Do these need to be updated, or were they unexpected? cc @liyuqian

This reverts commit e427c2d.

)

This reverts commit 120a1fc.

) This reverts commit 120a1fc. Updated the golden file

googlebot added the cla: yes label Apr 15, 2019

liyuqian force-pushed the opacity_text_field_test branch from fe5f3ba to c8f09e9 Compare April 16, 2019 20:35

liyuqian requested review from HansMuller and gspencergoog April 16, 2019 20:39

liyuqian changed the title ~~[wip] Opacity text field test~~ Fix text field selection toolbar under Opacity Apr 16, 2019

liyuqian requested review from GaryQian and jason-simmons April 16, 2019 20:41

liyuqian force-pushed the opacity_text_field_test branch from c8f09e9 to 031a85c Compare April 16, 2019 21:53

liyuqian requested a review from Hixie April 16, 2019 21:59

liyuqian mentioned this pull request Apr 16, 2019

Initialize OpacityLayer's matrix to identity flutter/engine#8467

Merged

Hixie added the framework flutter/packages/flutter repository. See also f: labels. label Apr 16, 2019

Hixie reviewed Apr 17, 2019

View reviewed changes

liyuqian added 4 commits April 18, 2019 10:18

Check

ff865d3

Matrix.translate and remove unnecessary pump

a754465

Add one pump back

e94307c

Otherwise, the order of "COPY/CUT/PASTE" will be somehow different and it'll cause a golden image mismatch.

liyuqian force-pushed the opacity_text_field_test branch from 324c26a to e94307c Compare April 18, 2019 17:24

Hixie reviewed Apr 23, 2019

View reviewed changes

liyuqian requested review from dnfield and goderbauer April 23, 2019 23:53

Use pump instead of pumpAndSettle

7c0c48c

goderbauer approved these changes Apr 29, 2019

View reviewed changes

Only skip golden test

10b8e21

liyuqian merged commit e427c2d into flutter:master Apr 29, 2019

jonahwilliams pushed a commit that referenced this pull request Apr 29, 2019

Revert "Fix text field selection toolbar under Opacity (#31097)"

1a95b23

This reverts commit e427c2d.

jonahwilliams mentioned this pull request Apr 29, 2019

Revert "Fix text field selection toolbar under Opacity" #31800

Merged

jonahwilliams pushed a commit that referenced this pull request Apr 29, 2019

Revert "Fix text field selection toolbar under Opacity (#31097)" (#31800

120a1fc

)

liyuqian added a commit to liyuqian/flutter that referenced this pull request Apr 29, 2019

Reland "Fix text field selection toolbar under Opacity (flutter#31097)"

6392d6d

This reverts commit 120a1fc.

liyuqian added a commit that referenced this pull request Apr 30, 2019

Reland "Fix text field selection toolbar under Opacity (#31097)" (#31802

71a89a4

) This reverts commit 120a1fc. Updated the golden file

liyuqian deleted the opacity_text_field_test branch May 1, 2019 22:26

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

Fix text field selection toolbar under Opacity #31097

Fix text field selection toolbar under Opacity #31097

Uh oh!

Conversation

liyuqian commented Apr 15, 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!

liyuqian commented Apr 15, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Apr 18, 2019

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!

jonahwilliams commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liyuqian commented Apr 15, 2019 •

edited

Loading