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

Add support for auxclick event #11571

Merged
merged 4 commits into from Aug 3, 2018
Merged

Add support for auxclick event #11571

merged 4 commits into from Aug 3, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Nov 16, 2017

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

@nhunzaker
Copy link
Contributor

Good question... How much do we want React to polyfill browser behavior? How do we decide what to polyfill?

@aweary
Copy link
Contributor

aweary commented Nov 16, 2017

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.

@jquense
Copy link
Contributor Author

jquense commented Nov 16, 2017

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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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/

Copy link
Contributor

@nhunzaker nhunzaker left a 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?

@jquense
Copy link
Contributor Author

jquense commented Nov 22, 2017

probably not worth the fixture? I'm not sure what it'd test that the unit test doesn't cover?

@jquense
Copy link
Contributor Author

jquense commented Nov 22, 2017

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

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

I'm very hesitant about adding more event polyfills given the focus on the bundle size.
What's the minimal constrained version we could do that would work only on supported browsers?

@jquense
Copy link
Contributor Author

jquense commented Jan 5, 2018

Minimimum is to add it the SimpleEventPlugin, and let the user handle the polyfill, e.g. listen for both click and auxclick

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

That sounds okay as the first step to me.

Copy link
Collaborator

@gaearon gaearon left a 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

Copy link
Contributor

@nhunzaker nhunzaker left a 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__ && [
Copy link
Contributor

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) {
Copy link
Contributor

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':

Copy link
Contributor

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?

@nhunzaker
Copy link
Contributor

Upstreamed this with master. @gaearon do you mind taking a second look?

Copy link
Collaborator

@gaearon gaearon left a 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

@nhunzaker
Copy link
Contributor

@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)

@nhunzaker
Copy link
Contributor

Waiting on CI for good measure, then I'll merge.

@gaearon
Copy link
Collaborator

gaearon commented Aug 3, 2018

Need to update snapshots.

@nhunzaker
Copy link
Contributor

Huzzah.

@nhunzaker
Copy link
Contributor

All set. Thanks @jquense!

@jquense
Copy link
Contributor Author

jquense commented Aug 3, 2018

thanks for cleaning this up @nhunzaker !

@jquense jquense deleted the auxclick branch August 3, 2018 20:02
@gaearon gaearon mentioned this pull request Sep 5, 2018
ljharb added a commit to enzymejs/enzyme that referenced this pull request Sep 5, 2018
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.

Feature Request: Support auxclick event (onAuxClick)
5 participants