-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018
Conversation
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. |
hmm, already registered but it was with a lower case 'j'. Hope someone can change that |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
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! |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
var previousScreenX = MouseMetrics.previousScreenX; | ||
MouseMetrics.setPreviousScreenX(event.screenX); | ||
return (previousScreenX) ? event.screenX - previousScreenX : 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you drop the extra new line here?
Could we get rid of movementY: function(event) {
if ('movementY' in event) {
return event.movementY;
}
var screenY = previousScreenY;
previousScreenY = event.screenY;
return screenY ? event.screenY - screenY : 0;
}, |
Just checking in here. It's been a while, I apologize for that, @jasonwilliams! It seems like the next steps would be:
@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. |
Sure, ill take a look, |
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! |
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.3% Details of bundled changes.Comparing: c78957e...2cff055 react-dom
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
} | ||
|
||
const screenX = previousScreenX; | ||
previousScreenX = event.ScreenX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be event.screenX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, yes it should
Is it global, or just local to that module? if so, would that matter? |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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 ⛵️. |
Added a quick fixture for mouse movement for good measure. Thanks, @jasonwilliams! |
|
||
const screenX = previousScreenX; | ||
previousScreenX = event.screenX; | ||
return screenX ? event.screenX - screenX : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's equal to 0
then it will still return 0
since that's the second expression in the ternary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if saved screenX
is 0
, but then you moved to 200
, you want to return 200 - 0
rather than 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Dan is right, and we need to correct this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/tc39/proposal-nullish-coalescing would have helped a lot with this, yes we should check if equal to null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, are we happy to assume screenX on event? If undefined we could end up with undefined - null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
|
||
const screenY = previousScreenY; | ||
previousScreenY = event.screenY; | ||
return screenY ? event.screenY - screenY : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
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