Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

jasonwilliams · 2017-02-16T23:45:02Z

Added movementX and movementY feature in accordance to https://w3c.github.io/pointerlock/#widl-MouseEvent-movementX
MouseEvent reference updated

This will now give full support for movementX and movementY including for browsers which don't support these events.
Not sure how to test this so any help is welcome

facebook-github-bot · 2017-02-16T23:45:05Z

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 - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

jasonwilliams · 2017-02-16T23:46:14Z

hmm, already registered but it was with a lower case 'j'. Hope someone can change that

facebook-github-bot · 2017-02-16T23:48:42Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

gaearon · 2017-04-21T14:40:08Z

Hey, sorry for no review. 😞

We have a large backlog of existing PRs and can’t dedicate enough time to the DOM right now because we’re finishing up the core rewrite.

We’ll try to get look at this when there is some time available!

aweary · 2017-04-21T14:44:23Z

I had a PR for this a long time ago at #6727 with the same implementation. I closed it because it also never got reviewed 😄 For what its worth, I think this is a ~reasonable way to support this feature.

nhunzaker

This seems legit to me. According to MDN, this is definitely the way to calculate it:

currentEvent.movementX = currentEvent.screenX - previousEvent.screenX.

https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/movementX

Just left a quick formatting request. Could you also run:

yarn prettier

This will format the project according to React's coding conventions (though it might not take out that extra new line).

nhunzaker · 2018-01-11T01:19:02Z

src/renderers/dom/shared/syntheticEvents/SyntheticMouseEvent.js

+    var previousScreenX = MouseMetrics.previousScreenX;
+    MouseMetrics.setPreviousScreenX(event.screenX);
+    return (previousScreenX) ? event.screenX - previousScreenX : 0;
+


Could you drop the extra new line here?

aweary · 2018-01-11T01:39:29Z

Could we get rid of MouseMetrics and just inline the logic into SyntheticMouseEvent? We could just use two local variables previousScreenY and previousScreenX and avoid the indirection of tracking it as an object property and updating it with a function call.

movementY: function(event) {
  if ('movementY' in event) {
     return event.movementY;
  }

 var screenY = previousScreenY;
 previousScreenY = event.screenY;
 return screenY ? event.screenY - screenY : 0;
 },

nhunzaker · 2018-06-08T11:48:13Z

Just checking in here. It's been a while, I apologize for that, @jasonwilliams!

It seems like the next steps would be:

Upstream this with master
Inline the MouseMetrics code into movementX and movementY ( we removed MouseMetrics)

@aweary do you still have interest on getting this in too? There's been a significant enough amount of changes to the repo that I wonder if we should help out on a rebase.

jasonwilliams · 2018-06-08T11:55:09Z

Sure, ill take a look,
ill start off with a rebase

aweary · 2018-06-08T23:46:16Z

@aweary do you still have interest on getting this in too?

Sure, might as well! It's not a frequently requested feature, but it's a small enough change that we might as well include it.

@jasonwilliams happy to help with any rebase issues you encounter!

pull-bot · 2018-06-11T13:13:17Z

ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.3%

Details of bundled changes.

Comparing: c78957e...2cff055

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	626.65 KB	627.15 KB	145.88 KB	145.99 KB	UMD_DEV
react-dom.production.min.js	🔺+0.2%	🔺+0.3%	94.25 KB	94.47 KB	30.52 KB	30.6 KB	UMD_PROD
react-dom.development.js	+0.1%	+0.1%	611.01 KB	611.51 KB	141.88 KB	141.98 KB	NODE_DEV
react-dom.production.min.js	🔺+0.2%	🔺+0.2%	92.75 KB	92.97 KB	29.51 KB	29.58 KB	NODE_PROD
ReactDOM-dev.js	+0.1%	+0.1%	620.2 KB	620.7 KB	141.22 KB	141.33 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.2%	🔺+0.3%	269.15 KB	269.64 KB	50.57 KB	50.71 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

nhunzaker

Just left a final request about event.ScreenX.

Otherwise, I think this looks good.

