Expose clipBehavior to more Material Buttons #20538
Conversation
|
Can't the ink well do its own clipping? We don't want to be clipping when there's no splash, right? |
|
@Hixie yes, now I used the InkWell's own borderRadius to do the clip. It seems that @lukef added the borderRadius clip about a year ago but half a year later @HansMuller made buttons to use Material's clip instead (so the borderRadius clip was never called after then)... We didn't find clip issues in the rounded rect button after setting Material's clip to none because the default rrect radii are very small and the splash/highlight color is close to transparent. If we have a magnifier, we would see the bleeding highlights and splash there too. This change should fix them all. |
|
I don't think this is the right fix. RawMaterialButton splashes should still be unconditionally clipped to their shape (to the shape of their Material). The default clip should not be Clip.none. |
|
Rather than try and forward fix this, lets roll back the original clipping PR (so I can roll g3) and spend the time on the right fix |
|
Reverting the whole change might be difficult due to potential conflicts.
Let's revert the FloatingActionButton's clipBehavior to Clip.antiAlias
first to solve the issue and unblock the roll.
…On Tue, Aug 14, 2018 at 9:19 AM Jonah Williams ***@***.***> wrote:
Rather than try and forward fix this, lets roll back the original clipping
PR (so I can roll g3) and spend the time on the right fix
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#20538 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AV7DMGDOArN6BUSf0fbrMiDfV5CH1Atnks5uQviNgaJpZM4V7H_X>
.
|
|
@jonahwilliams Since a full revert might have many conflicts, I'll merge this once all tests are passed. (Feel free to merge it if I forgot.) This should unblock the roll. I'll discuss with Hans and Ian later (unfortunately not today due to WFH) to see what's the right fix. |
|
Let's hold off on this one until we land the revert and have time to fully understand the clipping implications. |
|
Yeah the clip should be to the shape of the material, but it shouldn't be done by material, it should be done by the ink itself. We'll have to pass the shape to the inkwell. |
|
...and indeed it might be cheaper for the splash to use the path ops to find the intersection of the splash and the shape than to actually clip anything. |
|
@jonahwilliams @Hixie would you please review this simple PR again that exposes clipBehavior to more material buttons? Now we don't have to worry about InkWell as our other PRs have handled it. |
|
What about MaterialButton, RaisedButton, and OutlineButton? I think it would be a good idea to take care of all of these at once |
|
@HansMuller the clipBehavior is already defined in those buttons. |
|
LGTM. We should probably add some tests that verify that we're not adding clips back. |
This allows developers to control the clipBehavior of those buttons.