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

Fix movementX/Y polyfill with capture events #19672

Merged
merged 2 commits into from Aug 21, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 22 additions & 25 deletions packages/react-dom/src/events/SyntheticEvent.js
Expand Up @@ -152,11 +152,22 @@ export const UIEventInterface: EventInterfaceType = {
detail: 0,
};

let previousScreenX = 0;
let previousScreenY = 0;
// Use flags to signal movementX/Y has already been set
let isMovementXSet = false;
let isMovementYSet = false;
let lastMovementX;
let lastMovementY;
let lastMouseEvent;

function updateMouseMovementPolyfillState(event) {
if (event !== lastMouseEvent) {
if (lastMouseEvent && event.type === 'mousemove') {
lastMovementX = event.screenX - lastMouseEvent.screenX;
lastMovementY = event.screenY - lastMouseEvent.screenY;
} else {
lastMovementX = 0;
lastMovementY = 0;
}
lastMouseEvent = event;

Choose a reason for hiding this comment

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

Won't this GC root the event? This seems like a leak.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will root only one (the very last) mouse event, and only on IE11, so this should not be an issue.

}
}

/**
* @interface MouseEvent
Expand Down Expand Up @@ -189,31 +200,17 @@ export const MouseEventInterface: EventInterfaceType = {
if ('movementX' in event) {
return event.movementX;
}

const screenX = previousScreenX;
previousScreenX = event.screenX;

if (!isMovementXSet) {
isMovementXSet = true;
return 0;
}

return event.type === 'mousemove' ? event.screenX - screenX : 0;
updateMouseMovementPolyfillState(event);
return lastMovementX;
},
movementY: function(event) {
if ('movementY' in event) {
return event.movementY;
}

const screenY = previousScreenY;
previousScreenY = event.screenY;

if (!isMovementYSet) {
isMovementYSet = true;
return 0;
}

return event.type === 'mousemove' ? event.screenY - screenY : 0;
// Don't need to call updateMouseMovementPolyfillState() here
// because it's guaranteed to have already run when movementX
// was copied.
return lastMovementY;
},
};

Expand Down
Expand Up @@ -16,6 +16,7 @@ describe('SyntheticMouseEvent', () => {
let container;

beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');

Expand Down Expand Up @@ -77,4 +78,67 @@ describe('SyntheticMouseEvent', () => {
expect(events[1]).toBe(6);
expect(events[2]).toBe(0); // mousedown event should have movementX at 0
});

it('should correctly calculate movementX/Y for capture phase', () => {
const events = [];
const onMouseMove = event => {
events.push(['move', false, event.movementX, event.movementY]);
};
const onMouseMoveCapture = event => {
events.push(['move', true, event.movementX, event.movementY]);
};
const onMouseDown = event => {
events.push(['down', false, event.movementX, event.movementY]);
};
const onMouseDownCapture = event => {
events.push(['down', true, event.movementX, event.movementY]);
};

const node = ReactDOM.render(
<div
onMouseMove={onMouseMove}
onMouseMoveCapture={onMouseMoveCapture}
onMouseDown={onMouseDown}
onMouseDownCapture={onMouseDownCapture}
/>,
container,
);

let event = new MouseEvent('mousemove', {
relatedTarget: null,
bubbles: true,
screenX: 2,
screenY: 2,
});

node.dispatchEvent(event);

event = new MouseEvent('mousemove', {
relatedTarget: null,
bubbles: true,
screenX: 8,
screenY: 9,
});

node.dispatchEvent(event);

// Now trigger a mousedown event to see if movementX has changed back to 0
event = new MouseEvent('mousedown', {
relatedTarget: null,
bubbles: true,
screenX: 25,
screenY: 65,
});

node.dispatchEvent(event);

expect(events).toEqual([
['move', true, 0, 0],
['move', false, 0, 0],
['move', true, 6, 7],
['move', false, 6, 7],
['down', true, 0, 0],
['down', false, 0, 0],
]);
});
});