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

[react-interactions] additional check to avoid middle clicks triggering press #17367

Closed
wants to merge 2 commits into from

Conversation

Ambroos
Copy link

@Ambroos Ambroos commented Nov 14, 2019

Middle clicks in Chrome/Firefox on Mac using a touchpad + BetterTouchTool tap gesture trigger a press, causing navigation when this happens on links (in addition to opening links in new tabs).

T57092269 contains more information and videos of fix.

I'm not 100% sure if my edits to the comment make sense.

Looking at the native event for pointer up and down, buttons === 0 and button === 1 when this happens. We covered for buttons === 4 in pointer up, but it looks like these types of middle clicks look slightly different. According to the pointer events spec, button === 1 means it's a middle click, so we added this quick check to avoid triggering press on middle clicks.

I'd be happy to make more changes. Before this the intention was to keep middle clicks around and check for buttons === 4 in pointer up to discard presses there. An alternative to this change could be checking button there instead (as button is still 1 during pointer up). Or, we ignore button === 1 and buttons >= 4 in pointer down and can then remove the 4 check in pointer up completely.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 14, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7b21a1b:

Sandbox Source
staging-morning-9hhfb Configuration

@sizebot
Copy link

sizebot commented Nov 14, 2019

Warnings
⚠️ Base commit is broken: df8db4e

Size changes (stable)

Generated by 🚫 dangerJS against 7b21a1b

@sizebot
Copy link

sizebot commented Nov 14, 2019

Warnings
⚠️ Base commit is broken: df8db4e

Size changes (experimental)

Generated by 🚫 dangerJS against 7b21a1b

@Ambroos Ambroos changed the title Add additional check to avoid middle clicks triggering press [react-interactions] additional check to avoid middle clicks triggering press Nov 14, 2019
@trueadm trueadm requested a review from necolas November 14, 2019 15:45
@trueadm
Copy link
Contributor

trueadm commented Nov 14, 2019

The logic here makes sense but I'd like to get @necolas's eyes on this too as he has more context on the middle click behavior given the last PR – #16546.

@necolas
Copy link
Contributor

necolas commented Nov 14, 2019

Pretty sure parts of the app rely on middle click triggering pressstart and they can check the value of buttons to exclude middle clicks.

It might be that no unit tests failed here because we ignore button in the mock events in the tests. If the spec for this responder is going to change you need to add unit tests and make sure nothing will break internally

@trueadm
Copy link
Contributor

trueadm commented Nov 14, 2019

@Ambroos can you screenshot what you get with this sandbox, to eliminate all other possible causes for this issue:

https://codesandbox.io/s/strange-mcclintock-ttc61

Thinking about it more, it's maybe best to move this logic to the point (if it turns out that buttons isn't always reliable to use for middle clicks) where we check for buttons === 4 before we trigger onPress because, as @necolas said, we do want the pressStart to fire for middle-clicks and depend on this internally for various tools.

@Ambroos
Copy link
Author

Ambroos commented Nov 14, 2019

Sandbox console output after middle clicking, both in Chrome, Firefox and Safari:

button 1
buttons 0

This is where I noticed the bug originally. Touchpad-based middle clicks in macOS (triggered by BetterTouchTool) show up in all browsers as button: 1 / buttons: 0.

Browsers deal with these middle-clicks correctly, they open <a>s in new tabs and don't trigger click events. Because there are no actual mouse buttons being pressed, it looks like buttons remains 0, but it's still a middle click. I'm pretty sure we'll have to check both button and buttons.

@trueadm
Copy link
Contributor

trueadm commented Nov 14, 2019

@Ambroos This is only for BetterTouchTool still? Didn't you mention in our earlier discussion that you were able to repro with a hardware mouse too? Mind trying that again on the sandbox? It would be good to find the cause – browser, system setting, software, hardware etc

Maybe buttons is related to hardware data, but button is related to software/browser data?

@necolas
Copy link
Contributor

necolas commented Nov 14, 2019

How do you use the "touchpad + BetterTouchTool tap gesture"? As we should probably record a fixture of the event to unit test this scenario

@Ambroos
Copy link
Author

Ambroos commented Nov 14, 2019

  1. Open BetterTouchTool (there's a free trial) configuration
  2. For (magic) mouse / trackpad, add a new trigger under all apps (any trigger, I use 2-finger gesture TipTap left/right)
  3. Assign action middle click to the trigger.
  4. Use your newly configured gesture in any browser.

BetterTouchTools is pretty popular, probably the most popular magic mouse / trackpad enhancement app for macOS.

I'll test again with a hardware mouse later today.

@Ambroos
Copy link
Author

Ambroos commented Nov 14, 2019

I've updated the pull request, this approach should be less invasive but still accomplishes the same.

necolas added a commit to necolas/react that referenced this pull request Dec 9, 2019
Tools like BetterTouchTool for macOS trigger middle-clicks with a 'buttons'
value that doesn't correspond to the middle-mouse button. To account for this
we also inspect the value of 'button'.

Close facebook#17367
necolas added a commit to necolas/react that referenced this pull request Dec 9, 2019
Tools like BetterTouchTool for macOS trigger middle-clicks with a 'buttons'
value that doesn't correspond to the middle-mouse button. To account for this
we also inspect the value of 'button'.

Close facebook#17367
necolas added a commit that referenced this pull request Dec 9, 2019
…#17554)

Tools like BetterTouchTool for macOS trigger middle-clicks with a 'buttons'
value that doesn't correspond to the middle-mouse button. To account for this
we also inspect the value of 'button'.

Close #17367
@Ambroos Ambroos deleted the middle-click-fix branch December 9, 2019 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants