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

Prevent non-primary mouse button clicks from triggering click events #34573

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
6 participants
@WoH
Copy link
Contributor

WoH commented Nov 30, 2018

Summary

Firefox fires click events on left button, right button and scroll-wheel (any non-primary mouse key) clicks while other browsers don't.
This commit stops these firefox specific events from being handled like a primary click before other Rails UJS handlers start processing the event by checking which mouse key was used to trigger the event.

Other Information

Firefox fires click events even if the click originated from right-clicking or the scroll wheel.
See here:

A pointing device button (ANY button; soon to be primary button only) has been pressed and released on an element.

According to the Mozilla Docs, this should be safe for left and right handed mice.

The button number that was pressed when the mouse event was fired: Left button=0, middle button=1 (if present), right button=2. For mice configured for left handed use in which the button actions are reversed the values are instead read from right to left.

Related issue

#34541

@grant-roy

This comment has been minimized.

Copy link

grant-roy commented Nov 30, 2018

I am triggering a form submit with javascript after a button click, and

document.querySelector('form').dispatchEvent(new Event('submit'));

works perfectly fine with chrome and safari in terms of rails-ujs, but with firefox I get a full page request after the ajax request.

Will this also fix this issue?

@gmcgibbon
Copy link
Member

gmcgibbon left a comment

Nice job! Can you add a test here?

@gmcgibbon

This comment has been minimized.

Copy link
Member

gmcgibbon commented Nov 30, 2018

@grant-roy This doesn't fix that, but Rails.fire(form, 'submit'); should work. See #29546

@grant-roy

This comment has been minimized.

Copy link

grant-roy commented Nov 30, 2018

@gmcgibbon I've tried that in several different configurations, but I always get Rails not defined on the page. I've read through all the advice and issues with Webpack (which this issue seems to be related to) and nothing works. I've upgraded everything and still Rails is undefined clientside, even when I can see it in the on page js.

@gmcgibbon

This comment has been minimized.

Copy link
Member

gmcgibbon commented Nov 30, 2018

@grant-roy If you can reproduce it on a dummy rails app, feel free to open a separate issue. Maybe its a minification issue?

@WoH WoH force-pushed the WoH:firefox-right-click-bug branch 2 times, most recently from 19928d7 to 3913cad Dec 1, 2018

@WoH

This comment has been minimized.

Copy link
Contributor

WoH commented Dec 2, 2018

@gmcgibbon I added tests around click events on links and buttons.
If I should add more I'm happy to so, but I'd like to hear your feedback.

@gmcgibbon
Copy link
Member

gmcgibbon left a comment

Test coverage LGTM, but the UJS test suite appears to be failing on CI. Can you take a look and then squash your commits? Thanks! 😄

Show resolved Hide resolved actionview/CHANGELOG.md Outdated

@WoH WoH force-pushed the WoH:firefox-right-click-bug branch from 8113a0b to f3b5f59 Dec 3, 2018

@WoH

This comment has been minimized.

Copy link
Contributor

WoH commented Dec 3, 2018

CI fail was related to a timeout because the connection to the ujs server couldn't be established.

[Travis CI] actionview
Running command: bundle exec rake test:ujs
Timed out after 5 seconds

I am not sure why that would happen especially since I don't think editing the changelog affected that. 😆 The tests themselves are very similar to the ones checking the ctrl-click behavior, I doubt they introduced the issue. 😕 If I missed something I'd gladly accept hints.

@WoH WoH force-pushed the WoH:firefox-right-click-bug branch from f3b5f59 to fb5ac2b Dec 3, 2018

@rails-bot rails-bot bot added the activerecord label Dec 3, 2018

@WoH WoH force-pushed the WoH:firefox-right-click-bug branch from fb5ac2b to ef4ea9a Dec 3, 2018

@javan

This comment has been minimized.

Copy link
Member

javan commented Dec 3, 2018

The implementation here looks reasonable in that it follows existing conventions, but—pardon the following rant—they are not good conventions. For example, this should not be implemented as 7 separate event listeners:

delegate document, Rails.linkClickSelector, 'click', handleNonPrimaryClick
delegate document, Rails.linkClickSelector, 'click', handleDisabledElement
delegate document, Rails.linkClickSelector, 'click', handleConfirm
delegate document, Rails.linkClickSelector, 'click', handleMetaClick
delegate document, Rails.linkClickSelector, 'click', disableElement
delegate document, Rails.linkClickSelector, 'click', handleRemote
delegate document, Rails.linkClickSelector, 'click', handleMethod

And, "ignoring" an event should not cancel it or stop it from propagating:
Rails.handleMetaClick = (e) ->
link = this
method = (link.getAttribute('data-method') or 'GET').toUpperCase()
data = link.getAttribute('data-params')
metaClick = e.metaKey or e.ctrlKey
e.stopImmediatePropagation() if metaClick and method is 'GET' and not data

Instead, there should be a single listener for a given event type + selector that first tests the event for things like mouse button and meta key before acting on it. Here's a rough sketch using a helper borrowed from Turbolinks:

delegate document, Rails.linkClickSelector, 'click', (event) ->
  if clickEventIsSignificant(event)
    handleConfirm(event)
    handleRemote(event)
    handleMethod(event)

clickEventIsSignificant = (event) ->
  not (
    event.defaultPrevented or
    event.target.isContentEditable or
    event.which > 1 or
    event.altKey or
    event.ctrlKey or
    event.metaKey or
    event.shiftKey
  )

That 👆 probably won't work as-is, but I suspect it's not far off.

@javan

This comment has been minimized.

Copy link
Member

javan commented Dec 3, 2018

I'm gonna hand this one off to you, @guilleiguaran.

@javan javan assigned guilleiguaran and unassigned javan Dec 3, 2018

@WoH

This comment has been minimized.

Copy link
Contributor

WoH commented Dec 3, 2018

@javan Would you like to see that refactoring within this PR?

E: I'm open to adding this onto this PR, but I'd prefer to do that seperately.

@javan

This comment has been minimized.

Copy link
Member

javan commented Dec 4, 2018

@WoH, perhaps you could start by combining your handleNonPrimaryClick() handler with the existing handleMetaClick() (and renaming them appropriately). That way we're not adding yet another event listener.

@WoH WoH force-pushed the WoH:firefox-right-click-bug branch 2 times, most recently from a24d25c to e9ade80 Dec 5, 2018

Prevent unintended mouse keys from firing click events
Firefox fires click events on left-, right-
and scroll-wheel (any non-primary mouse key) clicks while other browsers don't.

@WoH WoH force-pushed the WoH:firefox-right-click-bug branch from e9ade80 to a261262 Dec 5, 2018

@WoH

This comment has been minimized.

Copy link
Contributor

WoH commented Dec 5, 2018

I incorporated feedback as far as the combining the two methods goes. Since we don't really handle anything except stopping propagation, using prevent in the method name sounded reasonable to me.
We can use insignificant as a combination of non-primary + some meta-clicks here aswell, so I 'borrowed' that name 😆

@rafaelfranca rafaelfranca merged commit b86f65a into rails:master Dec 5, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Dec 5, 2018

Merge pull request #34573 from WoH/firefox-right-click-bug
Prevent non-primary mouse button clicks from triggering click events

@WoH WoH deleted the WoH:firefox-right-click-bug branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment