Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chrome version 73 causing problems #1138

Closed
hydemalion opened this issue Mar 13, 2019 · 60 comments
Closed

Chrome version 73 causing problems #1138

hydemalion opened this issue Mar 13, 2019 · 60 comments

Comments

@hydemalion
Copy link

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
Copy link

Same here!

@DanielRuf
Copy link
Collaborator

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
Copy link

@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
Copy link

leesalminen commented Mar 13, 2019

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
Copy link

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
Copy link

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
Copy link

@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
Copy link
Author

@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
Copy link
Collaborator

DanielRuf commented Mar 13, 2019

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
Copy link

@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
Copy link
Author

@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
Copy link
Collaborator

DanielRuf commented Mar 13, 2019

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
Copy link
Author

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

@amsul
Copy link
Owner

amsul commented Mar 13, 2019

@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:

const getEventPath = (event: Event) => {
// $FlowFixMe
if (event.path) {
return event.path
}
const path = []
let target = event.target
// $FlowFixMe
while (target.parentNode) {
path.push(target)
// $FlowFixMe
target = target.parentNode
}
path.push(document, window)
return path
}

@leesalminen
Copy link

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

@amsul
Copy link
Owner

amsul commented Mar 13, 2019

@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
Copy link

@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
Copy link
Collaborator

DanielRuf commented Mar 13, 2019

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
Copy link

@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
Copy link

@DanielRuf I mean, you could do

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

}

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

@DanielRuf
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

DanielRuf commented Mar 14, 2019

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
Copy link
Collaborator

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

@DanielRuf
Copy link
Collaborator

DanielRuf commented Mar 14, 2019

Found the cause, we have a race condition.

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

Please test and verify if this works for you.

62a1860

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.

@DanielRuf
Copy link
Collaborator

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
Copy link

nicolomonili commented Mar 18, 2019

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

@DanielRuf
Copy link
Collaborator

This timeout?

Yes, this timeout value.

@nicolomonili
Copy link

With a value of 100 seems to be much better!

@DanielRuf
Copy link
Collaborator

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.

@nicolomonili
Copy link

Yes the problem is solved (in my case). I did the tests on Google Chrome v73.0.3683.75.
I tried with 50/60/70/80/90/100.

NateWr added a commit to NateWr/restaurant-reservations that referenced this issue Mar 18, 2019
Updates simple-admin-pages to 2.1.2, in order to address a regression
in Chromium that caused issues with the pickadate.js library.

amsul/pickadate.js#1138
@hydemalion
Copy link
Author

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.

@DanielRuf I also have found that 50 isn't a long enough timeout for me, in testing.

@DanielRuf
Copy link
Collaborator

#1142 should improve this.

@mvaragnat
Copy link

mvaragnat commented Mar 27, 2019

I tested the patch proposed by @caseyjhol (on version 3.5, for CSS reasons. I hope it doesn't change the logic below). It seems to me that it works better than the implemented solution...

My understanding of the problem : Using a Mac Pro trackpad, a "click" triggers a parasit click event on Chrome 73, when the finger is released. This click event does not have the right target for reasons that escape me. It therefore triggers the click listener set in the "open" function, to close the picker.

  • the implemented solution adds a minimum delay for the closing click. This filters out most parasit clicks from Chrome 73. However if the user takes too much time to release her finger, the delay can easily be larger than the min delay, and the picker will be closed

  • the patch from @caseyjhol sets the right path for the event. I don't fully understand what is going on with the event etc, but it is less fragile to user behavior

Am I missing something? Thanks for your code and reactivity in any case

@DanielRuf
Copy link
Collaborator

See #1138 (comment)

@DanielRuf
Copy link
Collaborator

@caseyjhol not sure if your code + mine correctly resolves it. Currently it is a nasty regression in Chromium and we should not try to fix browser bugs especially if they are so complicated.

@mvaragnat
Copy link

I think it's good that you guys have worked on a patch because it's breaking for our users 👍
perhaps a combination of both solutions could be the most robust I don't know

@leads
Copy link

leads commented Apr 10, 2019

Our team had this issue. FWIW we are using the classic theme and it was only an issue when the datepicker opened directly above the input. In the end we just changed the CSS so the .picker opened below the input.

@ConradSollitt
Copy link

This is still an ongoing issue in Chrome 85 and I see pickadate.js has not been updated in over a year so I'm not sure if this project is abandoned or not.

Anyways in case other developers find this here is a solution on how to solve this.

https://stackoverflow.com/questions/55200244/materialize-date-picker-automatically-hide-after-opening-problem-on-chrome

var $input = $('.datepicker').pickadate({
    selectMonths: true,
    selectYears: true,
    format: 'yyyy-mm-dd',
});
$input.on('mousedown', function cancelEvent(evt) {
    evt.preventDefault();
});

@DanielRuf
Copy link
Collaborator

@ConradSollitt it should be fixed in the latest release

Materialize (I'm currently one of the maintainers of a fork and I worked on it too) bundles a custom and old version of pickadate.

Still, Google has to fix their stuff. I guess the bugreport is still open.

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

No branches or pull requests