Skip to content
This repository has been archived by the owner on Jun 9, 2022. It is now read-only.

Fix negative event.timeStamp values on iOS 11.3 #550

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lasselaakkonen
Copy link

octobeard added a commit to octobeard/fastclick that referenced this pull request Apr 18, 2018
octobeard added a commit to Keenwawa/fastclick that referenced this pull request Apr 18, 2018
@lwenke
Copy link

lwenke commented Apr 24, 2018

The comment said "iOS 11.3 returns negative values for event.timeStamp after pausing/resuming a Cordova application". If that is true, what about this fix?
touchStartTime = (event.timeStamp < 0) ? (new Date()).getTime() : event.timeStamp;

@lasselaakkonen
Copy link
Author

That would also work, it would have the benefit of not having to detect platforms and hope for the best.

There is an unlikely edge case where it will fail, when event.timeStamp changes from negative to positive, but even that could be worked around by doing touchEndTime = (event.timeStamp < 0 || event.timeStamp < this.trackingClickStart ? (new Date()).getTime() : event.timeStamp) or by saving the source of the touch start timestamp (event vs Date) in onTouchStart and using the same source in onTouchEnd. Does that edge case matter for FastClick?

Normally the timestamps would increment like 3,4,5,6,7,.. but then at some point for some reason iOS starts returning them like this -4,-3,-2,-1,0,1,2,3,4,.. so the timestamps will at some point jump from negative to positive and then with (event.timeStamp < 0) ? (new Date()).getTime() : event.timeStamp FastClick will end up comparing a very large number ((new Date()).getTime()) and a lot smaller number (event.timeStamp, which on iOS seems to be some kind of "milliseconds since this session started" counter).

Should I change it like this?

    if (event.timeStamp < 0) {
      touchStartTime = (new Date()).getTime();
    } else {
      touchStartTime = event.timeStamp;
    }
    ...
    this.trackingClickStart = touchStartTime;
    if (event.timeStamp < 0 || event.timeStamp < this.trackingClickStart) {
      touchEndTime = (new Date()).getTime();
    } else {
      touchEndTime = event.timeStamp;
    }

Or simply check for event.timeStamp < 0?

@lwenke
Copy link

lwenke commented Apr 25, 2018

Sorry I don't really understand what is going on in the code but it would be really good if you could update it soon so that it works for iOS 11.4+ as well (if the problem is still there) so that if people download the fixed version of the app it would also work if they updated it to iOS 11.4.

I guess it would be good to take into account that edge case too.

@lasselaakkonen
Copy link
Author

Sorry, the consecutive integers were a bit confusing, it was just to illustrate that the numbers are integers and increment from 0 -> +INT or -INT -> +INT.

On iOS event.timeStamp values are milliseconds since something.

@lwenke
Copy link

lwenke commented Apr 26, 2018

Thanks for your code above - I edited the code using it.

…lues. Use onTouchEnd timestamp source based on onTouchStart timestamp source.
Saintless added a commit to Saintless/fastclick that referenced this pull request May 4, 2018
The value of lastClickTime only gets updated in onTouchEnd, after the value of lastClickTime is already compared to touchEndTime.

The value of lastClickTime is originally based on the event.timeStamp. When the event.timeStamp comes through as negative, the new value of touchStartTime can no longer be validly compared to the timeStamp, and the function will (often) return before lastClickTime can get updated. The same issue will manifest, until after the value of touchEndTime exceeds the original value of lastClickTime.

Setting lastClickTime to 0 when switching over to get a value based on getTime will help to avoid this. When switching over to the non-event.timeStamp value, lastClickTime should only be set to 0 once. All time comparisons from then on out should be made against a point in time, obtained when switching over, and compare milliseconds since that time.

This work was based on a pull request from @lasselaakkonen and is a fix to issue 549.
ftlabs#550
ftlabs#549
lib/fastclick.js Outdated
touchEndTime = (new Date()).getTime();
} else {
touchEndTime = event.timeStamp;
}

Choose a reason for hiding this comment

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

Is it possible for event.timeStamp to be positive for the touchstart event but negative for the touchend event? In which case, it would be better to only use (new Date()).getTime() instead of the event.timeStamp.

Otherwise, the comparisons below would cause the touchend to exit early:

 		// Prevent phantom clicks on fast double-tap (issue #36)
 		if ((touchEndTime - this.lastClickTime) < this.tapDelay) {

@mantovanig
Copy link

Please Merge!! 🙏 @JakeChampion

@danicholls
Copy link

This is one of the few areas where this library is really needed. Would be great to get this fix merged in!

@caoweiju
Copy link

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants