-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
Add support for auxclick event #11571
Conversation
Good question... How much do we want React to polyfill browser behavior? How do we decide what to polyfill? |
Historically, we've avoided adding events that we couldn't polyfill, so IMO we definitely need to include this polyfill and make sure this works in unsupported browsers like IE. |
A complicating factor with this event is that it was added along a side a change to click events, where they no longer fire for the middle mouse button. So even if it wasn't polyfillable i think there'd be a stronger case to add it (or use it to "fix" the click event) |
nativeEvent: MouseEvent, | ||
nativeEventTarget: EventTarget, | ||
) { | ||
if (topLevelType === 'topClick' && nativeEvent.button === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is button not 0? Does this account for right clicks? Is that even a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is any click that isn't a left click yeah. which is correct going off the mdn example https://mdn.github.io/dom-examples/auxclick/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having a fixtures to test this in browsers that don't have auxclick
?
probably not worth the fixture? I'm not sure what it'd test that the unit test doesn't cover? |
one open question here: should the "polyfill" do something more advanced in the case where auxclick is support to prevent a case where click and auxclick both fire? I don't know if that will be the case ever, right now FF and Chrome both don't fire click anymore for none left clicks so it isn't an issue |
I'm very hesitant about adding more event polyfills given the focus on the bundle size. |
Minimimum is to add it the SimpleEventPlugin, and let the user handle the polyfill, e.g. listen for both |
That sounds okay as the first step to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with just passing it through
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this could be moved through really quickly with a few changes. @jquense I'd be happy to pick this up tomorrow if you'd like.
@@ -136,7 +137,7 @@ const topLevelEventsToDispatchConfig: { | |||
}); | |||
|
|||
// Only used in DEV for exhaustiveness validation. | |||
const knownHTMLTopLevelTypes = [ | |||
let knownHTMLTopLevelTypes = __DEV__ && [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be reverted?
@@ -256,7 +259,7 @@ const SimpleEventPlugin: PluginModule<MouseEvent> = { | |||
break; | |||
default: | |||
if (__DEV__) { | |||
if (knownHTMLTopLevelTypes.indexOf(topLevelType) === -1) { | |||
if ((knownHTMLTopLevelTypes: any).indexOf(topLevelType) === -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too?
@@ -217,6 +219,7 @@ const SimpleEventPlugin: PluginModule<MouseEvent> = { | |||
case 'topMouseOut': | |||
case 'topMouseOver': | |||
case 'topContextMenu': | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you nix this space?
Upstreamed this with master. @gaearon do you mind taking a second look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good if it works in manual testing
@gaearon pushed a commit after some local testing. Just had to add aux click to the list of interactive event types (great change, @philipp-spiess btw) |
Waiting on CI for good measure, then I'll merge. |
Need to update snapshots. |
Huzzah. |
All set. Thanks @jquense! |
thanks for cleaning this up @nhunzaker ! |
…event support, new in v16.5 - facebook/react#11571 adds auxClick - facebook/react#13571 for v16.5
Adds support for
auxclick
. The little polyfill may be overkill, but it was simple enough to do so i put it in. Do we want to handle this or leave it up to users and add auxclick to the SimpleEvent plugin?closes #8529