fix(popover): Fix popover focus-leave dismissal#2298
Conversation
🦋 Changeset detectedLatest commit: 5e96fce The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Reviewed against A11Y-160. This PR cleanly tackles the close-on-blur part of the ticket. Mapping the diff to the ticket's three ACs:
| A11Y-160 AC | This PR | Notes |
|---|---|---|
Announce expanded/collapsed (aria-expanded) |
✅ pre-existing + new regression test | already set in toggleAccessibilityAttributes (popover.ts:477); the PR adds a guard test, not the implementation |
Announce item count on open (aria-setsize/aria-posinset or aria-label) |
❌ out of scope | not touched in the diff |
| Menu closes when Tab moves focus outside (SO_07) | ✅ implemented | classic + Svelte, gated to role="menu" |
So this is a focused fix for SO_07 plus a regression test for the aria-expanded AC — item-count (SO_08) is still outstanding.
👌 Nice incidental catch: returning the setup* teardown from PopoverReference's onMount fixes a real listener leak where the old code created the cleanup closures but never registered them.
| protected get shouldHideOnFocusLeave() { | ||
| return this.popoverElement?.getAttribute("role") === "menu"; | ||
| } |
There was a problem hiding this comment.
shouldHideOnFocusLeave reads role from popoverElement (the .s-popover div). But the documented menu-in-popover markup (menus.md) puts role="menu" on the inner <ul class="s-menu">, with no role on .s-popover:
<div class="s-popover"><ul class="s-menu" role="menu">…</ul></div>I loaded the built bundle in a browser to confirm: with role="menu" on the inner <ul>, this getter returns false and the popover stays open on focus-leave (SO_07 not fixed); with role="menu" on .s-popover it works. The new test passes only because its fixture puts the role on .s-popover, which differs from how we document menus.
Can you confirm what the real dropdowns render? If the role lives on the inner list, we should detect it there. Otherwise we at least need to document that focus-leave dismissal requires role="menu" on the .s-popover root.
If real markup keeps role="menu" on the inner list, an option that stays backward-compatible:
| protected get shouldHideOnFocusLeave() { | |
| return this.popoverElement?.getAttribute("role") === "menu"; | |
| } | |
| protected get shouldHideOnFocusLeave() { | |
| return ( | |
| this.popoverElement?.getAttribute("role") === "menu" || | |
| !!this.popoverElement?.querySelector('[role="menu"]') | |
| ); | |
| } |
| }; | ||
|
|
||
| const onFocusOut = (e: FocusEvent) => { | ||
| if (tooltip || !pstate.visible || !pstate.closeOnFocusLeave) { |
There was a problem hiding this comment.
onFocusOut doesn't check dismissible, but onOutclick (:204) and onKeypress (:235) both do. A role="menu" popover with dismissible={false} will still close on focus-leave. Same asymmetry on the classic side: shouldHideOnFocusLeave (popover.ts:91) ignores the hide-on-outside-click="never" opt-out that shouldHideOnOutsideClick (popover.ts:73) respects, so a menu that explicitly disabled auto-dismissal has no way to opt out of focus-leave dismissal. Intended? If not, gate both on the existing flags.
| if (tooltip || !pstate.visible || !pstate.closeOnFocusLeave) { | |
| if (tooltip || !pstate.visible || !pstate.closeOnFocusLeave || !dismissible) { |
| return; | ||
| } | ||
|
|
||
| const relatedTarget = e.relatedTarget; |
There was a problem hiding this comment.
When relatedTarget isn't a contained Node the handler dismisses. That's correct for "focus left to nowhere" (and it's the behavior the null-relatedTarget test pins), but it also means: clicking a non-focusable dead-space region inside the menu blurs the active item with relatedTarget=null and closes the menu, and alt-tabbing/window-blur closes it too. Acceptable to ship as-is, but flagging as a known tradeoff in case QA reports a menu closing on an in-menu click. No change requested. (Same on the Svelte side, Popover.svelte:217.)
|
Every popover demo in the docs uses the default Heads-up: this only demos the Svelte path — the docs site doesn't run classic JS, so classic dismissal still relies on the unit tests. |
2 participants
Stacks fixes to address A11Y-160
Summary
s-popoverwhen keyboard focus leaves both the trigger and popover, but only for popover content marked withrole="menu".Why
A11Y-160 reports that dropdown/popover menus remain visually and programmatically expanded after users tab away. This keeps
aria-expandedtrue even though focus has moved outside the trigger/popover pair.Stacks popovers remain a low-level primitive, so the new dismissal behavior is derived from menu semantics instead of applying to dialogs or generic popover content.
Validation
npm run test:unitinpackages/stacks-classic: 30 passing across Chromium, Firefox, and WebKit.npx web-test-runner --config web-test-runner.config.mjs --files src/components/Popover/Popover.test.tsinpackages/stacks-svelte: 43 passing.npx web-test-runner --config web-test-runner.config.mjsinpackages/stacks-svelte: 572 passing.npx svelte-check --tsconfig ./tsconfig.json: 0 errors/warnings.