fix: prevent accordion from clipping focus rings#583
fix: prevent accordion from clipping focus rings#583niklasbartsch wants to merge 2 commits intonank1ro:mainfrom
Conversation
Add clipBehavior parameter to SizeEffect and ShadAccordionItem to control clipping behavior during size transition animations. - Add clipBehavior parameter to SizeEffect class with custom _ShadSizeTransition - Add clipBehavior parameter to ShadAccordionItem and ShadAccordionTheme - Default to Clip.none in accordion to prevent focus ring clipping Fixes nank1ro#582
WalkthroughAdds a nullable Changes
Sequence Diagram(s)(omitted — change is primarily a focused bug fix to clipping and does not introduce a multi-component control-flow that benefits from a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nank1ro
left a comment
There was a problem hiding this comment.
Update the pubspec and the CHANGELOG to be ready to deploy it, thanks for the PR 🙏
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/src/utils/effects.dart (1)
46-84: Consider adding axis and alignment parameters for flexibility.The current implementation hardcodes vertical sizing (
heightFactor) and top-start alignment. Flutter'sSizeTransitionsupports configurableaxisandaxisAlignment. While the current implementation works well for the accordion use case, adding these parameters would makeShadSizeTransitionmore reusable for other scenarios.✨ Optional enhancement for axis/alignment support
Consider extending the implementation to support both horizontal and vertical sizing:
class ShadSizeTransition extends AnimatedWidget { const ShadSizeTransition({ super.key, required Animation<double> sizeFactor, this.clipBehavior = Clip.hardEdge, + this.axis = Axis.vertical, + this.axisAlignment = 0.0, this.child, }) : super(listenable: sizeFactor); final Clip clipBehavior; + final Axis axis; + final double axisAlignment; final Widget? child; Animation<double> get sizeFactor => listenable as Animation<double>; @override Widget build(BuildContext context) { + final alignment = axis == Axis.vertical + ? AlignmentDirectional(0.0, axisAlignment) + : AlignmentDirectional(axisAlignment, 0.0); + final result = Align( - alignment: AlignmentDirectional.topStart, + alignment: alignment, - heightFactor: sizeFactor.value, + heightFactor: axis == Axis.vertical ? sizeFactor.value : null, + widthFactor: axis == Axis.horizontal ? sizeFactor.value : null, child: child, ); if (clipBehavior == Clip.none) { return result; } return ClipRect( clipBehavior: clipBehavior, child: result, ); } }example/lib/pages/accordion.dart (1)
23-26: LGTM! Effective test case for the focus ring fix.The test implementation correctly demonstrates the issue and the fix. The toggle allows users to compare the old buggy behavior (clipping enabled) with the new correct behavior (clipping disabled).
The label "Clip content (old bug)" could be slightly clearer. Consider something like "Enable clipping (reproduces #582)" to make it more obvious that turning it ON reproduces the bug, while OFF shows the fix.
Also applies to: 39-39, 49-60, 82-86
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdexample/lib/pages/accordion.dartlib/src/utils/effects.dartpubspec.yaml
✅ Files skipped from review due to trivial changes (1)
- pubspec.yaml
🔇 Additional comments (3)
CHANGELOG.md (1)
1-5: LGTM! Clear and accurate changelog entry.The changelog properly documents both the bug fix and the new public API surface. The entries are well-formatted and provide sufficient context for users.
lib/src/utils/effects.dart (2)
14-14: LGTM! Well-documented clipBehavior parameter.The
clipBehaviorparameter with defaultClip.hardEdgemaintains backward compatibility while enabling the fix. The documentation clearly explains the purpose and usage.Also applies to: 20-26
35-39: LGTM! Proper integration with the new ShadSizeTransition.The refactoring to use
ShadSizeTransitionwith theclipBehaviorparameter correctly implements the solution for preventing focus ring clipping.
bug.mov@niklasbartsch When clip is none, the item of the accordion animates above the title, and it is not a good thing. |
|
@niklasbartsch did you have time to check my last comment? |


Summary
clipBehaviorparameter toSizeEffectclass with custom_ShadSizeTransitionwidgetclipBehaviorparameter toShadAccordionItemandShadAccordionThemeClip.nonein accordion to prevent focus ring clippingRoot Cause
The
ShadAccordionItemusesSizeTransitionfor the expand/collapse animation.SizeTransitioninternally usesClipRectwhich clips any content that overflows the bounds - including focus rings that extend slightly beyond the widget's boundary.Solution
Created a custom
_ShadSizeTransitionwidget that supports configurableclipBehavior. When set toClip.none, it skips theClipRectentirely, allowing focus rings and other content to extend beyond the widget's boundary.Test plan
ShadInputFormFieldinside aShadAccordionItemFixes #582
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.