8000 Revert "ListTile Material Ripple and Shape Patch (#74373)" by gspencergoog · Pull Request #76134 · flutter/flutter · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Revert "ListTile Material Ripple and Shape Patch (#74373)" #76134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 16, 2021
Merged

Revert "ListTile Material Ripple and Shape Patch (#74373)" #76134

merged 1 commit into from
Feb 16, 2021

Conversation

gspencergoog
Copy link
Contributor

This reverts commit 422916d because it causes unexpected changes in the rendering of the background of a ListTile.

The left side is before the change, and the right side is after (and the pink part is the diff).

Screenshot from 2021-02-16 10-44-20

Notice that the light grey background disappears.

/cc @NWalker1208

@gspencergoog
Revert "ListTile Material Ripple and Shape Patch (flutter#74373)"
35b1e58 
This reverts commit 422916d because it causes unexpected changes in the rendering of the background of a ListTile.
@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 16, 2021
@google-cla google-cla bot added the cla: yes label Feb 16, 2021
goderbauer
goderbauer approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member
@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@gspencergoog gspencergoog merged commit f8cd24d into flutter:master Feb 16, 2021
@gspencergoog gspencergoog deleted the revert_listtile branch February 16, 2021 19:24
@NWalker1208
Copy link
Contributor
NWalker1208 commented Feb 16, 2021 •
edited
Loading

This reverts commit 422916d because it causes unexpected 8000 changes in the rendering of the background of a ListTile.

The left side is before the change, and the right side is after (and the pink part is the diff).

Screenshot from 2021-02-16 10-44-20

Notice that the light grey background disappears.

/cc @NWalker1208

@gspencergoog Can you post the snippet of code that created the ListTiles with that background?

@gspencergoog
Copy link
Contributor Author

@NWalker1208 Unfortunately, I can't share the code, partly because it's hugely complicated, but also because the customer's code is proprietary (Google's). I'm trying to build a test case, but so far I am failing to get it to reproduce.

@NWalker1208
Copy link
Contributor

@gspencergoog I assume that the grey background on the first image is intentional. Otherwise, I feel like the second image actually looks better. Could you explain how their code tries to give the ListTile a grey background? For example, do they set the color property of the tile, or does a descendant of the ListTile paint the color?

Also, thanks for your patience. I didn’t think this contribution would be so troublesome, but maybe that's why nobody has fixed this issue before. Hopefully we can figure this out.

@gspencergoog
Copy link
Contributor Author

Yes, the grey background is set on the tileColor attribute as Colors.grey[50], and is intentional.

In looking at the widget tree, it looks like this may have to do with interactions with scrollable areas. If I replace the ListView with a Column in my example (linked below), the problem goes away. That might just be because it introduces different widgets in the tree, however.

In a diff of the two widget trees, this line (all from debugDumpApp()) in the "before your change" diff:

└ColoredBox(color: Color(0xfffafafa), renderObject: _RenderColoredBox#00000 relayoutBoundary=up35)

When run with your change, changes to:

└Ink(bg: ShapeDecoration(color: Color(0xfffafafa), shape: Border.all(BorderSide(Color(0xff000000), 0.0, BorderStyle.none))), state: _InkState#00000)
 └Padding-[GlobalKey#00000](padding: EdgeInsets.zero, dependencies: [Directionality], renderObject: RenderPadding#00000 relayoutBoundary=up35)
  └Builder(dependencies: [_LocalizationsScope-[GlobalKey#00000], Directionality, MediaQuery])

Which I think is mostly expected.

I think the problem has to do with how golden tests capture things, and in order to find the golden image, it needs a widget to start with, and if that widget contains a repaint boundary, it uses that repaint boundary, and if it doesn't, it uses an ancestor with a repaint boundary. Since you added in an Ink, it mutates which repaint boundary the test uses to capture the image. This might be a simple case of changing the test to match new expectations (making it search for a different widget to snapshot, for instance), but it also might point to some other issue.

Here's a ZIP file with a sample app that shows the problem (you'll need to run "flutter create ." in there before it'll build). Visually, the app when I run it is fine, but the flutter test fails with your code (doesn't draw the background into the golden), and succeeds without it. I included the golden image, the failed image (in test/failures) and some widget trees you can diff.

@NWalker1208
Copy link
Contributor
NWalker1208 commented Feb 20, 2021 •
edited
Loading

@gspencergoog So I believe this issue with the test is being caused because the background created by the Ink widget isn't actually painted by the Ink widget, but by it's nearest Material widget ancestor. The golden test only captures what's rendered on the nearest RepaintBoundary. When the tile is in a ListView, the Material widget has a few RepaintBoundaries between it and the ListTile. Before my change, the ListTile painted its own background, so it was captured by the test. Since my change causes the Material widget to render the background, the golden test fails because it doesn't see past the nearest RepaintBoundary.

One way to fix this would be to replace the Ink widget with a Material widget, so that it renders all InkFeatures by itself. This would satisfy the test, but it would be less efficient, especially in situations where there are many ListTiles. The alternative, like you mentioned, is to try to change the test, but I think that would have to happen on your end. What would you recommend?

@gspencergoog
Copy link
Contributor Author

@NWalker1208 I think you have it figured out. I don't think replacing the Ink with a Material is the right way to go: it's just going to be inefficient.

I think I'll try and change the test, since it's likely that the actual app is rendering properly, it's just being captured differently now. Obviously, I'd have to verify that first.

I'll see if can create a new PR (with your changes) and try to land it simultaneously with changes to the internal test (we have a mechanism for that). (It may be a few days, I've been busy...)

@NWalker1208
Copy link
Contributor

@gspencergoog Sounds like a plan. Let me know if you run into any new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gspencergoog @NWalker1208 @goderbauer
0