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

Chrome version 73 causing problems #1138

Closed hydemalion closed 5 years ago

hydemalion commented 5 years ago

In Chrome 73 desktop we're seeing various odd behaviors in both the newest version of pickadate and the older version we were using.

One user reported the datepicker opens as soon as the page loads instead of waiting for him to click on the input. (unable to reproduce as of yet).

I myself am seeing the datepicker close and run the OnClose event immediately after clicking to open it.

On the examples on your demo site https://amsul.ca/pickadate.js/, I'm seeing what appears to be the datepicker opening twice; it flashes up, then flashes down, then flashes up again. In the console, for the events example that logs the events I see "opened up", "closed now", and then "opened up" again.

I don't see any of this behavior in Edge or Firefox. I also didn't see the problem in Chrome 72,

leesalminen commented 5 years ago

Same here!

DanielRuf commented 5 years ago

Sounds like a regression in Chrome which would be a browser bug.

We will check if we have to adjust something but if it is a bug in Chrome it should be reported in their issue tracker.

leesalminen commented 5 years ago

@DanielRuf I agree that it definitely sounds like a regression in Chrome and not pickadate's "fault". With that being said, Chrome is the most popular browser and with evergreen updates my users are getting updated without their knowledge.

Any chance this project would accept a donation for helping me resolve this issue today? I can send $1000 via BTC/Cash App/Venmo/whatever, but I really need a fix today!

leesalminen commented 5 years ago

I'm just digging through the source code commenting out lines of code to see if I can get a change of behavior. So far, I'm finding this in picker.js:

 // If the target of the event is not the element, close the picker picker.
                        // * Don’t worry about clicks or focusins on the root because those don’t bubble up.
                        //   Also, for Firefox, a click on an `option` element bubbles up directly
                        //   to the doc. So make sure the target wasn't the doc.
                        // * In Firefox stopPropagation() doesn’t prevent right-click events from bubbling,
                        //   which causes the picker to unexpectedly close when right-clicking it. So make
                        //   sure the event wasn’t a right-click.
                        // * In Chrome 62 and up, password autofill causes a simulated focusin event which
                        //   closes the picker.
                        if ( ! event.isSimulated && target != ELEMENT && target != document && event.which != 3 ) {

                            // If the target was the holder that covers the screen,
                            // keep the element focused to maintain tabindex.
                            P.close( target === P.$holder[0] )
                        }

When I comment out P.close(), the picker remains open in Chrome 73 as it should. Trying to work out the conditions of that if statement now.

leesalminen commented 5 years ago

I think it has something to do with Event.path (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h-JwMiPUnuU/discussion) being removed in Chrome 73?

leesalminen commented 5 years ago

So it looks like getRealEventTarget() looks for event.path, which was removed in Chrome 73. It looks like we're supposed to use event.getComposedPath() now as far as I can tell.

leesalminen commented 5 years ago

@hydemalion for now, I'm commenting out

P.close( target === P.$holder[0] )

