Do not use a timer for an explicit timeout of 0 #1146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `Press` gesture recognizer in HammerJS has problems consistently firing a `pressup` event following a `press` event in the context of small `time` values. This is well-known. See e.g. hammerjs#1011, hammerjs#836, hammerjs#751. We've found that given a pair of `Press` and `Pan` gestures configured as below, the `pressup` event will fairly consistently break ( especially when using touch input ). This particular scenario is used for detecting a `Press` for instant setting of a position and `Pan` gradually controlling a set position ( we specifically use this for creating a "dual-ended between" variant of the `input type="range"` ): ```js { recognizers : [ [ Hammer.Pan, { direction : Hammer.DIRECTION_HORIZONTAL, threshold : 0 }], [ Hammer.Press, { threshold : 1, time : 0 }] ] }``` The root cause is with a granularity problem with browser timeouts as used in the `setTimeoutContext`, which causes part of the gesture logic to fire too late (or not at all). This issue and probably others like it, such as hammerjs#1011, hammerjs#836 or hammerjs#751 can be fixed by not using a timeout at all if the gesture's `time` property is 0.
runspired
requested changes
May 28, 2019
| @@ -9,5 +9,10 @@ import bindFn from './bind-fn'; | |||
| * @returns {number} | |||
| */ | |||
| export default function setTimeoutContext(fn, timeout, context) { | |||
| return setTimeout(bindFn(fn, context), timeout); | |||
There was a problem hiding this comment.
I'd prefer to use resolve().then to schedule a microtask (high priority with immediate flush) to ensure we preserve async semantics.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Pressgesture recognizer in HammerJS has problems consistently firing apressupevent following apressevent in the context of smalltimevalues. This is well-known. See e.g. #1011, #836, #751.We've found that given a pair of
PressandPangestures configured as below, thepressupevent will fairly consistently break ( especially when using touch input ).This particular scenario is used for detecting a
Pressfor instant setting of a position andPangradually controlling a set position ( we specifically use this for creating a "dual-ended between" variant of theinput type="range"):The root cause is with a granularity problem with browser timeouts as used in the
setTimeoutContext, which causes part of the gesture logic to fire too late (or not at all). This issue and probably others like it, such as #1011, #836 or #751 can be fixed by not using a timeout at all if the gesture'stimeproperty is 0.