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

Clicks stop working on iOS 11.3 after resuming #549

Open
lasselaakkonen opened this issue Apr 10, 2018 · 34 comments
Open

Clicks stop working on iOS 11.3 after resuming #549

lasselaakkonen opened this issue Apr 10, 2018 · 34 comments

Comments

@lasselaakkonen
Copy link

lasselaakkonen commented Apr 10, 2018

I have reproduced this issue with a Cordova app using UIWebView on iOS 11.3 on multiple devices.

According to this discussion, this issue applies to mobile Safari as well:
http://forum.framework7.io/t/ios-11-3-phonegap-resume-problem/2912

To reproduce the issue with Cordova:

  • Open app
  • Put app in background
  • Resume app after 2-5min
    -> click events are triggered with event.timeStamp as a negative number

The negative timestamp is then incremented in successive clicks and clicking starts working when the number has increment above 0.

Framework7 has fixed the issue here:
framework7io/framework7@ac02ad1

I have forked this project and fixed the issue in a similar way here:
https://github.com/lasselaakkonen/fastclick/tree/fix-ios-11-3-event-timestamps

I am not familiar with FastClick internals, are there going to be any issues with using the current time as a timestamp, instead of reading it from the event?

@pollingj
Copy link

Thanks so much for this. A massive life saver! 🥇

@Anopetia
Copy link

Thank you!!!!

@dimitriadamou
Copy link

Thank you, life saver

@toxwj123
Copy link

Very thank you, solve my problem

@benmcmaster
Copy link

benmcmaster commented Apr 15, 2018

@lasselaakkonen Awesome Thank you!!!

Question: I looked at your branch's diff: why are you checking for iOS 11.3 specifically?

var deviceIsIOS11_3 = deviceIsIOS && (/OS 11_3(_\d)?/).test(navigator.userAgent);

Shouldn't we just use (new Date()).getTime(); globally? What happens in 11.4? Are you assuming it will be fixed?

@lasselaakkonen
Copy link
Author

@benmcmaster I do not know how sensitive FastClick is to the changes in timestamps and I do not know how how much of a difference there can be between event.timeStamp and (new Date()).getTime(), so to play it safe and not break any other platforms I made the change only to iOS 11.3.

I am hoping it will be fixed in in iOS 11.4. And it might already be fixed, because I think I had iOS 11.4 beta installed on a device and I could not replicate the issue on that .. if I remember correctly. I could verify that.

@vahvarh
Copy link

vahvarh commented Apr 16, 2018

