Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Mar 28, 2022

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

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 28, 2022
@bleroux bleroux changed the title Fix InkWell highlight and splash sometimes persists (#96338) Fix InkWell highlight and splash sometimes persists Mar 28, 2022
@bleroux bleroux requested a review from TahaTesser March 28, 2022 20:15
@TahaTesser TahaTesser requested a review from chunhtai March 31, 2022 12:02
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member

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

Copy link
Contributor Author

@bleroux bleroux Apr 5, 2022

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).

Copy link
Member

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

Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

@bleroux bleroux requested review from chunhtai and nt4f04uNd April 5, 2022 08:24
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants