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

Move Mousewheel Event Type Detection to BrowserConstants (#11585) #11594

Conversation

cristidrg
Copy link
Contributor

Before submitting a pull request, please make sure the following is done:

  1. Fork the repository and create your branch from master.
  2. Run yarn in the repository root.
  3. If you've fixed a bug or added code that should be tested, add tests!
  4. Ensure the test suite passes (yarn test). Tip: yarn test --watch TestName is helpful in development.
  5. Format your code with prettier (yarn prettier).
  6. Make sure your code lints (yarn lint). Tip: yarn linc to only check changed files.
  7. Run the Flow typechecks (yarn flow).
  8. If you haven't already, complete the CLA.

Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html

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.

This is great, I just realized we can be a little more aggressive with removing this compatibility code than I originally thought.

Let's go ahead and drop the fallback checks and just leave the code removal in ReactBrowserEventEmitter.

Thank you for sending this out!

topWheel: 'wheel',
topWheel: isEventSupported('wheel')
? 'wheel'
: isEventSupported('mousewheel') ? 'mousewheel' : 'DOMMouseScroll',
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some quick checks. We have DOMMouseScroll for Firefox, but MDN claims that wheel is supported as of Firefox 17.0. We support the Firefox Extended Support Release (ESR), which is presently at 52.5.0. I think this is a safe change that will not affect any user.

Also it also looks like IE9 supports wheel (though it's not on the window according to MDN, the event is still supported).

So we can drop this check all-together. That would mean we'd revert the changes to this file and just keep the code removal in ReactBrowserEventEmitter.

@aweary, @jquense, or @gaearon any concerns with sending this out in a patch release?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind if your test it in the minimal supported browsers.

// @see http://www.quirksmode.org/dom/events/tests/scroll.html
trapBubbledEvent('topWheel', 'DOMMouseScroll', mountAt);
}
} else if (dependency === 'topScroll') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

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.

Tested in:

  • Firefox 47, 52, 56
  • Chrome 47, 62
  • Safari 7.1, 11
  • IE 9, 10, 11
  • Edge 14, 16
  • Opera 18, 49

I tested with a hardware mouse wheel and gesture scrolling on track pads.

Thanks, @cristidrg!

@nhunzaker nhunzaker merged commit bd9dbd5 into facebook:master Nov 20, 2017
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.

4 participants