line 336 you have a bug:
if (deviceIsIOS && targetElement.setSelectionRange && targetElement.type.indexOf('date') !== 0 && targetElement.type !== 'time' && targetElement.type !== 'month' && targetElement.type !== 'email') {

should read

    if (deviceIsIOS && targetElement.setSelectionRange && targetElement.type.indexOf('date') !== -1 && targetElement.type !== 'time' && targetElement.type !== 'month' && targetElement.type !== 'email') {

@lanshengzhong
Copy link

Thank you all!!!!

@lasselaakkonen
Copy link
Author

@vahvarh I have not modified that line.

(Offtopic: but if you are referring to targetElement.type.indexOf('date') !== 0, it could probably also be !== -1 and it wouldn't change the behavior)

@sergetk
Copy link

sergetk commented Apr 17, 2018

@lasselaakkonen
I can confirm that 11.4 beta has this issue

@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;
I was hoping there could be a fix that would still work if it was using iOS 11.4 and the bug still existed.

@rmondello
Copy link

It's great that there's a workaround for this! Has anyone filed a bug at the WebKit project bug tracker (https://bugs.webkit.org) or Apple's bug tracker (https://bugreport.apple.com)?

@cdumez
Copy link

cdumez commented Apr 26, 2018

I filed https://bugs.webkit.org/show_bug.cgi?id=185040, I am looking into this bug. Thanks.

@zmen
Copy link

zmen commented Apr 27, 2018

In my case, event.timeStamp doesn't return negative value, but it indeed became smaller than lastClickTime after resuming. Once resuming, it increased normally.
So I think instead of checking timeStamp negative or replace it with new Date().getTime(), we may just do this:

    // if (event.timeStamp - this.lastClickTime < this.tapDelay) {
    if (event.timeStamp - this.lastClickTime < this.tapDelay && event.timeStamp > this.lastClickTime) {
      event.preventDefault();
    }

@lasselaakkonen
Copy link
Author

@cdumez Thank you for looking in to the issue and fixing it in WebKit. Do you know how WebKit/iOS releases work, when can that fix be expected to be released?

The issue has now been reproduced on iOS 11.3 (at least by me and many others in this thread on multiple devices) and iOS 11.4 beta (at least by @sergetk). The timeStamp values somehow decrease when the app is in the background, meaning timestamps can be smaller after resuming and even negative.

I have removed the iOS 11.3 detection from my branch and now the trickery is done like this in onTouchStart:

		if (event.timeStamp < 0) {
			touchStartTime = (new Date()).getTime();
			this.isTrackingClickStartFromEvent = false;
		} else {
			touchStartTime = event.timeStamp;
			this.isTrackingClickStartFromEvent = true;
		}

And onTouchEnd tries to use the same timestamps as in onTouchStart like this:

		if (this.isTrackingClickStartFromEvent) {
			touchEndTime = event.timeStamp;
		} else {
			touchEndTime = (new Date()).getTime();
		}

@zmen I believe that is related to another issue, which also causes the negative timestamps, but does not break FastClick completely. When timestamps are smaller after resuming, than before pausing, those lastClickTime calculations will be wrong. I didn't try to figure out what the calculations are doing, but they will definitely and up with wrong results when lastClickTime is larger than timeStamp. Looking at the patch to WebKit by @cdumez, it seems like WebKit can still return smaller timestamps after resuming: https://bugs.webkit.org/attachment.cgi?id=339005&action=prettypatch

(For some inexplicable I was not able to reproduce the bug now with FastClick in my app... I was getting negative timestamps, but clicks were still working. I'm not sure what was going on, but those latest changes to FastClick should not break any existing behavior, if platforms return correct timestamps.)

@cdumez
Copy link

cdumez commented Apr 28, 2018

@lasselaakkonen: Unfortunately, Apple does not comment on when a particular fix will ship to customers.

@AlexPashley
Copy link

is there an example of a fully updated working version of fastclick?

@romainlq
Copy link

romainlq commented May 2, 2018

@AlexPashley there is a fork which fixes this issue #550

@gyoong2
Copy link

gyoong2 commented May 2, 2018

Is anyone still having this issue even after taking the fix? I haven't been able to narrow it down, but after doing some exhaustive testing, there still seems to be some instances in where the clicks aren't working.

Also, this.isTrackingClickStartFromEvent never seems to be reset once it's set to true. Could this create a scenario where you're comparing different "timestamps" in the onTouchEnd to determine if there has been a "click"?

@lasselaakkonen
Copy link
Author

@gyoong2 Have you found some specific scenario where the this.isTrackingClickStartFromEvent hack does not work? The flag is set on every onTouchStart. Is there some scenario where touch events do not initiate from onTouchStart?

I'm sure this is still an issue:

When timestamps are smaller after resuming, than before pausing, those lastClickTime calculations will be wrong. I didn't try to figure out what the calculations are doing, but they will definitely end up with wrong results when lastClickTime is larger than timeStamp.

.. but I haven't looked in to it, I'm guessing it might break a click immediately after resuming?

@sqhtiamo
Copy link

sqhtiamo commented May 3, 2018

@lasselaakkonen 'timestamps are smaller after resuming' also occurred in my cases, after several tests.

@gyoong2
Copy link

gyoong2 commented May 3, 2018

@lasselaakkonen - I have not explicitly seen when this.isTrackingClickStartFromEvent was failing. It was more of an observation, but to your point, onTouchEnd can't be called before onTouchStart, so this should actually should be good. I probably fell into the scenario where 'timestamps are smaller after resuming'.

Saintless added a commit to Saintless/fastclick that referenced this issue 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
@Saintless
Copy link

I've added a pull request based on the one by @lasselaakkonen

I was still experiencing the problem after #550 but thanks to that pull, I was able to understand where the complete fix for my issue was.

@willcalderbank
Copy link

Even after trying both @lasselaakkonen and @Saintless patches, my client is still reporting the same issue. Anyone else found that it doesnt solve the problem?

@skicson
Copy link

skicson commented Jun 5, 2018

@willcalderbank yes, I'm still seeing the problem after using this latest code. I'm trying to insert some instrumentation so I can figure out what's going on...

@cdumez
Copy link

cdumez commented Jun 5, 2018

By the way, the first iOS 12 developer seed came out yesterday and it would be good if you could confirm that the bug is completely fixed in the seed. Thanks.

skicson added a commit to skicson/fastclick-abs-event-timestamp that referenced this issue Jun 5, 2018
@ksibod
Copy link

ksibod commented Jun 18, 2018

@cdumez I cannot replicate the issue with the iOS 12 developer preview. (Using the published fastclick package 1.0.6)

@ksibod
Copy link

ksibod commented Jun 19, 2018

I'd also like to point out that as long as my device was connected to a Mac computer through USB, I could not reproduce the issue (since this is a timestamp issue, I'm guessing iOS was utilizing the timestamp from the computer during those wake events - not sure about this).

As soon as I disconnected the USB cable and tried the steps from OP, I was able to reproduce the issue.
iPhone 7 - 11.3.1

@gwynjudd
Copy link

gwynjudd commented Jun 26, 2018

Please try out the fix in #564 and report back success/fail. I don't have the resources to fully test it myself but I just wanted to try a slightly different attack at the problem

@caoweiju
Copy link

@romainlq
Copy link

So what does it say ? Has it been fixed ? In which ios version ?

@cdumez
Copy link

cdumez commented Nov 28, 2018

I already provided the WebKit bug report earlier in this thread. I also mentioned that my fix went into iOS 12. @ksibod also reported he could no longer reproduce the issue with iOS 12.

@ghost
Copy link

ghost commented Jan 23, 2019

Is this version available now, Still I see old changes on master branch.
https://www.npmjs.com/package/fastclick still this is showing old version.
When can we expect the update?

@kenberkeley
Copy link

Any news?

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

No branches or pull requests