search

amsul / pickadate.js

The mobile-friendly, responsive, and lightweight jQuery date & time input picker.
http://amsul.ca/pickadate.js
MIT License
7.7k stars 1.01k forks source link

Debounce the invocation of the open method - fixes #1138 #1140

Closed DanielRuf closed 5 years ago

DanielRuf commented 5 years ago

I have applied the proposed changes. When we are ready to merge I will squash the commits into one.

amsul commented 5 years ago

@DanielRuf I think we're ready to merge em in. We can ship a new build for 3.6.1 after.

DanielRuf commented 5 years ago

I'll do the change to 50 instead of 100 ms and then squash the commits

DanielRuf commented 5 years ago

Ready to be merged. Thanks for the feedback.

colinhowe commented 5 years ago

Hi, we've just hit this on our site. Using the most recent version of pickadate we're seeing that the close event is being fired immediately after opening ~50% of the time. If you want to replicate it consistently you need to do a long click on the input box (which explains why it happens 50% of the time). When the click is fired it is using the input box as the target element and totally ignoring the new element that has appeared.

You can see this behaviour on the demo site: https://amsul.ca/pickadate.js/

This behaviour only started in Chrome 73 and is not present in Safari 12.0.3.

The dirty fix that springs to mind is to have the open handler register whether the opening click has completed or not. If the click has not been completed then the close handler should do a no-op.

Edit: further debugging has shown that on a slow click the focus event is fired followed by a click event on mouse up. On a normal click just the click event is fired once and so gets handled correctly.

DanielRuf commented 5 years ago

Hi @colinhowe, please read the PR and the comments and see the changes. This is a regression in Chrome. This PR is just a workaround to fix it.

DanielRuf commented 5 years ago

https://bugs.chromium.org/p/chromium/issues/detail?id=941910

DanielRuf commented 5 years ago

This behaviour only started in Chrome 73 and is not present in Safari 12.0.3.

Because webKit !== Chromium ;-)

colinhowe commented 5 years ago

I realise that about Safari. It was more to give additional context. Anyway, I've tracked down what has happened in Chromium. I'll raise a PR as I believe the issue we're seeing is different to this one.

DanielRuf commented 5 years ago

No, it is the exact same issue which is fixed by this PR.

DanielRuf commented 5 years ago

Please try the patch / changes of this PR.

colinhowe commented 5 years ago

I am using master which contains this PR. It is not the exact same issue. You can replicate on your own demo page which is also running this PR.

  1. Go to https://amsul.ca/pickadate.js/
  2. Click and hold on the picker
  3. Release

Expected behaviour: calendar stays open Actual behaviour: it closes

DanielRuf commented 5 years ago
  • Click and hold on the picker
  • Release

No one does this. You have to increase the timeout to dethrottle it. The cause is still the same.

colinhowe commented 5 years ago

The website uses 3.6.2 - I did check before sending you the instructions.

The instructions are purposefully exaggerated. All you have to do is be slow enough between mousedown and mouseup to trigger a DOM change. I figured saying 'click for ~50ms' would be less helpful.

As a general fyi, older people do use the internet. They do click slowly.

gregblass commented 5 years ago

FYI - I just had a client let me know that on some slower machines, 100ms doesn't seem to be enough to do the trick.

DanielRuf commented 5 years ago

@gregblass we are aware of it and there is an open issue which tries a different workaround.

gregblass commented 5 years ago

Got it! Hey would you mind linking me to that?

DanielRuf commented 5 years ago

See https://github.com/amsul/pickadate.js/pull/1145

ivanvgh commented 4 years ago

@DanielRuf I'm sorry. I should have been more active and send the patches to these repos myself. I also got this datepicker and slider repo which I recall I had discussion with you about confused.

Anyhow, the quick fix for this one will be just asking the UA to route the up events to the same input elements that started this with so that the logic of dismissing the popup due to the click handler doesn't get triggered. So before I send the patch and the real fix, could you load this page. I assume this still has the bad behavior here (i.e. popup closing if the user does a long click). Is that right? I'm not familiar with the versioning of this library but I just want to make sure we test the patch on a live example that does show the problem.

Now just open the devtools and paste this code:

document.querySelector('.picker__input').addEventListener('pointerdown', e=>e.target.setPointerCapture(e.pointerId));

It should fix the problem for the first date picker. Do you see any problem after this patch to that page?

You are rigth, but I think this will fix the whole problem: document.querySelectorAll('.datepicker').forEach(t => t.addEventListener('mousedown', e=>e.preventDefault()));