Add clipBehavior to widgets with clipRect #55977

liyuqian · 2020-04-29T18:41:01Z

Description

We add clipBehavior to

Flex
FittedBox
UnconstrainedBox
Wrap
Stack
EditableText
ListWheelScrollView
SingleChildScrollView
NestedScrollView
ShrinkWrappingViewport

Eventually, the first 4 of them will be default to Clip.none while the remaining 6 of them are default to Clip.hardEdge.

In the current PR, all of them are still default to Clip.hardEdge to not break some Google usages. Once we've updated the code inside Google, we'll update the clipBehavior.

Related Issues

This is a continuation of #18057, and this fixes #21830

Tests

Unit tests added to each modified widget and render object.

Breaking Change

Yes, this is a breaking change as all those widgets previously default to clip with anti-alias.
- I wrote a design doc: https://flutter.dev/go/clip-behavior
- I got input from the developer relations team, specifically from filiph
- I wrote a migration guide

skia-gold · 2020-04-29T18:49:25Z

Gold has detected one or more untriaged digests on patchset 9.
Please triage them at https://flutter-gold.skia.org/search?issue=55977.

fluttergithubbot · 2020-04-29T19:32:45Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

Changes to golden files are considered breaking changes, so consult Handling Breaking Changes to proceed. While there are exceptions to this rule, if this patch modifies an existing golden file, it is probably not an exception. Only new golden file tests, or downstream changes like those from skia updates are considered non-breaking.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #55977 at sha 9b1569df6c88349372787e7df5597b8d83582942

goderbauer · 2020-04-29T20:08:25Z

This appears to be breaking some tests (see cirrus).

liyuqian · 2020-04-29T21:23:10Z

@goderbauer : thanks! Just fixed them.

skia-gold · 2020-04-29T21:31:25Z

Gold has detected one or more untriaged digests on patchset 10.
Please triage them at https://flutter-gold.skia.org/search?issue=55977.

fluttergithubbot · 2020-04-29T21:57:39Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55977 at sha dc4ed1291ab026257428c0d631709e60802b3f84

fluttergithubbot · 2020-04-29T22:42:35Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55977 at sha c2e2ad91876e292dfc598b71e8226569b34a2bfe

goderbauer · 2020-04-30T21:57:49Z

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

Wondering whether we really need this class. The renderobjects in question could just use the mixin directly?

I added ClippableRenderBox for SingleChildScrollView respects clipBehavior test: https://github.com/flutter/flutter/pull/55977/files#diff-dc4070bb6bd99e8491da558051581285R45. Its render object _RenderSingleChildViewport is private so I have to create a parent render object class for testing... I'd love to remove ClippableRenderBox if there's another way to test it.

Could you re-write the test as follows:

final dynamic renderObject = tester.renderObject.where((RenderObject o) => o.runtimeType.toString() == '_RenderSingleChildViewport'); expect(renderObject.clipBehavior, equals(Clip.hardEdge));

Note the cast to dynamic which should allow you to call whatever you want on it.

we definitely shouldn't add to our public API surface just to work around Dart testability limitations. Better to just go fix Dart.

goderbauer · 2020-04-30T21:59:29Z

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

It's strange that there are two flags here to control whether to clip or not. clipToSize and clipBehavior.

I'd love to remove clipToSize and overflow! My previous concern was how it affects our breaking change migration. My old post https://groups.google.com/g/flutter-dev/c/XMHE0XdsXxI/m/nonrNQpbBAAJ seems to still cover this PR if I don't remove clipToSize and overflow. But after a second thought, I think it's better to remove them immediately so the old code would result in compile errors instead of runtime errors saying that clipToSize/overflow is incompatible with the default clipBehavior. My next patch will remove them, and I'll write a new breaking change announcement with migration suggestions.

goderbauer · 2020-04-30T22:01:39Z

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

Same here, it's strange that there are two flags to control clipping.

Will remove.

goderbauer · 2020-04-30T22:02:13Z

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

ClippableRenderBox claims that the default is none.

Do you mean that the auto-generated API doc for RenderViewportBase will say that the default is Clip.none instead of Clip.hardEdge because ClippableRenderBox defaults it to Clip.none? Some ClippableRenderBox subclasses default to Clip.none while some default to Clip.hardEdge. I'd like to know if there's a way to specify/modify in each subclass.

I don't think dartdoc provides any affordances for this, but I am no dartdocs expert.

But the auto-generated documentation for all classes that extend ClippableRenderBox or mixin the mixin and set the default to something other then "none" will have incorrect documentation with the current approach.

I wouldn't bother with the mixin. Just inline the duplicate code. It's not complex logic, and it might not be identical everywhere (as shown by the different defaults, for one).

goderbauer · 2020-04-30T22:33:45Z

packages/flutter/lib/src/widgets/editable_text.dart

in the constructor its set to hardEdge