Is there any concern about saving the prior screenX/screenY globally? Like if React is used with shadow DOM - does that matter?

I don't want this to become too much of a rabbit hole. I'll block out some time later today to look into this and do some quick testing.

Thanks for updating this, and for sticking with us!

nhunzaker · 2018-06-11T16:42:59Z

packages/react-dom/src/events/SyntheticMouseEvent.js

+    }
+
+    const screenX = previousScreenX;
+    previousScreenX = event.ScreenX;


Should this be event.screenX?

oops, yes it should

jasonwilliams · 2018-06-11T20:37:08Z

Is there any concern about saving the prior screenX/screenY globally?

Is it global, or just local to that module? if so, would that matter?

nhunzaker · 2018-06-11T22:00:14Z

Ah, by globally I meant like in the same browser window. I wanted to make sure that there wasn't anything odd about custom elements, which have caused some issues in the past because they get put into an island.

But I don't think that's a problem here. Take a look at the following Codepen:

https://codepen.io/nhunzaker/pen/xzqeXp

Here, I created a custom component and added a mouse-move listener to it so that I could make sure that the parent element (the body of the page) had the same values as shadow DOM:

I wanted to make sure we didn't have to track this per document root. Looks like we're in the clear!

nhunzaker

This looks good on my end. @aweary or @jquense would you be willing to take a second look at this?

nhunzaker · 2018-06-12T18:07:50Z

Did some additional testing. Judging from MDN, movementX/Y isn't supported in Safari or IE.

Comparing the values between FF/Chrome and Safari/IE. This checks out. I've tested specifically in:

Chrome 66
Firefox 60
IE9, IE10, IE11
Safari 7.1, 11.1

Looks like all values consistently round to integers in both the native and synthetic implementation as well.

@jasonwilliams excellent work! I'll leave this out a bit longer to give others a chance to weigh in, but this is good to 🚢 in my ⛵️.

nhunzaker · 2018-06-15T13:10:13Z

Added a quick fixture for mouse movement for good measure. Thanks, @jasonwilliams!

gaearon · 2018-06-15T22:33:45Z

packages/react-dom/src/events/SyntheticMouseEvent.js

+
+    const screenX = previousScreenX;
+    previousScreenX = event.screenX;
+    return screenX ? event.screenX - screenX : 0;


I think the check here intends to check against null but will accidentally also be falsy if screenX is actually equal to 0. Please avoid truthy checks like this, they tend to hide bugs.

If it's equal to 0 then it will still return 0 since that's the second expression in the ternary.

I think that if saved screenX is 0, but then you moved to 200, you want to return 200 - 0 rather than 0.

I believe Dan is right, and we need to correct this.

https://github.com/tc39/proposal-nullish-coalescing would have helped a lot with this, yes we should check if equal to null

Another thing is that using the same variable for both null and numeric values can mess with VM optimizations. I think it would be better to use a separate boolean flag here.

Also, are we happy to assume screenX on event? If undefined we could end up with undefined - null

Yep, screenX/Y are very well supported.

Ok so if screenX will always be an Number, could we not just initiate previousScreenX as 0 Instead of null, and then instead of a ternary or null checking, always perform the minus operation. Because we can guarantee both operands will be Numbers by this point .

Wouldn't make the first movementX equal to the current screen coordinate, instead of 0? I think 0 would be expected (because the cursor didn't "jump" from top left point).

gaearon · 2018-06-15T22:34:00Z

packages/react-dom/src/events/SyntheticMouseEvent.js

+
+    const screenY = previousScreenY;
+    previousScreenY = event.screenY;
+    return screenY ? event.screenY - screenY : 0;


facebook-github-bot added the GH Review: review-needed label Feb 16, 2017

jasonwilliams changed the title ~~Adding movementX and movementY to synthenticMouseEvent~~ Adding movementX and movementY to synthenticMouseEvent fixes #6723 Feb 16, 2017

jasonwilliams mentioned this pull request Feb 16, 2017

support for event.movementX/Y [feature-request] #6723

Closed

facebook-github-bot added the CLA Signed label Feb 16, 2017

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

