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

do not propagate events through portal #1840

Merged
merged 9 commits into from May 28, 2021
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
8 changes: 4 additions & 4 deletions packages/@react-aria/interactions/src/useHover.ts
Expand Up @@ -104,7 +104,7 @@ export function useHover(props: HoverProps): HoverResult {
let {hoverProps, triggerHoverEnd} = useMemo(() => {
let triggerHoverStart = (event, pointerType) => {
state.pointerType = pointerType;
if (isDisabled || pointerType === 'touch' || state.isHovered) {
if (isDisabled || pointerType === 'touch' || state.isHovered || !event.currentTarget.contains(event.target)) {
return;
}

Expand All @@ -130,7 +130,7 @@ export function useHover(props: HoverProps): HoverResult {
let triggerHoverEnd = (event, pointerType) => {
state.pointerType = '';
state.target = null;

if (pointerType === 'touch' || !state.isHovered) {
return;
}
Expand Down Expand Up @@ -164,7 +164,7 @@ export function useHover(props: HoverProps): HoverResult {
};

hoverProps.onPointerLeave = (e) => {
if (!isDisabled) {
if (!isDisabled && e.currentTarget.contains(e.target as HTMLElement)) {
triggerHoverEnd(e, e.pointerType);
}
};
Expand All @@ -182,7 +182,7 @@ export function useHover(props: HoverProps): HoverResult {
};

hoverProps.onMouseLeave = (e) => {
if (!isDisabled) {
if (!isDisabled && e.currentTarget.contains(e.target as HTMLElement)) {
triggerHoverEnd(e, 'mouse');
}
};
Expand Down
59 changes: 53 additions & 6 deletions packages/@react-aria/interactions/src/usePress.ts
Expand Up @@ -210,11 +210,10 @@ export function usePress(props: PressHookProps): PressResult {

let pressProps: HTMLAttributes<HTMLElement> = {
onKeyDown(e) {
if (isValidKeyboardEvent(e.nativeEvent)) {
if (isValidKeyboardEvent(e.nativeEvent) && e.currentTarget.contains(e.target as HTMLElement)) {
e.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do the check for keyboard events as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah most likely. i'll see if i can add a good test for that

e.stopPropagation();


// If the event is repeating, it may have started on a different element
// after which focus moved to the current element. Ignore these events and
// only handle the first key down event.
Expand All @@ -230,11 +229,15 @@ export function usePress(props: PressHookProps): PressResult {
}
},
onKeyUp(e) {
if (isValidKeyboardEvent(e.nativeEvent) && !e.repeat) {
if (isValidKeyboardEvent(e.nativeEvent) && !e.repeat && e.currentTarget.contains(e.target as HTMLElement)) {
triggerPressUp(createEvent(state.target, e), 'keyboard');
}
},
onClick(e) {
if (e && !e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

if (e && e.button === 0) {
e.stopPropagation();
if (isDisabled) {
Expand Down Expand Up @@ -279,8 +282,8 @@ export function usePress(props: PressHookProps): PressResult {

if (typeof PointerEvent !== 'undefined') {
pressProps.onPointerDown = (e) => {
// Only handle left clicks
if (e.button !== 0) {
// Only handle left clicks, and ignore events that bubbled through portals.
if (e.button !== 0 || !e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

Expand Down Expand Up @@ -315,6 +318,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onMouseDown = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

if (e.button === 0) {
// Chrome and Firefox on touch Windows devices require mouse down events
// to be canceled in addition to pointer events, or an extra asynchronous
Expand All @@ -328,6 +335,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onPointerUp = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

// Only handle left clicks
// Safari on iOS sometimes fires pointerup events, even
// when the touch isn't over the target, so double check.
Expand Down Expand Up @@ -377,13 +388,17 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onDragStart = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

// Safari does not call onPointerCancel when a drag starts, whereas Chrome and Firefox do.
cancel(e);
};
} else {
pressProps.onMouseDown = (e) => {
// Only handle left clicks
if (e.button !== 0) {
if (e.button !== 0 || !e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

Expand Down Expand Up @@ -413,6 +428,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onMouseEnter = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

e.stopPropagation();
if (state.isPressed && !state.ignoreEmulatedMouseEvents) {
state.isOverTarget = true;
Expand All @@ -421,6 +440,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onMouseLeave = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

e.stopPropagation();
if (state.isPressed && !state.ignoreEmulatedMouseEvents) {
state.isOverTarget = false;
Expand All @@ -429,6 +452,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onMouseUp = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

if (!state.ignoreEmulatedMouseEvents && e.button === 0) {
triggerPressUp(e, state.pointerType);
}
Copy link
Member

Choose a reason for hiding this comment

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

Make sure we get all of the events assigned to pressProps. I think maybe touch events are missing? Is there a reason they were not included?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think i had placed them all the functions with an event, but i couldn't tell if they were doing anything or not. should i go ahead and place them in all the pressProps events?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah all of the ones that are on pressProps, not global listeners.

Expand Down Expand Up @@ -458,6 +485,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onTouchStart = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

e.stopPropagation();
let touch = getTouchFromEvent(e.nativeEvent);
if (!touch) {
Expand All @@ -483,6 +514,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onTouchMove = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

e.stopPropagation();
if (!state.isPressed) {
return;
Expand All @@ -501,6 +536,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onTouchEnd = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

e.stopPropagation();
if (!state.isPressed) {
return;
Expand All @@ -523,6 +562,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onTouchCancel = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

e.stopPropagation();
if (state.isPressed) {
cancel(e);
Expand All @@ -541,6 +584,10 @@ export function usePress(props: PressHookProps): PressResult {
};

pressProps.onDragStart = (e) => {
if (!e.currentTarget.contains(e.target as HTMLElement)) {
return;
}

cancel(e);
};
}
Expand Down
89 changes: 88 additions & 1 deletion packages/@react-aria/interactions/test/useHover.test.js
Expand Up @@ -11,8 +11,13 @@
*/

import {act, fireEvent, render} from '@testing-library/react';
import {installPointerEvent} from '@react-spectrum/test-utils';
import {ActionButton} from '@react-spectrum/button';
import {Dialog, DialogTrigger} from '@react-spectrum/dialog';
import {installMouseEvent, installPointerEvent} from '@react-spectrum/test-utils';
import MatchMediaMock from 'jest-matchmedia-mock';
import {Provider} from '@react-spectrum/provider';
import React from 'react';
import {theme} from '@react-spectrum/theme-default';
import {useHover} from '../';

function Example(props) {
Expand Down Expand Up @@ -469,4 +474,86 @@ describe('useHover', function () {
expect(el.textContent).toBe('test');
});
});

describe('portal event bubbling', () => {
function PortalExample(props) {
let {elementType: ElementType = 'div', ...otherProps} = props;
let {hoverProps} = useHover(otherProps);
return (
<Provider theme={theme}>
<ElementType {...hoverProps} tabIndex="0">
<DialogTrigger>
<ActionButton>open</ActionButton>
<Dialog>
test
</Dialog>
</DialogTrigger>
</ElementType>
</Provider>
);
}

beforeAll(() => {
jest.useFakeTimers();
});
afterAll(() => {
jest.useRealTimers();
});

let matchMedia;
beforeEach(() => {
matchMedia = new MatchMediaMock();
// this needs to be a setTimeout so that the dialog can be removed from the dom before the callback is invoked
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(() => cb(), 0));
});

afterEach(() => {
// Ensure we close any dialogs before unmounting to avoid warning.
let dialog = document.querySelector('[role="dialog"]');
if (dialog) {
act(() => {
fireEvent.keyDown(dialog, {key: 'Escape'});
fireEvent.keyUp(dialog, {key: 'Escape'});
jest.runAllTimers();
});
}

matchMedia.clear();
window.requestAnimationFrame.mockRestore();
});

describe.each`
type | prepare | actions
${'Mouse Events'} | ${installMouseEvent} | ${[
(el) => fireEvent.mouseEnter(el, {button: 0}),
(el) => fireEvent.mouseLeave(el, {button: 0})
]}
${'Pointer Events'} | ${installPointerEvent}| ${[
(el) => fireEvent(el, pointerEvent('pointerover', {button: 0})),
(el) => fireEvent(el, pointerEvent('pointerout', {button: 0}))
]}
`('$type', ({actions: [start, end], prepare}) => {
prepare();
it('stop event bubbling through portal', () => {
const hoverMock = jest.fn();
let res = render(
<PortalExample
onHoverStart={hoverMock}
onHoverEnd={hoverMock}
onHoverChange={hoverMock} />
);

fireEvent.click(res.getByText('open'));

act(() => {
jest.runAllTimers();
});

let el = res.getByText('test');
start(el);
end(el);
expect(hoverMock.mock.calls).toHaveLength(0);
});
});
});
});