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 TooltipTrigger with DialogTrigger #5526

Merged
merged 4 commits into from
Dec 7, 2023
Merged

Fix TooltipTrigger with DialogTrigger #5526

merged 4 commits into from
Dec 7, 2023

Conversation

devongovett
Copy link
Member

Closes #3009, closes #4195

This is an alternate fix to #4195. It uses native onPointerDown and onKeyDown to hide tooltips instead of usePress. This avoids the double event causing tooltips preventing dialog triggers and other press events from working. It also allows press events on elements with tooltips to propagate rather than blocking interactions which would be unexpected.

I tried to add a test but was unable to reproduce the issue in a test environment. I believe this is due to act batching the double press events together unlike how it works in the browser.

@rspbot
Copy link

rspbot commented Dec 5, 2023

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

I think @LFDanLu had some concerns with this approach from when it was suggested last time https://github.com/adobe/react-spectrum/pull/4195/files#r1136175022
From reading back over it, I don't follow what it was (might've been all theoretical), we just return a smaller set of event handlers now, but they are the same kinds as before. Tested it in hooks docs and new stories are working for me

@LFDanLu
Copy link
Member

LFDanLu commented Dec 5, 2023

The concern from before was that the event propagation of the pointer/keyboard events would be inconsistent depending on how the user constructed their TooltipTrigger. If their button within their implemented TooltipTrigger used useButton, the pointer/keyboard events would have their event propagation automatically stopped due to usePress and useFocusable respectively. If their button within their TooltipTrigger was just a native button like it is in the useTooltipTrigger then event propagation wouldn't be stopped.

@devongovett
Copy link
Member Author

I think that's expected. Tooltips on their own shouldn't stop propagation, because clicks aren't really their main purpose, just a secondary interaction that's not exposed to the app developer. If the button has usePress then they should stop propagation because the click is being handled by the app via the onPress event.

@rspbot
Copy link

rspbot commented Dec 6, 2023

@rspbot
Copy link

rspbot commented Dec 7, 2023

@rspbot
Copy link

rspbot commented Dec 7, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/tooltip

TooltipTriggerAria

 TooltipTriggerAria {
   tooltipProps: DOMAttributes
-  triggerProps: (DOMAttributes & PressProps & HoverProps & FocusEvents)
+  triggerProps: DOMAttributes
 }

it changed:

  • useTooltipTrigger

@devongovett devongovett merged commit 4d32d75 into main Dec 7, 2023
2 checks passed
@devongovett devongovett deleted the tooltip-press branch December 7, 2023 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DialogTriggers do not work with TooltipTriggers
4 participants