inertiajs / inertia

Inertia.js lets you quickly build modern single-page React, Vue and Svelte apps using classic server-side routing and controllers.
https://inertiajs.com
MIT License
6.02k stars 405 forks source link

Skip intercepting non-left button clicks on links #1908

Closed derrickreimer closed 1 week ago

derrickreimer commented 1 week ago

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.

reinink commented 1 week ago

Thanks @derrickreimer! 🙌

shengslogar commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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:

https://github.com/inertiajs/inertia/blob/7526daa664ab23dcb75fda410a76721043c9798d/packages/core/src/shouldIntercept.ts#L6

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:

https://github.com/inertiajs/inertia/blob/3a63529262288d9b3079360b67be50d7ef3b5d27/packages/react/src/Link.ts#L73

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.