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

Adding movementX and movementY to synthenticMouseEvent fixes #6723 #9018

Merged
merged 3 commits into from
Jun 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions fixtures/dom/src/components/Header.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class Header extends React.Component {
<option value="/custom-elements">Custom Elements</option>
<option value="/media-events">Media Events</option>
<option value="/pointer-events">Pointer Events</option>
<option value="/mouse-events">Mouse Events</option>
</select>
</label>
<label htmlFor="react_version">
Expand Down
3 changes: 3 additions & 0 deletions fixtures/dom/src/components/fixtures/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import EventPooling from './event-pooling';
import CustomElementFixtures from './custom-elements';
import MediaEventsFixtures from './media-events';
import PointerEventsFixtures from './pointer-events';
import MouseEventsFixtures from './mouse-events';

const React = window.React;

Expand Down Expand Up @@ -49,6 +50,8 @@ function FixturesPage() {
return <MediaEventsFixtures />;
case '/pointer-events':
return <PointerEventsFixtures />;
case '/mouse-events':
return <MouseEventsFixtures />;
default:
return <p>Please select a test fixture.</p>;
}
Expand Down
16 changes: 16 additions & 0 deletions fixtures/dom/src/components/fixtures/mouse-events/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import FixtureSet from '../../FixtureSet';
import MouseMovement from './mouse-movement';

const React = window.React;

class MouseEvents extends React.Component {
render() {
return (
<FixtureSet title="Mouse Events" description="">
<MouseMovement />
</FixtureSet>
);
}
}

export default MouseEvents;
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import TestCase from '../../TestCase';

const React = window.React;

class MouseMovement extends React.Component {
state = {
movement: {x: 0, y: 0},
};

onMove = event => {
this.setState({x: event.movementX, y: event.movementY});
};

render() {
const {x, y} = this.state;

const boxStyle = {
padding: '10px 20px',
border: '1px solid #d9d9d9',
margin: '10px 0 20px',
};

return (
<TestCase
title="Mouse Movement"
description="We polyfill the movementX and movementY fields."
affectedBrowsers="IE, Safari">
<TestCase.Steps>
<li>Mouse over the box below</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The reported values should equal the pixel (integer) difference
between mouse movements positions.
</TestCase.ExpectedResult>

<div style={boxStyle} onMouseMove={this.onMove}>
<p>Trace your mouse over this box.</p>
<p>
Last movement: {x},{y}
</p>
</div>
</TestCase>
);
}
}

export default MouseMovement;
21 changes: 21 additions & 0 deletions packages/react-dom/src/events/SyntheticMouseEvent.js
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;
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

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.

Copy link
Contributor Author

@jasonwilliams jasonwilliams Jun 20, 2018

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;