Removed ClipRect from AnimatedCrossFade#152027
Conversation
chunhtai
left a comment
There was a problem hiding this comment.
This pr also needs a test
| curve: widget.sizeCurve, | ||
| child: widget.layoutBuilder(topChild, topKey, bottomChild, bottomKey), | ||
| ), | ||
| return AnimatedSize( |
There was a problem hiding this comment.
This seems dangerous. If the content is indeed bigger, this may cause a regression
There was a problem hiding this comment.
I think this ClipRect here is undesired behavior but i would understand concerns about changing default behaviour. Making this optional while leaving default behavior as is may be a better idea. Let me prepare a commit for it.
There was a problem hiding this comment.
It might be better to expose clipBehavior as a property so folks can disable/enable/configure as they like.
There was a problem hiding this comment.
ClipRect is 100% the wrong behavior IMO since the defaultLayoutBuilder purposely sets clipBehavior: Clip.none one would assume that overriding the builder would solve the clipping issue (in the case where people do not want to clip) but nope!
There was a problem hiding this comment.
This seems dangerous. If the content is indeed bigger, this may cause a regression
The content should be contained inside of the stack which itself has a clipping behavior
|
(triage) @kaaninel Do you still have plans to come back to this one and address the feedback given above? |
I just returned from my holiday. I can continue on this topic now. |
|
Hey @kaaninel do you have plans to address the feedback above? Thanks! |
|
Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to re-open or submit a new PR if you have the time to address the review comments. Thanks! |
`AnimatedCrossFade` wraps its content in a hardcoded `ClipRect` with no way to change or disable the clipping behavior. This causes widgets with shadows, overflowing content (e.g. `CircularProgressIndicator` stroke), or other visual effects to be unintentionally clipped. This PR adds a `clipBehavior` parameter to `AnimatedCrossFade` (defaults to `Clip.hardEdge` for backwards compatibility) and forwards it to the internal `ClipRect` in `_AnimatedCrossFadeState.build()`. This follows the same pattern used by `Stack`, `AnimatedSize`, `ListView`, and other Flutter widgets that expose `clipBehavior`. PR flutter#152027 attempted to fix this by removing `ClipRect` entirely, but was closed after reviewers recommended exposing `clipBehavior` as a configurable parameter instead. Fixes flutter#152025 Fixes flutter#25950 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Victor Sanni <victorsanniay@gmail.com>
This PR is a possible solution for #152025. Current implementation of AnimatedCrossFade includes ClipRect without any option to configure it. Removing this widget from build function seems to not cause any issues with my test cases. In case clipping behavior is needed developer can clip the child widget before passing it to AnimatedCrossFade.
As you can see shadow is clipped and causing a weird look.
Removing ClipRect results with expected/desired behavior.
#152025