-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Exposes onSecondaryTap in InkWell. #119058
Conversation
17de905
to
97e7030
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.
Hello @chinmoy12c thank you for your contribution. This is a much needed PR and I'm happy to give some feedback.
- The ink splash on secondary tap is not currently enabled. This is because the ink splash is created in
handleTapDown
but that is only called when aPointerDownEvent
comes from akPrimaryButton
. We can take a look athandleTapDown
for guidance on that and probably abstract some of the code there into something like this. For this to work we would also have to handleonSecondaryTapDown
.
void handleAnyDown(TapDownDetails details) {
if (_anyChildInkResponsePressed) {
return;
}
_startNewSplash(details: details);
}
-
We should also call
updateHighlight(_HighlightType.pressed, value: false);
inhandleSecondaryTap
to cancel the ink splash when the click/tap is released. -
I wonder if we should also provide
onSecondaryTapUp
(called at the same time asonSecondaryTap
but provides aTapUpDetails
) in this PR as well as ononSecondaryTapCancel
to be consistent with theprimary
button gestures callbacks thatInkWell
provides.
@@ -1239,6 +1252,7 @@ class _InkResponseState extends State<_InkResponseStateWidget> | |||
onTapCancel: enabled ? handleTapCancel : null, |
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.
If we look at this enabled
property it looks like it includes all of the gesture callbacks. I think we should also add onSecondaryTap
into this to correctly handle those cases.
bool isWidgetEnabled(_InkResponseStateWidget widget) {
return widget.onTap != null || widget.onDoubleTap != null || widget.onLongPress != null || widget.onTapDown != null;
}
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.
Added callbacks like onSecondaryTap
, onSecondaryTapUp
, and onSecondaryTapDown
should also go here.
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.
I noticed that onTapUp
was not included in this, so I added that too.
97e7030
to
395c3d8
Compare
Google Test failures look unrelated. |
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.
I think we should also add entries for the new callbacks here
b3300ee
to
4cca6b7
Compare
@@ -1110,6 +1149,13 @@ class _InkResponseState extends State<_InkResponseStateWidget> | |||
} | |||
} | |||
|
|||
void handleSecondaryTap() { |
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.
I think we should also handle and expose secondary tap cancel in a similar way to other callbacks and handleTapCancel
since it is possible a tap is cancelled and we want the splash to also cancel in that case.
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.
Hey, @Renzo-Olivares I've updated the PR, sorry for the delay missed the notifications.
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.
No worries! Sorry for the delay, i've been out of office for the past week as well.
Hi @chinmoy12c, where you still working on this / did you have any questions about any reviews? |
08ce89e
to
c377ecc
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.
LGTM other than my final comments. Thank you for your patience and for this awesome contribution!
@@ -1110,6 +1149,13 @@ class _InkResponseState extends State<_InkResponseStateWidget> | |||
} | |||
} | |||
|
|||
void handleSecondaryTap() { |
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.
No worries! Sorry for the delay, i've been out of office for the past week as well.
@chinmoy12c Looks like some tests are failing, but they seem unrelated. Maybe a sync up to master will fix them. |
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.
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.
LGTM 👍. Just some nits. Thank you for the PR!
ae0b394
to
a744165
Compare
This has the potential to be a breaking change because InkWell will now steal secondary tap gestures from overlapping widgets. For example, I noticed this exact problem in data-table-2: maxim-saplin/data_table_2#176 The way to fix it is to simply migrate to the new secondary tap API on InkWell rather than using an overlapping widget to receive secondary taps. |
It's worth to have the new events added to TableRowInkWell as well. |
onSecondaryTap still not working for me with trackpad two fingers click on macOS, the ripple effect is loading but then nothing is triggered, a long press is working. [✓] Flutter (Channel stable, 3.10.6, on macOS 13.0 22A380 darwin-x64, locale en-DE) |
This PR adds onSecondaryTap to InkWell.
Fixes: #118622
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.