(This comment applies to some other places in the PR as well)

Good catch! Forget to change after copy/paste... Fixed for all 5 widgets now.

goderbauer · 2020-04-30T22:35:42Z

Can you please fill out the PR template. The section about breaking changes would be most relevant.

What's the plan to make this breaking change? Will this not break many customers in unsuspecting ways?

liyuqian

Filled out the PR template with the breaking change section. I'll come up with a design doc with the new template, contact the DevRel team, and send out an updated migration guide (the old one is https://groups.google.com/g/flutter-dev/c/XMHE0XdsXxI/m/nonrNQpbBAAJ) to flutter-dev or flutter-announcement.

liyuqian · 2020-05-01T22:09:31Z

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

I added ClippableRenderBox for SingleChildScrollView respects clipBehavior test: https://github.com/flutter/flutter/pull/55977/files#diff-dc4070bb6bd99e8491da558051581285R45. Its render object _RenderSingleChildViewport is private so I have to create a parent render object class for testing... I'd love to remove ClippableRenderBox if there's another way to test it.

liyuqian · 2020-05-01T22:28:37Z

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

I'd love to remove clipToSize and overflow! My previous concern was how it affects our breaking change migration. My old post https://groups.google.com/g/flutter-dev/c/XMHE0XdsXxI/m/nonrNQpbBAAJ seems to still cover this PR if I don't remove clipToSize and overflow. But after a second thought, I think it's better to remove them immediately so the old code would result in compile errors instead of runtime errors saying that clipToSize/overflow is incompatible with the default clipBehavior. My next patch will remove them, and I'll write a new breaking change announcement with migration suggestions.

liyuqian · 2020-05-01T22:28:49Z

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

Will remove.

liyuqian · 2020-05-01T22:32:24Z

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

Do you mean that the auto-generated API doc for RenderViewportBase will say that the default is Clip.none instead of Clip.hardEdge because ClippableRenderBox defaults it to Clip.none? Some ClippableRenderBox subclasses default to Clip.none while some default to Clip.hardEdge. I'd like to know if there's a way to specify/modify in each subclass.

liyuqian · 2020-05-01T22:33:24Z

packages/flutter/lib/src/widgets/editable_text.dart

Good catch! Forget to change after copy/paste... Fixed for all 5 widgets now.

skia-gold · 2020-05-01T23:33:25Z

Gold has detected one or more untriaged digests on patchset 12.
Please triage them at https://flutter-gold.skia.org/search?issue=55977.

skia-gold · 2020-05-01T23:35:25Z

Gold has detected one or more untriaged digests on patchset 12.
Please triage them at https://flutter-gold.skia.org/search?issue=55977.

fluttergithubbot · 2020-05-02T00:03:20Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55977 at sha 89c34d5e5e0838c47963bcdf9989d231326e327a

skia-gold · 2020-05-05T21:37:25Z

Gold has detected one or more untriaged digests on patchset 13.
Please triage them at https://flutter-gold.skia.org/search?issue=55977.

fluttergithubbot · 2020-05-05T22:07:21Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55977 at sha 8367ad27807951a25080410411a91bf8c124367f

See also flutter/flutter#55977

goderbauer · 2020-05-07T20:10:02Z

Does this change cause any trouble when rolled into google3?

Hixie · 2020-05-07T20:24:16Z

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

this doesn't belong in object.dart. It's way too specific.

Is there any reason we wouldn't just duplicate this code everywhere rather than using mixins? Mixins aren't free (they add more classes to our class hierarchy, etc). I don't know how well they interact with tree shaking either. We don't have any trouble duplicating other fields. Why is this one special?

That will also fix the documentation trouble I mentioned in the other comment.

Removed the Mixin. BTW, I'm curious if we had any experiments on the Mixin's impact on app size? For example, what's the Flutter gallery's size difference between (1) copying a small piece of code dozens of times and (2) defining a small mixin which is reused dozens of times?

Those widgets are NestedScrollView and ShrinkWrappingViewport.

This reverts commit cd593da.

)" (flutter#58344)" This reverts commit 1d395c5.

#59364) * Revert "Revert "Add clipBehavior to widgets with clipRect (#55977)" (#58344)" This reverts commit 1d395c5. * Add missed Overflow

…r#55977" (flutter#59364) * Revert "Revert "Add clipBehavior to widgets with clipRect (flutter#55977)" (flutter#58344)" This reverts commit 1d395c5. * Add missed Overflow

urusaich · 2020-08-11T15:07:24Z

Good feature.
But what about ScrollView and its children (ListView)?

liyuqian · 2020-08-11T17:27:44Z

@urusai88 : they're being added in #63147

…r#55977" (flutter#59364) * Revert "Revert "Add clipBehavior to widgets with clipRect (flutter#55977)" (flutter#58344)" This reverts commit 1d395c5. * Add missed Overflow

