-
Notifications
You must be signed in to change notification settings - Fork 239
fix(overlay): siblings overlays stay visible #5951
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
base: main
Are you sure you want to change the base?
Conversation
|
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
| // The overlay's transition guarantee uses 3 animation frames before | ||
| // calling finish() which triggers returnFocus(). We need to wait longer | ||
| // than that before re-enabling hover to prevent it from opening. | ||
| requestAnimationFrame(() => { |
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.
This looks a bit wonky and I assume it might bring further timing issues down the road when used in quite complex applications.
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.
@spdev3000 Thanks for the feedback. I’ve refactored this to a cleaner approach using a single setTimeout. The 100ms delay reliably covers the close transition and focus return, while remaining short enough to avoid any perceived sluggishness.
This also smooths out edge cases like rapid close/open interactions. Please take another look and let me know if you see any remaining concerns or edge cases we should address.
|
looks good for me, one question: |
To your first question this is intentional - modal overlays should trap interaction until explicitly dismissed. This matches the Spectrum design pattern where modals require explicit user action to close. If you need click-outside-to-close behavior, you should use |
|
To #1 re #2: Yes we have use cases, where no "cancel" button is rendered inside a dialog and Escape should not dismiss or cancel the dialog, so we'd need such a behavior. |
…r lastInteraction guard
@spdev3000 you can use the overlay type="page" |
Description
Root cause
When we migrated from .showModal() to the Popover API for performance optimization, modal overlays were configured to use popover="auto". The browser's Popover API only allows one popover="auto" element to be visible at a time—opening a second one automatically closes the first (light dismiss behavior).
Additionally, a recent PR (#5907) added click-blocking for modal overlays to replicate
showModal()behavior. This click-blocking was preventing light dismiss from working when combined withpopover="manual".Changes
Overlay.tspopover="auto"topopover="manual"showModal()needsModalBackdropto only apply to page overlayshandlePointerupin OverlayStack for light dismissOverlayStack.tshandlePointerdownto only block clicks for page overlayspointerdownPathis cached sohandlePointerupcan handle light dismisspopover="manual"doesn't have automatic Escape key dismissal, we now handle it explicitlyMotivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review