in picker.js as a workaround. The downside is that you have to explicitly select a date or click the "Close" button to close the picker (you can't click elsewhere on the screen to dismiss).

It looks like the Shadow DOM API has changed in Chrome 73, which the picker is relying on to determine whether a user clicked on the picker to open it, or elsewhere on the page to close it.

hydemalion commented 5 years ago

@leesalminen we were just coming to the conclusion that disabling the click outside to close for chrome was the best workaround for now, thanks so much for your help finding that!

DanielRuf commented 5 years ago

2018Q4

Start showing deprecation message on stable channel (M70)

Were there deprecation warnings in 70+ stable?

Also I guess you both use Canary so 73 is not shipped to normal users until April.

Note: even though Event.path was originally defined with Shadow DOM V0, and is superseded by Event.composedPath() in the V1 spec, Event.path can work without Shadow DOM V0 and will be deprecated separately.

https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath#Browser_compatibility

leesalminen commented 5 years ago

@DanielRuf

https://chromereleases.googleblog.com/2019/03/stable-channel-update-for-desktop_12.html

The Chrome team is delighted to announce the promotion of Chrome 73 to the stable channel for Windows, Mac and Linux. This will roll out over the coming days/weeks.

I can confirm that I am not running Canary/Dev channel. I've had several hundred users report this issue starting today. Looks like 73 is getting rolled out to all users now.

To be honest, I don't remember seeing any deprecation notice in 70. Just posting my stream of thoughts as I debug this.

I can confirm that commenting out P.close( target === P.$holder[0] ) in the open method in picker.js mostly resolves the issue (for me, at least). It looks like open calls getRealEventTarget, which uses event.path. It seems like that how that function works has changed with Chrome 73. Again, just my hunch. Could be totally wrong.

hydemalion commented 5 years ago

@DanielRuf I didn't notice any depreciation messages either, but I also wasn't looking for them, we're using this plugin on a site that we don't touch/change very frequently. And I'm also not using Canary.

DanielRuf commented 5 years ago

Looks like 73 is getting rolled out to all users now.

Seems they shipped it too early and deprecated it too early.

Well, I'll check if we can extend the code part to include the new method.

It seems it was promoted to stable and rolled out after my work day.

It would have been better if the Chrome team would have respected their plans (separate deprecation, release in April, ...).

Now my local Chrome instance has also updated to M73.

Seems 75 is now Canary and there it is worse.

Please test https://codepen.io/DanielRuf/pen/EMoRKp

hydemalion commented 5 years ago

Getting console error to the effect of "event.composedPath is not a function", and not just in chrome, in other browsers as well.

amsul commented 5 years ago

@DanielRuf I haven't had a chance to look into this as I'm quite busy at work..but just an fyi, this is how we get the event's path in v5: https://github.com/amsul/pickadate.js/blob/47e8dab60955da0e46fa547820c8a95181890f8f/lib/renderers/dom/getValueFromMouseEvent.js#L37-L56

leesalminen commented 5 years ago

@DanielRuf https://codepen.io/DanielRuf/pen/EMoRKp is working for me in v73 and v72. Would you like a hand testing on v75?

amsul commented 5 years ago

@leesalminen ah I understand your frustration..regarding the donation, that is always appreciated but it's difficult to guarantee we can resolve it "today".

I haven't fully set up our funding distribution model yet, but we do have a BTC account now where we accept donations should you feel inclined: (on the bottom right https://amsul.ca/pickadate.js/v5/).

Whichever one of our contributors is able to get to it, for now, I'd manually distribute the bounty to them 🌟

leesalminen commented 5 years ago

@DanielRuf can confirm @hydemalion report of console error for event.composedPath is not a function. The datepicker still "works" though and doesn't close as soon as its opened. Looks like you could change

if ( event.path || event.composedPath() ) {
    path = event.path || event.composedPath()
}

to

if ( event.path || event.composedPath ) {
    path = event.path || event.composedPath()
}

Basically just testing that event.composedPath exists and not that the output of the function is truthy.

DanielRuf commented 5 years ago

Mh, does not seem to work with the solution that you recommend. Will investigate further.

I still think path is still not deprecated. Otherwise event.composedPath is not a function would never be thrown.

leesalminen commented 5 years ago

@DanielRuf I'm just headed into a meeting for about an hour and then I'll be back. One thing maybe important to keep in mind is that I'm using container: 'body' in my $.pickadate options object. Don't know if that throws a wrench into anything. I don't know where you're located, but if it's dinner time soon we can pick this up tomorrow. I'll still pay :).

leesalminen commented 5 years ago

@DanielRuf I mean, you could do

if( event.thispropdoesntexist || event.composedPath() ) {

}

and it'll exec composedPath(). undefined is falsey?

DanielRuf commented 5 years ago

Correct check would be with typeof but still this is not a valid method as I think jQuery.Event still provides only .path as this is not the real DOM event (different APIs).

caseyjhol commented 5 years ago

For reference, check the console of this reduced test case (without pickadate.js) while clicking the input: https://jsfiddle.net/caseyjhol/gftba6vc/3/. event.path is working correctly.

caseyjhol commented 5 years ago

As a hacky workaround, I tried passing through the event from the initial focus click event listener in the prepareElement function (as event.path is correct there for some reason), and it's working: https://plnkr.co/edit/itltewqb5mJgew55KZtd?p=preview. Relevant lines: 229, 268, 270, 637.

DanielRuf commented 5 years ago

The cause is something else much deeper in the code. In the open method we add another listener which triggers the close method but this is directly fired as it seems.

The other PR with the typeof addition is still needed to support future Chromium / Blink based browsers.

So far Event.path does not seem to be deprecated.

https://jsfiddle.net/caseyjhol/gftba6vc/3/. event.path is working correctly

That's what I wrote but still, Chrome will change this in the near future too. It is just a red herring currently and not the cause.

As a hacky workaround, I tried passing through the event from the initial focus click event listener in the prepareElement function (as event.path is correct there for some reason), and it's working:

Not really a solution. I think there is some event propagation and bubbling issue. I will come up with a solution later today.

caseyjhol commented 5 years ago

Oh, definitely not a solution. Was mainly just posting it for anybody who needs a temporary workaround.

DanielRuf commented 5 years ago

Found the cause, we have a race condition.

https://codepen.io/DanielRuf/pen/QoQLMv

Please test and verify if this works for you.

https://github.com/amsul/pickadate.js/commit/62a186094f60a4a8a7ac210cbab36f855b8c639e

https://github.com/amsul/pickadate.js/commit/62a186094f60a4a8a7ac210cbab36f855b8c639e.patch

