ftlabs / fastclick

Polyfill to remove click delays on browsers with touch UIs
MIT License
18.66k stars 3.23k forks source link

Clicks stop working on iOS 11.3 after resuming #549

Open lasselaakkonen opened 6 years ago

lasselaakkonen commented 6 years ago

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:

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: https://github.com/framework7io/framework7/commit/ac02ad1514f53ddd47a663f281104049ae59cb52

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 commented 6 years ago

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

Anopetia commented 6 years ago

Thank you!!!!

dimitriadamou commented 6 years ago

Thank you, life saver

toxwj123 commented 6 years ago

Very thank you, solve my problem

benmcmaster commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

Thank you all!!!!

lasselaakkonen commented 6 years ago

@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 commented 6 years ago

@lasselaakkonen I can confirm that 11.4 beta has this issue

lwenke commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

zmen commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

AlexPashley commented 6 years ago

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

romainlq commented 6 years ago

@AlexPashley there is a fork which fixes this issue https://github.com/ftlabs/fastclick/pull/550

gyoong2 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

gyoong2 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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.

ksibod commented 6 years ago

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

ksibod commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

look at this: Events can have a negative timestamp which causes app breakage -- webkit's bug

romainlq commented 5 years ago

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

cdumez commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Any news?