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

Disable ViewportMetrics unless MouseEvent lacks support for pageX/pageY #2271

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@syranide
Copy link
Contributor

commented Sep 29, 2014

For #1300, only IE8 lacks pageX/pageY (and perhaps really old FF or w/e I guess).

  • Test plan: not yet (want your input on the PR first)
  • If approved: will need to make a manual test and verify in all browsers

From my #1300 (comment):


https://github.com/facebook/react/blob/master/src/browser/ui/ReactEventListener.js#L65
https://github.com/facebook/react/blob/master/src/browser/ui/ReactEventListener.js#L165

That makes no sense at all to me, refresh does not take an argument, yet we get the scroll position and provide it as an argument. Instead, refresh gets the scroll position internally and uses that? So we call getUnboundedScrollPosition twice, but only use the result once (also, this should be broken for events inside iframes, for IE8).


https://github.com/facebook/react/blob/master/src/browser/eventPlugins/TapEventPlugin.js#L54
I'm pretty sure any browsers that supports touch also supports pageX/pageY... ?

if (!isMonitoringScrollValue) {
if (hasEventPageXY === undefined) {
hasEventPageXY =
document.createEvent && 'pageX' in document.createEvent('MouseEvent');

This comment has been minimized.

Copy link
@sophiebits

sophiebits Oct 3, 2014

Collaborator

I think @petehunt's plan was that we'd only access the DOM in files in src/browser/ui/ – but maybe ReactBrowserEventEmitter should be moved there?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2015

This seems pretty reasonable, though I know there's some internal FB code that uses ViewportMetrics that will need to be changed if we do this.

@nelix

This comment has been minimized.

Copy link

commented Nov 12, 2015

What if we instead of detecting modern browsers we used a feature flag? It seems kinda niche, unless you are doing stuff with scrolling/long lists.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Dec 16, 2015

@syranide This seems good – mind rebasing?

@gaearon

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

I rebased this in #6129.
Let’s continue the discussion there.
Thank you for the fix, @syranide!

@gaearon gaearon closed this Feb 26, 2016

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.