[InkWell] fix: Prevent disabled buttons from passing taps to parent GestureDetectors#187551
[InkWell] fix: Prevent disabled buttons from passing taps to parent GestureDetectors#187551lucas-goldner wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request wraps the button layout in a GestureDetector to absorb taps when the button is disabled, preventing them from propagating to parent gesture detectors, and adds a regression test for this behavior. Feedback suggests using HitTestBehavior.deferToChild instead of HitTestBehavior.opaque when the button is enabled to prevent the GestureDetector from unexpectedly intercepting hit tests in transparent areas of the button.
| result = GestureDetector( | ||
| onTap: widget.enabled ? null : () {}, | ||
| behavior: HitTestBehavior.opaque, | ||
| excludeFromSemantics: true, | ||
| child: result, | ||
| ); |
There was a problem hiding this comment.
When the button is enabled, onTap is null. Hardcoding behavior: HitTestBehavior.opaque means that even when the button is enabled, this outer GestureDetector will force the hit-test behavior to be opaque. This could potentially intercept hit tests in transparent areas of the button or its padding, affecting parent or sibling widgets in unexpected ways.
Using HitTestBehavior.deferToChild when the button is enabled ensures that the GestureDetector remains completely passive and does not alter the hit-testing behavior of the enabled button.
result = GestureDetector(
onTap: widget.enabled ? null : () {},
behavior: widget.enabled ? HitTestBehavior.deferToChild : HitTestBehavior.opaque,
excludeFromSemantics: true,
child: result,
);There was a problem hiding this comment.
Changed it to deferToChild! 👍🏽
|
@ reviewer looking at this PR: Hi! I noticed that the Material library in flutter/flutter has been frozen since April 7th as part of the decoupling effort. Since this PR modifies button_style_button.dart in the Material library, should I port this to the |
| // When the button is disabled, absorb taps to prevent them from | ||
| // propagating to parent gesture detectors. | ||
| // See https://github.com/flutter/flutter/issues/138981 | ||
| result = GestureDetector( |
There was a problem hiding this comment.
Why not wrap with an AbsorbPointer instead?
| WidgetTester tester, | ||
| ) async { | ||
| final theme = ThemeData(); | ||
| testWidgets( |
There was a problem hiding this comment.
Can these formatting changes be undone?
Description
Fixes #138981
When a disabled
ButtonStyleButton(e.g.ElevatedButton,TextButton,OutlinedButton,FilledButton) withonPressed: nullis placed inside a parentGestureDetector, tapping the disabled button incorrectly triggers the parent'sonTapcallback.Before:
before.mov
After:
after.mov
Root Cause
The
InkWellinsideButtonStyleButtonuses aGestureDetectorwithHitTestBehavior.opaque, but when the button is disabled, all gesture callbacks are set tonull. This means:TapGestureRecognizeris registered in the gesture arenaGestureDetector's recognizer wins the arena unopposedonTapfires — even though a disabled button was tappedApproaches Considered
AbsorbPointer: Tried wrapping theInkWellwithAbsorbPointer(absorbing: !widget.enabled), but this doesn't work.AbsorbPointer.hitTestreturnstruewithout adding itself to the hit test result, which still causes the parentGestureDetectorto add itself viahitTestChildrenand fire its callback.Conditional
GestureDetectorwrapper (if (!widget.enabled)): This worked for tap absorption but broke thedisabled and hovered ElevatedButton responds to mouse-exittest. Conditionally inserting/removing a widget changes the tree structure, causing theMouseRegioninsideInkWellto lose its hover state.Always-present
GestureDetectorwrapper (chosen approach): Wraps theInkWellin aGestureDetectorthat is always present but only registers a no-oponTapwhen the button is disabled. When enabled,onTapisnullso no extra recognizer is registered. This avoids tree structure changes and correctly absorbs taps on disabled buttons by competing in and winning the gesture arena.Fix
Added a
GestureDetectorwrapper inButtonStyleButton._build()that registers a no-oponTap: () {}when the button is disabled. ThisTapGestureRecognizercompetes in the gesture arena, wins (being closer to the tap point than the parent), and discards the event.Pre-launch Checklist
///).