nhunzaker suggested changes Jan 11, 2018

View reviewed changes

jasonwilliams force-pushed the movementx branch from bdeee63 to 31cac0e Compare June 11, 2018 13:01

adding movementX and movementY into syntheticMouseEvent

5b2676b

jasonwilliams force-pushed the movementx branch from 31cac0e to 5b2676b Compare June 11, 2018 13:05

nhunzaker approved these changes Jun 11, 2018

View reviewed changes

nhunzaker suggested changes Jun 11, 2018

View reviewed changes

fixing case mistake

51bc415

nhunzaker approved these changes Jun 11, 2018

View reviewed changes

aweary approved these changes Jun 14, 2018

View reviewed changes

Add test fixture for movementX/Y fields

2cff055

nhunzaker merged commit 64c54ed into facebook:master Jun 15, 2018

gaearon reviewed Jun 15, 2018

View reviewed changes

jasonwilliams mentioned this pull request Jun 20, 2018

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

Merged

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

ghost mentioned this pull request Dec 12, 2018

Add movementX and movementY to MouseEvent DefinitelyTyped/DefinitelyTyped#31297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

jasonwilliams commented Feb 16, 2017 •

edited

facebook-github-bot commented Feb 16, 2017

jasonwilliams commented Feb 16, 2017

facebook-github-bot commented Feb 16, 2017

gaearon commented Apr 21, 2017

aweary commented Apr 21, 2017

nhunzaker left a comment

nhunzaker Jan 11, 2018

aweary commented Jan 11, 2018

nhunzaker commented Jun 8, 2018 •

edited

jasonwilliams commented Jun 8, 2018 •

edited

aweary commented Jun 8, 2018

pull-bot commented Jun 11, 2018 •

edited

react-dom

nhunzaker left a comment •

edited

nhunzaker Jun 11, 2018

jasonwilliams Jun 11, 2018

jasonwilliams commented Jun 11, 2018 •

edited

nhunzaker commented Jun 11, 2018 •

edited

nhunzaker left a comment

nhunzaker commented Jun 12, 2018 •

edited

nhunzaker commented Jun 15, 2018

gaearon Jun 15, 2018

aweary Jun 15, 2018 •

edited

gaearon Jun 15, 2018

nhunzaker Jun 15, 2018

jasonwilliams Jun 15, 2018

gaearon Jun 15, 2018

jasonwilliams Jun 15, 2018

nhunzaker Jun 16, 2018

jasonwilliams Jun 20, 2018 •

edited

gaearon Jun 20, 2018

gaearon Jun 15, 2018

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

Conversation

jasonwilliams commented Feb 16, 2017 • edited

facebook-github-bot commented Feb 16, 2017

jasonwilliams commented Feb 16, 2017

facebook-github-bot commented Feb 16, 2017

gaearon commented Apr 21, 2017

aweary commented Apr 21, 2017

nhunzaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Jan 11, 2018

nhunzaker commented Jun 8, 2018 • edited

jasonwilliams commented Jun 8, 2018 • edited

aweary commented Jun 8, 2018

pull-bot commented Jun 11, 2018 • edited

react-dom

nhunzaker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonwilliams commented Jun 11, 2018 • edited

nhunzaker commented Jun 11, 2018 • edited

nhunzaker left a comment

Choose a reason for hiding this comment

nhunzaker commented Jun 12, 2018 • edited

nhunzaker commented Jun 15, 2018

Choose a reason for hiding this comment

aweary Jun 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonwilliams Jun 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonwilliams commented Feb 16, 2017 •

edited

nhunzaker commented Jun 8, 2018 •

edited

jasonwilliams commented Jun 8, 2018 •

edited

pull-bot commented Jun 11, 2018 •

edited

nhunzaker left a comment •

edited

jasonwilliams commented Jun 11, 2018 •

edited

nhunzaker commented Jun 11, 2018 •

edited

nhunzaker commented Jun 12, 2018 •

edited

aweary Jun 15, 2018 •

edited

jasonwilliams Jun 20, 2018 •

edited