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

When available, use the mouse event buttons property. #782

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Feb 27, 2018

Firefox erroneously sets the which value to the left button even when it is not held down. The buttons property is correct for browsers that support it, but Safari doesn't, so we have to fall back to using the which value in that case.

This fixes #781.

Firefox erroneously sets the which value to the left button even when it
is not held down.  The buttons property is correct for browsers that
support it, but Safari doesn't, so we have to fall back to using the
which value in that case.
@manthey manthey requested a review from brianhelba February 27, 2018 21:36
Copy link
Contributor

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

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

Code generally looks good. I'll test it in a bit, then I can approve.

/* When handling touch events, evt.which can be a string, in which case
* handle a "button" with that name -- a non-integer string will not
* evaluate as being between 1 and 3. */
if (evt.which && !(evt.which >= 1 && evt.which <= 3)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a separate conditional block, whereby one of the other previous conditionals runs with this too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't need to be, as in the touch events where it will get triggered, it can never also be in a mouseup event, and we want it to trigger regardless of the buttons conditional.

@manthey
Copy link
Contributor Author

manthey commented Feb 28, 2018

We can use the tutorials for showing off bugs as well as fixes. For instance: Tutorial link.

We should add a three-pane default-editable tutorial to facilitate this (rather then using the Basic Tutorial where some steps are ignored).

Copy link
Contributor

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

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

LGTM

@manthey manthey merged commit a54ed2b into master Feb 28, 2018
@manthey manthey deleted the use-buttons-property branch February 28, 2018 14:58
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.

On Firefox, mosemove events always indicate the left mouse button is clicked
2 participants