-
Notifications
You must be signed in to change notification settings - Fork 368
button: Replace GestureDetector with Listener for AnimatedScaleOnTap
#1954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
7710b5d to
7034690
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below.
test/widgets/button_test.dart
Outdated
| check(scale).equals(expectedScale); | ||
| } | ||
|
|
||
| testWidgets('Animation happen instantly when tap down', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this test without your change in lib/widgets/button.dart, it still passes. That means it's not doing its job.
| return Listener( | ||
| behavior: HitTestBehavior.translucent, | ||
| onTapDown: (_) => _changeScale(widget.scaleEnd), | ||
| onTapUp: (_) => _changeScale(1), | ||
| onTapCancel: () => _changeScale(1), | ||
| onPointerDown: (_) => _changeScale(widget.scaleEnd), | ||
| onPointerUp: (_) => _changeScale(1), | ||
| onPointerCancel: (_) => _changeScale(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the docs for GestureDetector.onTapDown and friends, it looks like one of the helpful features of that higher-level API is that it only considers pointer events from a "primary button". I don't know how common it is e.g. to use a stylus with multiple buttons, but this seems like functionality that would be helpful to keep.
The event passed to the Listener.onPointerDown callback (etc.) has a buttons field. So in each of the handlers, let's check whether the event's buttons field equals kPrimaryButton, and only respond if it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the name and dartdoc of this AnimatedScaleOnTap should be updated to match the new behavior: it's no longer about a tap gesture, which is only recognized after 100ms. For example:
/// Apply [Transform.scale] to the child widget on primary pointer-down,
/// and reset its scale on -up or -cancel, with animated transitions.
class AnimatedScaleOnPointerDown extends StatefulWidget {|
Thank you @chrisbobbe for the Review, added a timeout of 50 millisecond to pumpAndSettle to make sure in the test that animation happens almost instantly. Checked it with the previous code of button.dart that it failed, the updated code in button.dart as proposed in this PR does pass those test. PTAL. |
7034690 to
d79b712
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR eliminates the 100ms delay in scale animations by replacing GestureDetector with Listener in button touch feedback. The widget has been renamed from AnimatedScaleOnTap to AnimatedScaleOnPrimaryPointerDown to better reflect its implementation, which now responds immediately to pointer events rather than waiting for gesture recognition.
Key changes:
- Replaced
GestureDetectorwith lower-levelListenerAPI for immediate touch response - Renamed widget to
AnimatedScaleOnPrimaryPointerDownto reflect the implementation change - Added primary button detection logic to only respond to primary pointer button presses
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/widgets/button.dart | Implemented Listener-based touch handling with primary button detection, replacing gesture-based approach |
| lib/widgets/home.dart | Updated all widget references to use the renamed AnimatedScaleOnPrimaryPointerDown |
| test/widgets/button_test.dart | Added test verifying instant animation response on pointer down/up |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/widgets/button.dart
Outdated
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | ||
| }, | ||
| onPointerUp: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _pressedButton state should be reset to 0 in onPointerUp as well, not just in onPointerCancel. Without this, if the user presses the primary button and releases it, then presses a secondary button, the widget may incorrectly maintain the previous button state. Add _pressedButton = 0; after the scale change.
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| _pressedButton = 0; |
test/widgets/button_test.dart
Outdated
| check(renderObject).size.equals(Size.square(40)); | ||
| }); | ||
|
|
||
| group('AnimatedScaleOnTap', () { |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test group name should be updated to match the renamed widget. Change 'AnimatedScaleOnTap' to 'AnimatedScaleOnPrimaryPointerDown'.
| group('AnimatedScaleOnTap', () { | |
| group('AnimatedScaleOnPrimaryPointerDown', () { |
lib/widgets/button.dart
Outdated
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | ||
| }, | ||
| onPointerUp: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | ||
| }, | ||
| onPointerCancel: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect operator precedence. The != operator has higher precedence than &, so this expression is evaluated as _pressedButton & (kPrimaryButton != 0) instead of (_pressedButton & kPrimaryButton) != 0. Add parentheses around the bitwise AND operation: if ((_pressedButton & kPrimaryButton) != 0).
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | |
| }, | |
| onPointerUp: (_) { | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| }, | |
| onPointerCancel: (_) { | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| if ((_pressedButton & kPrimaryButton) != 0) _changeScale(widget.scaleEnd); | |
| }, | |
| onPointerUp: (_) { | |
| if ((_pressedButton & kPrimaryButton) != 0) _changeScale(1); | |
| }, | |
| onPointerCancel: (_) { | |
| if ((_pressedButton & kPrimaryButton) != 0) _changeScale(1); |
lib/widgets/button.dart
Outdated
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | ||
| }, | ||
| onPointerUp: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | ||
| }, | ||
| onPointerCancel: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect operator precedence. The != operator has higher precedence than &, so this expression is evaluated as _pressedButton & (kPrimaryButton != 0) instead of (_pressedButton & kPrimaryButton) != 0. Add parentheses around the bitwise AND operation: if ((_pressedButton & kPrimaryButton) != 0).
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | |
| }, | |
| onPointerUp: (_) { | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| }, | |
| onPointerCancel: (_) { | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| if((_pressedButton & kPrimaryButton) != 0) _changeScale(widget.scaleEnd); | |
| }, | |
| onPointerUp: (_) { | |
| if((_pressedButton & kPrimaryButton) != 0) _changeScale(1); | |
| }, | |
| onPointerCancel: (_) { | |
| if((_pressedButton & kPrimaryButton) != 0) _changeScale(1); |
lib/widgets/button.dart
Outdated
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | ||
| }, | ||
| onPointerUp: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | ||
| }, | ||
| onPointerCancel: (_) { | ||
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect operator precedence. The != operator has higher precedence than &, so this expression is evaluated as _pressedButton & (kPrimaryButton != 0) instead of (_pressedButton & kPrimaryButton) != 0. Add parentheses around the bitwise AND operation: if ((_pressedButton & kPrimaryButton) != 0).
| if(_pressedButton & kPrimaryButton != 0) _changeScale(widget.scaleEnd); | |
| }, | |
| onPointerUp: (_) { | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| }, | |
| onPointerCancel: (_) { | |
| if(_pressedButton & kPrimaryButton != 0) _changeScale(1); | |
| if((_pressedButton & kPrimaryButton) != 0) _changeScale(widget.scaleEnd); | |
| }, | |
| onPointerUp: (_) { | |
| if((_pressedButton & kPrimaryButton) != 0) _changeScale(1); | |
| }, | |
| onPointerCancel: (_) { | |
| if((_pressedButton & kPrimaryButton) != 0) _changeScale(1); |
d79b712 to
365824a
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below, from a human review. 🙂
I haven't been expecting Copilot AI to come along and give its feedback—I plan to discuss with Greg when he's back from his vacation. 🙂
lib/widgets/button.dart
Outdated
| onTapUp: (_) => _changeScale(1), | ||
| onTapCancel: () => _changeScale(1), | ||
| onPointerDown: (PointerDownEvent pointerDownEvent) { | ||
| _pressedButton = pointerDownEvent.buttons; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why store .buttons in a stateful variable? The events passed to onPointerUp and onPointerCancel have a .buttons field too, right; is there a reason not to instead just act on that data in the event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PointerDownEvent parameter is not accessible in other friends of onPointerDown function, since the type of the parameter and respective function differs. From its dartdoc:
/// Called when a pointer comes into contact with the screen (for touch
/// pointers), or has its button pressed (for mouse pointers) at this widget's
/// location.
final PointerDownEventListener? onPointerDown;
...
/// Called when a pointer that triggered an [onPointerDown] is no longer in
/// contact with the screen.
final PointerUpEventListener? onPointerUp;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, onPointerUp and onPointerCancel don't receive a PointerDownEvent. They do receive a PointerUpEvent and a PointerCancelEvent, respectively, and both of those events have a .buttons field (inherited from the base class PointerEvent, just like PointerDownEvent.buttons).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buttons is a bitmask of currently pressed buttons at the time of the event, not which button triggered the event.
For eg. pointerEventUp.buttons represent the bit set of buttons currently pressed during the pointerUp Event not the button that was just released. Hence we don't require them in the first place, other than pointerDownEvent since we want to make sure the pointerDownEvent is initiated by kPrimaryButton.
For my investigation i found that the Dart Docs are bit misleading for this method.
test/widgets/button_test.dart
Outdated
|
|
||
| // Now that the widget is being held down, its scale should be at the target scaleEnd i.e 0.95. | ||
| final gesture = await tester.startGesture(tester.getCenter(find.byType(ZulipIconButton))); | ||
| await tester.pumpAndSettle(const Duration(milliseconds: 50)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pumpAndSettle? From its dartdoc:
/// Repeatedly calls [pump] with the given `duration` until there are no
/// longer any frames scheduled. This will call [pump] at least once, even if
/// no frames are scheduled when the function is called, to flush any pending
/// microtasks which may themselves schedule a frame.The AnimatedScale should get set to 0.95 right away, right, and it handles the animation internally. How about:
| await tester.pumpAndSettle(const Duration(milliseconds: 50)); | |
| await tester.pump(); |
test/widgets/button_test.dart
Outdated
|
|
||
| // After releasing, the scale must return to 1.0. | ||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| await tester.pumpAndSettle(); | |
| await tester.pump(); |
test/widgets/button_test.dart
Outdated
|
|
||
| final animatedScaleFinder = find.byType(AnimatedScale); | ||
|
|
||
| // Now that the widget is being held down, its scale should be at the target scaleEnd i.e 0.95. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't add anything necessary that's not already clear from the code; let's remove it.
test/widgets/button_test.dart
Outdated
| await tester.pumpAndSettle(const Duration(milliseconds: 50)); | ||
| checkScale(tester, animatedScaleFinder, 0.95); | ||
|
|
||
| // After releasing, the scale must return to 1.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can also be removed (same reasoning).
| child: AnimatedScaleOnPrimaryPointerDown( | ||
| scaleEnd: 0.95, | ||
| duration: Duration(milliseconds: 100), | ||
| child: UnconstrainedBox( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the UnconstrainedBox needed for the test? Seems like not, and the test would be simpler without it.
365824a to
7d025ed
Compare
|
Thank you @chrisbobbe for the review!, Yes I was also wonder about the Copilot AI feedback. Have pushed the changes, Please take a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
button: Replace GestureDetector with Listener for `AnimatedScaleOnTap`
Fixes #1953.
Latter provides instant scaleDown animation on tap,
unlike GestureDetector which has a `kPressTimeOut` causing
delay of 100ms
https://main-api.flutter.dev/flutter/gestures/kPressTimeout-constant.html
Commit-message nits:
- Empty line after the
Fixesline. - The text after that is redundant with the information in the
Fixesline; let's delete it. (Also I noticedkPressTimeOutis misspelled there.)
See also: #1954 (comment)
|
Thank you @chrisbobbe for the Review, I have replied to the comment #1954 (comment) and updated the commit message. Will keep in mind to insert a new line between the |
7d025ed to
b2cc223
Compare
Fixes #1953.
The Scale down and Scale up as well as Ink Splash animation occurs after a finite delay of 100ms. Mainly due to
GestureDetectorused insideAnimatedScaleOnTap.This can be fixed by using
Listenerhence replacing theGestureDetectorinsideAnimatedScaleOnTap.Discussion: #mobile > issue in responsiveness of home bottom navbar icons
Design: Figma Design
Comparison Through Video of Fix-up
Before
implementation.mp4
After
fixup.mp4