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

Skip intercepting non-left button clicks on links #1908

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

derrickreimer
Copy link
Contributor

@derrickreimer derrickreimer commented Jun 24, 2024

Fixes #1772 #1329 #1679

All major browsers except Safari trigger the auxclick instead of click when the middle button is clicked on a mouse. WebKit has a long-standing bug that does not adhere to this behavior, so Safari dispatches a plain old click event instead. But alas, we can key off of the button property on MouseEvent to detect if this is an auxiliary click, and skip intercepting in that case.

I had to tweak the TypeScript types here to handle receiving a MouseEvent and discriminate accordingly.

@derrickreimer derrickreimer changed the title Skip intercepting auxiliary (middle) clicks on links Skip intercepting non-left button clicks on links Jun 25, 2024
@reinink reinink merged commit 6012330 into master Jun 25, 2024
7 checks passed
@reinink reinink deleted the skip-intercepting-aux-clicks branch June 25, 2024 16:26
@reinink
Copy link
Member

reinink commented Jun 25, 2024

Thanks @derrickreimer! 🙌

@shengslogar
Copy link

Nice! I'm a bit surprised to see #1679 and this fix applied to both the Vue and React wrappers. I'm using @inertiajs/vue3 1.1.0 in my own project and just confirmed middle click is working prior to this patch. 🧐

In fact, native <a> tags in both React (demo) and Vue.js (demo) don't even trigger a click event when middle clicked, defaulting to the browser opening the href in a new tab. So now I'm not quite sure how this was an issue to begin with?

@derrickreimer
Copy link
Contributor Author

Interesting! I am seeing the onclick handler being triggered by middle click in both the React & Vue demos in Safari (17.5).

CleanShot 2024-06-25 at 19 34 59

I'm not really sure if the way click triggering has changed in Vue's internals over time, but at any rate, this fix should cover off Inertia intercepting any aux clicks that may make there way into the onclick handler.

@shengslogar
Copy link

shengslogar commented Jun 26, 2024

After some debugging, it turns out the original problem comes down to the event.which > 1 statement of this line, and is specific to the React wrapper only:

(isLink && event.which > 1) ||

For regular mouse events, event.which === 2 for middle clicks, and 1 for left clicks, both in Vue and React.

However, React events are "synthetic events" and don't provide the which property, so it's always undefined (https://react.dev/reference/react-dom/components/common#react-event-object-properties). (That also means the TypeScript type here of MouseEvent | KeyboardEvent, as-is, is technically incorrect for the React wrapper.)

Screenshot of console-logged React event, with "nativeEvent" and "which: 2" highlighted
Updated React demo, updated Vue demo

If we revert this PR and update the following line to pass event.nativeEvent, all should work as originally intended:

if (shouldIntercept(event)) {

It's still unclear to me what issue was behind #1679. which in Vue 3.3.4 behaves the same as described above, and shouldIntercept hasn't changed in the last few years. Apart from asking the original issue owner for a repro, I would be willing to write that specific case off (e.g., perhaps they were defining <Link as="button" />, which would render a <button> tag and cause Inertia to rightfully always intercept).

@shengslogar
Copy link

It is also worth mentioning that all of this is still specific to Safari, as you rightfully pointed out in your last comment. A click event, and subsequently shouldIntercept, is NEVER called when middle clicking links in Chrome and Firefox.

@derrickreimer
Copy link
Contributor Author

Ah nice investigative work! Yep, updating the event we pass to shouldIntercept makes sense to me. Seems as though the now-deprecated which check could be removed as well?

@shengslogar
Copy link

Oh, good call! I didn't realize which was deprecated. In that case, your use of button should be a drop-in replacement. 👍

I also like your explicit use of !== 0, since it's unclear to me when some of the other options (browser back/forward?) might be triggered.

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.

Middle mouse button click does not open link in new tab when using Safari
4 participants