fix(overlay): don't close auto overlay on click inside content in a focusable ancestor - #6625
Open
Rajdeepc wants to merge 6 commits into
Open
fix(overlay): don't close auto overlay on click inside content in a focusable ancestor#6625Rajdeepc wants to merge 6 commits into
Rajdeepc wants to merge 6 commits into
Conversation
…ancestor closeOnFocusOut decided whether focus left the overlay by checking if the newly-focused element was a descendant of the overlay. Clicking non-focusable overlay content resolves focus onto the nearest focusable ancestor instead of the click target; when that ancestor also wraps the overlay itself, it sits above the overlay in the DOM, so the check always concluded focus had left and closed the overlay even though the click landed inside it. closeOnFocusOut now also tracks whether the pointerdown causing the current focus change originated inside the overlay's own composed subtree, and treats focus as remaining within the overlay in that case regardless of where it was ultimately resolved to. Fixes #5731 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5057a0d The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Rajdeepc
added
Status:Ready for review
Contributor
📚 Branch Preview Links🔍 Gen1 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 |
Collaborator
Coverage Report for CI Build 32134464340Coverage increased (+0.009%) to 96.25%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…verage Render each popover already open (matching the other click-triggered stories in this file) and drop the dev tag plus the VRT/Chromatic skips, since this is a real bug-fix regression story, not a dev-only scratch example, and should get golden-hash coverage like any other story here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The three-overlay combined story rendered every sp-overlay open at once, but the native popover="auto" behavior only allows a single auto popover open at a time, so the browser itself force-closed two of the three regardless of the closeOnFocusOut fix. Split it into one story per trigger variant, each with a single already-open overlay, so all three variants get real VRT coverage. Also fixed the flex-column wrapper defaulting to align-items: stretch, which stretched the trigger buttons to full width. Verified in a live browser: the previously-broken (wrapped) case stays open when clicking its own content and still closes on an actual outside click. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cdransf
requested changes
Aug 17, 2026
| // The interaction that caused this focus change started within the overlay, | ||
| // even though focus may have resolved onto an ancestor outside of it. Treat | ||
| // focus as still within the overlay. | ||
| if (pointerdownWasWithinOverlay) { |
Member
There was a problem hiding this comment.
Is this guard too broad? It's placed before we check event.relatedTarget (we could check if it's an ancestor of the overlay in the composed tree rather than tracking the pointer gesture origin). E.g. if we have a button or interaction inside the overlay that sends focus elsewhere, the overlay would stay open. ✨
Contributor
Author
There was a problem hiding this comment.
If we check relatedTarget is an ancestor of the overlay alone isn't a safe replacement though, since document.body is trivially an ancestor of every overlay. I think that'd stop the overlay from ever closing on outside click.
I would track the actual pointerdown target, and only suppress the close when relatedTarget is an ancestor of (or equal to) that specific element.
| capture: true, | ||
| }); | ||
| } else { | ||
| listenerRoot.removeEventListener( |
Member
There was a problem hiding this comment.
Should we reset lastPointerdownWasWithinOverlay here?
this.lastPointerdownWasWithinOverlay = false;
Contributor
Author
There was a problem hiding this comment.
Will reset it there so a pending flag can't leak into the next open cycle if the overlay tears down before its matching pointerup fires. Good catch.
…rget check Addresses review feedback on #6625: the previous boolean flag treated any pointerdown anywhere inside the overlay as sufficient to suppress a subsequent focusout, which would also mask a legitimate close if something inside the overlay explicitly redirected focus to an unrelated element on pointerdown/mousedown. Now tracks the specific pointerdown target and only suppresses the close when relatedTarget is an ancestor of (or equal to) that same target, i.e. exactly the relationship the browser creates by resolving focus to the nearest focusable ancestor of a non-focusable click target. An explicit focus redirect to an unrelated element no longer matches this and falls through to the normal close path. Also: - clears the pending pointerdown record on pointercancel in addition to pointerup, so an interrupted gesture (e.g. the OS cancelling a touch drag) can't leave a stale record behind - resets the record when the auto-type listeners are torn down, so a pending record can't leak into the next time the same overlay instance opens - factors the shadow-DOM-safe "is X an ancestor of Y" check (already used to test overlay containment) into a shared private helper, now reused for both the original containment check and the new one, with `{ once: true }` so the throwaway listener cleans itself up Added a regression test for the redirect-to-unrelated-element case; confirmed it fails against the previous implementation and passes against this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… scenario Reverts the earlier split into three stories back into a single focusableAncestorWrapper story covering all scenarios, per feedback that a single story is preferred over three. Since the native popover="auto" behavior only allows one auto popover open at a time, none are pre-opened via the `open` attribute anymore; the story is closed by default and driven interactively (click to open, click content to verify it stays open, click outside to verify it closes). Also folds in a fourth scenario for the reviewer-flagged case from PR #6625: a button inside the popover that redirects focus to an unrelated field on pointerdown. Clicking it should still close the popover, demonstrating the tightened ancestor-of-click-target check. Verified all four scenarios interactively in a live Storybook: the wrapped cases stay open on internal clicks, and the redirect case still closes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-ancestor-swc-5731
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes
[type="auto"]sp-overlayincorrectly closing when clicking non-focusable content inside its own popover, when the trigger and the overlay share a focusable ancestor (e.g. a<div tabindex="0">wrapping both).closeOnFocusOutdecided whether focus left the overlay by checking if the newly-focused element (event.relatedTarget) was a descendant of the overlay. Clicking non-focusable overlay content (plain text, padding, etc.) causes the browser to resolve focus onto the nearest focusable ancestor instead of the click target; when that ancestor also wraps the overlay itself, it sits above the overlay in the DOM, so the check always concluded focus had left and closed the overlay even though the click landed inside it.closeOnFocusOutnow also tracks whether thepointerdowncausing the current focus change originated inside the overlay's own composed subtree (via a new capture-phasepointerdown/pointeruppair mirroring the existingfocusoutlistener lifecycle), and treats focus as remaining within the overlay in that case regardless of where it was ultimately resolved to.Motivation and context
Reported in the linked issue: any consumer that needs a focusable wrapper around a trigger +
sp-overlaypair (for example, a keyboard-navigable card or list row) sees the overlay dismiss itself on the very first click inside its own content, makingtype="auto"unusable in that composition pattern.Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Auto overlay stays open when clicking its own content inside a focusable wrapper
Overlay ElementstoryfocusableAncestorWrapper(devtag) in StorybookNo regression to existing focus-out / outside-click dismissal
type="auto"overlay elsewhere in the library (e.g. Picker, Action Menu)Device review
Accessibility testing checklist
Keyboard (required — document steps below)
type="auto"overlay via its trigger with Enter/SpaceScreen reader (required — document steps below)
type="auto"overlay wrapped in a focusable ancestor, with a screen reader activeVerification performed
overlay-element.test.ts) reproducing the reported scenario exactly (trigger + overlay wrapped intabindex="0", click on non-focusable content) — passes.overlayyarn test group: all tests pass.overlay-apiyarn test group (action-menu, dialog, menu, overlay, picker, tooltip — the realtype="auto"consumers): all tests pass, no regressions.dev-tagged Storybook story (focusableAncestorWrapper) reproducing the exact three-button scenario from the issue for manual verification.eslint/prettier: clean.