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.

Sign up for GitHub

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

Jump to bottom

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

Merged
merged 3 commits into from
Jun 15, 2018
+89 −0
Merged

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b2676b
adding movementX and movementY into syntheticMouseEvent
Jun 11, 2018
51bc415
fixing case mistake
jasonwilliams Jun 11, 2018
2cff055
Add test fixture for movementX/Y fields
nhunzaker Jun 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/react-dom/src/events/SyntheticMouseEvent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
import SyntheticUIEvent from './SyntheticUIEvent';
import getEventModifierState from './getEventModifierState';

let previousScreenX = null;
let previousScreenY = null;

/**
* @interface MouseEvent
* @see http://www.w3.org/TR/DOM-Level-3-Events/
Expand All @@ -34,6 +37,24 @@ const SyntheticMouseEvent = SyntheticUIEvent.extend({
: event.fromElement)
);
},
movementX: function(event) {
if ('movementX' in event) {
return event.movementX;
}

const screenX = previousScreenX;
previousScreenX = event.ScreenX;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yes it should

return screenX ? event.screenX - screenX : 0;
Copy link
Collaborator

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.

Copy link
Contributor

@aweary aweary Jun 15, 2018 •
edited
Loading

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.

Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, screenX/Y are very well supported.

Copy link
Contributor Author

@jasonwilliams jasonwilliams Jun 20, 2018 •
edited
Loading

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 .

Copy link
Collaborator

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).

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

const screenY = previousScreenY;
previousScreenY = event.screenY;
return screenY ? event.screenY - screenY : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

},
});

export default SyntheticMouseEvent;