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

Add mousedown/up tracking to allow for change in behaviour in Chrome 73 #1145

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

colinhowe
Copy link
Contributor

Chrome 73 changes the behaviour of click events when the mousedown is on one element
and the mouseup is on another. The previous behaviour was to not emit any click event
at all. The new behaviour is to emit a click event with the target being the
common ancestor of the elements involved.

In the case of pickadate the calendar is popped up over the input element
meaning that the mousedown event targest the input but the mouseup targest
the calendar itself. This results in the click event targetting the common
ancestor which is not particularly useful.

The workaround here is to track the mousedown and mouseup events during
calendar opening and ignore any click events generated during this.

See https://bugs.chromium.org/p/chromium/issues/detail?id=946459&can=2&q=click
for additional details.

A minimal codepen demonstrating the issue in Chrome is available here
https://codepen.io/mortah/pen/oVRVxa

This manifests itself in pickadate as the picker opening and shutting really quickly. If you want to
replicate it reliably then you need to hold the mouse down slightly before releasing. You can do this
on the demo page for pickadate (https://amsul.ca/pickadate.js/)

@colinhowe
Copy link
Contributor Author

Failed linting. I'm happy to fix that up if this PR is of interest.

@DanielRuf DanielRuf closed this Mar 27, 2019
@DanielRuf
Copy link
Collaborator

DanielRuf commented Mar 27, 2019

Hi @colinhowe,

Thanks for the PR but this is already fixed in 3.6.1.

https://bugs.chromium.org/p/chromium/issues/detail?id=941910 is the issue which documents the cause.

Also your change does not support touch(start/end) and focus.

This is a regression bug in Chromium 73.

@DanielRuf DanielRuf reopened this Mar 27, 2019
@DanielRuf
Copy link
Collaborator

I'm happy to fix that up if this PR is of interest.

Hi, yes please fix this. Did you also test with touch emulation using the developer tools in Chrome? What about tabbing / focusing into the element?

@colinhowe
Copy link
Contributor Author

I haven't yet tested the touch emulation. I'll do that in the morning. In theory this bug shouldn't impact touch. Yet theory often fails to stand up to the practical.

I did give tabbing / focusing in a quick whirl, but, I'll give it a more thorough testing if you're happy with the approach in this fix.

Chrome 73 changes the behaviour of click events when the mousedown is on one element
and the mouseup is on another. The previous behaviour was to not emit any click event
at all. The new behaviour is to emit a click event with the target being the
common ancestor of the elements involved.

In the case of pickadate the calendar is popped up over the input element
meaning that the mousedown event targest the input but the mouseup targest
the calendar itself. This results in the click event targetting the common
ancestor which is not particularly useful.

The workaround here is to track the mousedown and mouseup events during
calendar opening and ignore any click events generated during this.

See https://bugs.chromium.org/p/chromium/issues/detail?id=946459&can=2&q=click
for additional details.
@colinhowe
Copy link
Contributor Author

Fixed up the change to pass linting.
Tested touch events - this change didn't impact them at all.
Tabbing and focus events work as expected.
Added a test for the new behaviour.

@DanielRuf
Copy link
Collaborator

@amsul

Copy link
Owner

@amsul amsul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colinhowe this looks great. Thanks! 🙇

@amsul amsul merged commit 6a469ea into amsul:master Apr 2, 2019
@colinhowe
Copy link
Contributor Author

👍 Thank you for merging!

Copy link
Collaborator

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next time please use a (feature) branch (instead of using the master branch) ;-)

@colinhowe
Copy link
Contributor Author

Will a new minor release be coming with these changes? Would love to get our code back onto mainline :)

@amsul
Copy link
Owner

amsul commented Apr 3, 2019

@colinhowe on it

@amsul
Copy link
Owner

amsul commented Apr 3, 2019

@colinhowe https://github.com/amsul/pickadate.js/releases/tag/3.6.3 🙂

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

Successfully merging this pull request may close these issues.

3 participants