-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
stop propagation of clicks on tooltips #4479
Conversation
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.
Great, looks like it's working.
Would you mind adding a test to TooltipTrigger tests as well?
it('does not propagate pointer or click through the portal', () => {
let onPointerDown = jest.fn();
let onPointerUp = jest.fn();
let onClick = jest.fn();
let {getByRole} = render(
<Provider theme={theme}>
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */}
<div onPointerDown={onPointerDown} onPointerUp={onPointerUp} onClick={onClick}>
<TooltipTrigger>
<ActionButton>Trigger</ActionButton>
<Tooltip>Helpful information.</Tooltip>
</TooltipTrigger>
</div>
</Provider>
);
let button = getByRole('button');
act(() => {
button.focus();
});
let tooltip = getByRole('tooltip');
triggerPress(tooltip);
expect(onPointerDown).not.toHaveBeenCalled();
expect(onPointerUp).not.toHaveBeenCalled();
expect(onClick).not.toHaveBeenCalled();
});
packages/@react-spectrum/tooltip/stories/TooltipTrigger.stories.tsx
Outdated
Show resolved
Hide resolved
packages/@react-spectrum/tooltip/stories/TooltipTrigger.stories.tsx
Outdated
Show resolved
Hide resolved
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.
A bit concerned about using usePress here just to stop propagation of click events, especially since usePress handles so much extra stuff like preventing text selection. Any reason we don't just use a pointerdown handler with stopPropagation? The tooltip shouldn't be keyboard focusable anyways and tooltips aren't accessible on touch devices so no need for all the extra handling for those right?
I think it's fine to stop it with usePress. Because it's more than just pointerDown, we currently still support devices that don't have pointer events, so the full list would be
Might also need it for touch? and maybe the up/move/enter/leave? Seemed easier to let usePress just do it's thing, which is to block anything that might be perceived as a click. |
This reverts commit b677593.
Closes #4480
✅ Pull Request Checklist:
📝 Test Instructions:
Open new storybook entry. Click red box. Button toggles disabled state. Try clicking on tooltip, click event is not propagated to the red div.
🧢 Your Project: