-
-
Notifications
You must be signed in to change notification settings - Fork 472
fix(useHideOthers): fix edge case with useHideOthers when nested in a a native popover #2336
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: v2
Are you sure you want to change the base?
Conversation
|
@coderabbitai review please |
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@zernonia: I'll review the changes in this PR right away. ✅ Actions performedReview triggered.
|
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.
| // Skip if inside a closed native popover | ||
| if (el && !el.closest('[popover]:not(:popover-open)')) | ||
| undo = hideOthers(el) | ||
| else if (undo) | ||
| undo() |
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.
🧩 Analysis chain
🌐 Web query:
:popover-open CSS pseudo-class browser support 2025
💡 Result:
Short answer — as of Jan 13, 2026: :popover-open is supported in current major browsers (Chromium-based browsers from Chrome/Edge 114+, Firefox 125+, and Safari 17+). Older browser versions do not support it. Sources: [1][2].
Sources
- Can I use (selector :popover-open) — browser support matrix. [1]
- MDN Popover API / :popover-open documentation. [2]
🌐 Web query:
CSS closest() method SyntaxError unsupported pseudo-class selector
💡 Result:
Element.closest(selector) uses the browser’s CSS selector parser (same as querySelector/matches). If the selector string is invalid or contains a pseudo‑class the browser doesn’t support, closest() throws a SyntaxError / DOMException (invalid selector / unsupported pseudo‑class). See MDN docs for closest() and :has() (browser support matters). [1][2]
Quick fixes
-
Remove or avoid unsupported pseudo‑classes in the selector (e.g. older browsers may not support :has()). [2][1]
-
Wrap the call in try/catch and fall back to manual traversal if it fails:
try { node.closest(selector); }
catch (e) { /* fallback: traverse parents and use matches() or custom logic */ } -
Use :is() / :where() when possible to make selector lists forgiving, or include a polyfill for unsupported selectors (or for :has()). [3][1]
If you paste the exact selector and target browsers you need to support, I can suggest a minimal fallback or polyfill.
Wrap in try-catch to handle browsers without :popover-open support.
The logic correctly skips hideOthers when inside a closed popover. However, closest() with the :popover-open pseudo-class throws a SyntaxError in browsers without Popover API support (Chrome <114, Firefox <125, Safari <17).
Wrap the selector in try-catch for graceful fallback:
Defensive fix
// Skip if inside a closed native popover
- if (el && !el.closest('[popover]:not(:popover-open)'))
+ let isInsideClosedPopover = false
+ try {
+ isInsideClosedPopover = !!el?.closest('[popover]:not(:popover-open)')
+ }
+ catch {
+ // :popover-open not supported in this browser, assume not inside closed popover
+ }
+ if (el && !isInsideClosedPopover)
undo = hideOthers(el)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Skip if inside a closed native popover | |
| if (el && !el.closest('[popover]:not(:popover-open)')) | |
| undo = hideOthers(el) | |
| else if (undo) | |
| undo() | |
| // Skip if inside a closed native popover | |
| let isInsideClosedPopover = false | |
| try { | |
| isInsideClosedPopover = !!el?.closest('[popover]:not(:popover-open)') | |
| } | |
| catch { | |
| // :popover-open not supported in this browser, assume not inside closed popover | |
| } | |
| if (el && !isInsideClosedPopover) | |
| undo = hideOthers(el) | |
| else if (undo) | |
| undo() |
resolves #2240
this should also fix it for other elements that are using native popover.
Summary
Fixes
useHideOthersapplyingaria-hiddenwhen nested inside a closed native popover.Problem
When a component using
useHideOthersis inside a native<div popover>, it runs even when the popover is closed.Solution
Skip
hideOtherswhen inside a closed native popover. This is a simple non-reactive fix. It doesn't track when the popover opens/closes. Could be made reactive if needed.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.