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

setting a flag, so that the first movement will have the correct value #13082

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

jasonwilliams
Copy link
Contributor

This should hopefully fix the initial movementX and movementY values introduced in #9018

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@gaearon
Copy link
Collaborator

gaearon commented Jun 20, 2018

Out of curiosity, can you make some browser testing and see what browser gives us for the first event, when this property is supported natively?

@nhunzaker
Copy link
Contributor

nhunzaker commented Jun 20, 2018

As far as I can tell, the initial movement is always 0, using this method:

  1. Open the /mouse-events fixture
  2. Refresh the page, and immediately alt+tab to another app
  3. Hover over the fixture
  4. Tab back to the browser
  5. Move your mouse

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.

As far as I can tell, this looks correct. However I will not be able to test in IE until tomorrow morning (Airplane wifi :()

@nhunzaker
Copy link
Contributor

This does, however, appear to work correctly in Safari 11.1 (Safari is the other browser that does not support movementX/Y)

@gaearon gaearon merged commit 5b3d17a into facebook:master Jun 20, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jun 20, 2018

LGTM

@jasonwilliams
Copy link
Contributor Author

jasonwilliams commented Jun 20, 2018

@gaearon @nhunzaker
I've tested Chrome, Firefox and Edge, by using the mouse-event fixture, setting a breakpoint over https://github.com/facebook/react/blob/master/packages/react-dom/src/events/SyntheticMouseEvent.js#L45 and then hovering over the page after page load.

Chrome 67.0.3396.87 i did quite a few times, and the first hover event always had movementX at 0
chromepageloadmovementx

Edge 42.17134.1.0 had very similar results after quite a few tries
edgepageloadmovementx

Firefox Developer Edition 61.0b14 seemed to be the outlier here, as i was getting random values each time, sometimes 13, or 18 or 12. This could be saying more about dev tools though, rather than the initial value of movementX
firefoxpageloadmovementx

@gaearon
Copy link
Collaborator

gaearon commented Jun 20, 2018

Nice! This is sufficient—I think 0 is the most reasonable initial value.

@aweary
Copy link
Contributor

aweary commented Jun 20, 2018

0 is the spec-defined initial value 👍

https://w3c.github.io/pointerlock/#extensions-to-the-mouseevent-interface

The un-initialized value of movementX/Y must be 0.

I also noticed that the spec states:

movementX/Y must be zero for all mouse events except mousemove.

It's potentially minor but we're not restricting our polyfill mousemove events, so it's slightly out of spec.

@gaearon
Copy link
Collaborator

gaearon commented Jun 20, 2018

It's potentially minor but we're not restricting our polyfill mousemove events, so it's slightly out of spec.

Oh, I missed that. We should definitely limit it to mousemove events. I don't think it's minor—if you rely on movementX/movementY then you'll potentially drop movements.

@aweary
Copy link
Contributor

aweary commented Jun 20, 2018

@jasonwilliams do you want to fix this in another follow up?

@jasonwilliams
Copy link
Contributor Author

Yup I missed that too, I can take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants