-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Fix InkWell highlight and splash sometimes persists #100880
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
Conversation
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.
Should this be in _startSplash? otherwise, you need to handle this in _simulateTap and _simulateLongPress
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 you do put it into _startSplash, can you also rename it to something like _startNewSplash
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 don't think it is correct to cancel the splash each time the pointer goes down
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.
@nt4f04uNd
I too had the same concern at first.
_currentSplash stores a splash for a small timeframe, starting with onTapDown and ending with a cancel() or a confirm() at gesture end. Maybe a better name could be found for this variable ?
Previous code assumed that onTapDown could not be called twice before first gesture end. Unfortunately this is possible when handling some slow double taps : first call to onTapDown starts a splash, stores it in _currentSplash, second call starts a new splash and overiddes _currentSplash. The first started splash is neither canceled or confirmed : it is stale and stay visible forever (InkSplash assumes that cancel() or confirm() will be called to animate splash alpha to 0.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.
@bleroux I'm not sure what would be a correct solution to the issue you mention, but cancelling the splash right away means it has no chance to ever be confirmed, so it still doesn't seem right to me
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 change will cancel splash in very rare cases as _currentSplash is nullified when a gesture ends.
Here 'cancel' means only means 'animating alpha to 0.0'. We can also decide to 'confirm' as it only means 'animating alpha to 0.0 and grow circle quickly'.
Without this change the issue is very visible to end users (splash are stale, here is a new issue filled today #101750).
@chunhtai thank you to share your take on this ? Is there something about it in Material spec ?
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.
@nt4f04uNd If the previous splash is not confirmed or cancelled when the second tap down, it is likely no one is tracking the previous tap. In this case, it makes sense to me to cancel it. This is also a rare cases which should only happen in this delay double taps.
@bleroux I couldn't find this specific case in material design.
chunhtai
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.
LGTM
83a1dab to
dfccb1e
Compare
This PR fixes two issues related to #96338
1 - InkWell highlight sometimes persists when InkWell defines an onDoubleTap handler
2 - InkWell splash sometimes persists when InkWell defines an onDoubleTap handler
(A DartPad gist that reproduces those issues : https://dartpad.dev/?id=1ec7aa89eb9c65c79a3840443a3092c5)
Pre-launch Checklist
///).