liyuqian added the c: API break Backwards-incompatible API changes label Apr 29, 2020

liyuqian requested review from Hixie and goderbauer April 29, 2020 18:41

googlebot added the cla: yes label Apr 29, 2020

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 29, 2020

liyuqian force-pushed the clip branch 2 times, most recently from 6223a26 to 9b1569d Compare April 29, 2020 19:01

fluttergithubbot added the will affect goldens Changes to golden files label Apr 29, 2020

liyuqian force-pushed the clip branch from dc4ed12 to c2e2ad9 Compare April 29, 2020 22:12

goderbauer reviewed Apr 30, 2020

View reviewed changes

liyuqian commented May 1, 2020

View reviewed changes

liyuqian force-pushed the clip branch from 89c34d5 to 8367ad2 Compare May 5, 2020 21:28

liyuqian added a commit to liyuqian/website that referenced this pull request May 7, 2020

Add clip behavior design doc for breaking change

ce87215

See also flutter/flutter#55977

liyuqian mentioned this pull request May 7, 2020

Add clip behavior design doc for breaking change flutter/website#4057

Merged

sfshaza2 pushed a commit to flutter/website that referenced this pull request May 7, 2020

Add clip behavior design doc for breaking change (#4057)

fbb0617

See also flutter/flutter#55977

Hixie reviewed May 7, 2020

View reviewed changes

liyuqian added 14 commits May 28, 2020 15:57

Add clipBehavior to TextEditable

bc86b3b

Add clipBehavior to ListWheelScrollView

5d5ea43

Add clipBehavior to SingleChildScrollView

2e4b25c

Add clipBehavior to RenderViewportBase's widgets

0a11a0e

Those widgets are NestedScrollView and ShrinkWrappingViewport.

Fix tests

f6f62d3

Remove enum Overflow and fix typo

802de50

Remove clipToSize

438a497

Analyze fix

022af99

Remove Mixin and other small fixes

af6112a

Fix tests and respect Stack's default clipBehavior

32fb17f

Add Overflow back to make it non-breaking

deaff1f

Restore clipBehavior to make it non-breaking

991779c

Small fixes

4c7a512

Fix rebase

8bf12f9

liyuqian force-pushed the clip branch from 2de6aa0 to 8bf12f9 Compare May 28, 2020 23:07

liyuqian merged commit cd593da into flutter:master May 29, 2020

mehmetf added a commit that referenced this pull request May 31, 2020

Revert "Add clipBehavior to widgets with clipRect (#55977)"

8e56960

This reverts commit cd593da.

mehmetf mentioned this pull request May 31, 2020

Revert "Add clipBehavior to widgets with clipRect" #58344

Merged

mehmetf added a commit that referenced this pull request May 31, 2020

Revert "Add clipBehavior to widgets with clipRect (#55977)" (#58344)

1d395c5

This reverts commit cd593da.

liyuqian mentioned this pull request Jun 12, 2020

Reland non-breaking "Add clipBehavior to widgets with clipRect #55977" #59364

Merged

4 tasks

liyuqian added a commit to liyuqian/flutter that referenced this pull request Jun 17, 2020

Revert "Revert "Add clipBehavior to widgets with clipRect (flutter#55977

5b25a08

)" (flutter#58344)" This reverts commit 1d395c5.

liyuqian added a commit that referenced this pull request Jun 17, 2020

Reland non-breaking "Add clipBehavior to widgets with clipRect #55977" (

42e02d6

#59364) * Revert "Revert "Add clipBehavior to widgets with clipRect (#55977)" (#58344)" This reverts commit 1d395c5. * Add missed Overflow

xu-baolin mentioned this pull request Oct 29, 2020

UnconstrainedBox overflow indicator should not shown when overflowing but [clipBehavior] is not [Clip.none] #69299

Closed

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

Add clipBehavior to widgets with clipRect #55977

Add clipBehavior to widgets with clipRect #55977

Uh oh!

Conversation

liyuqian commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking Change

Uh oh!

skia-gold commented Apr 29, 2020

Uh oh!

fluttergithubbot commented Apr 29, 2020

Uh oh!

goderbauer commented Apr 29, 2020

Uh oh!

liyuqian commented Apr 29, 2020

Uh oh!

skia-gold commented Apr 29, 2020

Uh oh!

fluttergithubbot commented Apr 29, 2020

Uh oh!

fluttergithubbot commented Apr 29, 2020

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!

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!

goderbauer commented Apr 30, 2020

Uh oh!

liyuqian 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!

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!

skia-gold commented May 1, 2020

Uh oh!

skia-gold commented May 1, 2020

Uh oh!

fluttergithubbot commented May 2, 2020

Uh oh!

liyuqian commented Apr 29, 2020 •

edited

Loading

urusaich commented Aug 11, 2020 •

edited

Loading