If this is the solution and the event handling in Chrome 73+ the cause I will open a Chromium bugtracker entry to check with the Chromium team if this is a regression or if we have used an anti pattern.

pandzorz commented 5 years ago

That somewhat resolves it. Clicking will open it, close, then open again. It looks to be something to do with mouseup event. If you click, hold, release, it will close on release. If you right click to open, it will stay open. You can also tab into the field and it will stay open.

DanielRuf commented 5 years ago

Mh, does not seem to resolve this.

Ok, other approach: https://codepen.io/anon/pen/QoQbGd

dontGiveFocus was not correctly set due to strict comparison.

The part with the dontGiveFocus is the problematic part due to the click event and focus event.

At least there is some very weird behavior in Chrome @amsul. Not sure if (and how) we should fix this.

Somehow the event target is always body.

DanielRuf commented 5 years ago

Let's see if the Chromium team can bisect the change which causes this. Unfortunately it is harder to bisect Chrome than Firefox (who provide mozregression for this but there is no solution for Chrome).

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

DanielRuf commented 5 years ago

Currently debouncing the call (focus and click are fired at the same moment) seems to solve this: https://codepen.io/DanielRuf/pen/WmMwGG

hydemalion commented 5 years ago

Currently debouncing the call (focus and click are fired at the same moment) seems to solve this: https://codepen.io/DanielRuf/pen/WmMwGG

This last example seems to be working in 73 for me.

mattonit commented 5 years ago

Has anyone fixed it up already?

DanielRuf commented 5 years ago

Has anyone fixed it up already?

See https://github.com/amsul/pickadate.js/commit/9282a8021c47e39d1aee72d69557d6e4ee68034d

https://github.com/amsul/pickadate.js/commit/9282a8021c47e39d1aee72d69557d6e4ee68034d.patch

DanielRuf commented 5 years ago

@amsul what do you think? So far we have no feedback from the Chrom(ium) team.

amsul commented 5 years ago

@DanielRuf good stuff! 👏

Looking into the PR now.

jigs144379 commented 5 years ago

@DanielRuf I agree that it definitely sounds like a regression in Chrome and not pickadate's "fault". With that being said, Chrome is the most popular browser and with evergreen updates my users are getting updated without their knowledge.

Any chance this project would accept a donation for helping me resolve this issue today? I can send $1000 via BTC/Cash App/Venmo/whatever, but I really need a fix today!

Hay leesalminen,

I have resolved this issue permanently. issue in materialize js

jigs144379 commented 5 years ago

For solution send me mail , my email id is : khoradiya.jignesh@gmail.com

DanielRuf commented 5 years ago

For solution send me mail , my email id is : khoradiya.jignesh@gmail.com

Sorry but this does not work like this. Please share with us which exact issue you had and how you solved it. It will also help others.

DanielRuf commented 5 years ago

The Chromium team bisected the builds and found the causing change.

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

So the regression is verified.

amsul commented 5 years ago

@DanielRuf thanks for triaging this forward 🙇

amsul commented 5 years ago

@DanielRuf @hydemalion 3.6.1 is now published. It resolves this race condition introduced in Chrome 73 👍

jigs144379 commented 5 years ago

Please provide proper solution, Thanks a lot in advance

On Sat, Mar 16, 2019, 5:09 AM amsul notifications@github.com wrote:

@DanielRuf https://github.com/DanielRuf @hydemalion https://github.com/hydemalion 3.6.1 is now published. It resolves this race condition introduced in Chrome 73 👍

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/amsul/pickadate.js/issues/1138#issuecomment-473473523, or mute the thread https://github.com/notifications/unsubscribe-auth/APSREmYI5mXjflKnluvwCNSNP5sZ4tuYks5vXC86gaJpZM4btubq .

DanielRuf commented 5 years ago

Please provide proper solution, Thanks a lot in advance

We did, please check the last commit.

DanielRuf commented 5 years ago

https://github.com/amsul/pickadate.js/issues/1138#issuecomment-472884198

nicolomonili commented 5 years ago

As anyone tested version 3.6.1? Even with this version (on Google Chrome v73) i have the same problem.

DanielRuf commented 5 years ago

As anyone tested version 3.6.1? Even with this version (on Google Chrome v73) i have the same problem.

Did you try to increase the timeout and clear your browser cache? So far we can not do much more than increasing the timeout value.

nicolomonili commented 5 years ago

Yes it is not a cache problem (i have this on my js/css files -> file.js/css?<?= $v;?>. This timeout?

DanielRuf commented 5 years ago

This timeout?

Yes, this timeout value.

nicolomonili commented 5 years ago

With a value of 100 seems to be much better!

DanielRuf commented 5 years ago

With a value of 100 seems to be much better!

Does it solve your problem?

@amsul should we increase it? Keep in mind that we also have to change it in the tests then.