Repository navigation
Conversation
component:HoverCard FR6: Escape closes a visible card through the shared dismissal stack. The stack listens in each layer's own document, found through getContainer, but useHoverCard passed none and read the page's document for presence, the focus check, and the focus-into-card check. A card rendered into an iframe or a Picture-in-Picture window therefore never closed on Escape there (WCAG 1.4.13), and focus moving into it closed it. useHoverCard now registers with getContainer: () => triggerRef.current and reads the trigger's ownerDocument wherever it read the page's. Tests: three jsdom cases in a real second document (red before), a Chromium spec over a new no-visual iframe story. Record: FR6 loses its gap sentence, AR2 and Current-state impact drop the other-document gap, and the verification row names the cases.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec cf5ab9d |
github-actions
Bot
removed
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the hook change itself works (the three new iframe cases fail on main and pass here), but a record fix and a wording or test fix are needed first.
docs/families/overlay-dismissal.mdstill ends the Tooltip/HoverCard member row with "HoverCard reads its presence from the page's document, so in another document it takes no request (FR8) and returns no focus there (FR9)"; please drop that sentence so it does not contradict the updated FR6.- The changeset and FR6 name Picture-in-Picture windows, but every new case runs in a same-origin iframe; add a Picture-in-Picture case to
HoverCardSecondDocument.a11y.chromium.spec.ts(the Popover second-document spec already has the harness) or narrow the wording to iframes.
Optional: move ownerDocumentOf so the "Focus trigger behavior" doc comment stays on HoverCardFocusTrigger.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A `HoverCard` rendered into another document, such as an iframe or a Picture-in-Picture window, now closes when Escape is pressed there, and Escape from inside the card returns focus to its trigger. Moving focus from the trigger into such a card also keeps it open. Before, the card read only the page's document, so Escape in the other document never closed it. |
There was a problem hiding this comment.
This names Picture-in-Picture windows, but the jsdom cases and the Chromium spec only run in a same-origin iframe. Either add a Picture-in-Picture case to HoverCardSecondDocument.a11y.chromium.spec.ts (the Popover second-document spec opens one) or narrow this to "a same-origin iframe".
| /** | ||
| * Focus trigger behavior for hover cards | ||
| */ | ||
| /** | ||
| * The document the card lives in: its trigger's, which may be an iframe or a | ||
| * Picture-in-Picture window. This page's before a trigger attaches; null with | ||
| * no DOM. | ||
| */ | ||
| function ownerDocumentOf(el: HTMLElement | null): Document | null { | ||
| if (el != null) { | ||
| return el.ownerDocument; | ||
| } | ||
| return typeof document === 'undefined' ? null : document; | ||
| } | ||
|
|
||
| export type HoverCardFocusTrigger = 'auto' | 'always' | 'never'; |
There was a problem hiding this comment.
Non-blocking: inserting the helper here leaves the existing "Focus trigger behavior for hover cards" JSDoc on ownerDocumentOf, so the exported HoverCardFocusTrigger loses its doc comment. Move the helper above line 62 or below the type. Its own comment can keep only the reason: "The card lives in its trigger's document. That can be an iframe or a Picture-in-Picture window."
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
…ent; add the Picture-in-Picture case CodeBunny on #9592: the family record's Tooltip/HoverCard row still said HoverCard reads its presence from the page's document; that sentence goes, and the FR8/FR9 evidence row names HoverCard's second-document checks. The story gains a Pop out button that portals the card into a Document Picture-in-Picture window, and the Chromium spec checks Escape and the focus return there too, so the changeset's Picture-in-Picture claim has a test. The ownerDocumentOf helper moves above HoverCardFocusTrigger's doc comment, and the readiness count drops to seven gaps.
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec b3b8e9f |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the hook fix works in both an iframe and a Picture-in-Picture window, but the new Picture-in-Picture Chromium case cannot pass as written.
- In
HoverCardSecondDocument.a11y.chromium.spec.ts, the secondtrigger.hover()(line 97) lands where the pointer already rests, so nomouseenterfires and line 98 times out; addingawait pictureInPicture.mouse.move(0, 0);before it makes all three cases pass.
Optional: the change also lets a card whose trigger sits inside a raw useFocusTrap container take Escape before that trap in the page, which deserves a line in the changeset, and ownerDocumentOf's comment can keep only its reason.
[Automated review]
| await pictureInPicture.keyboard.press('Escape'); | ||
| await expect(message).toBeHidden(); | ||
|
|
||
| await trigger.hover(); |
There was a problem hiding this comment.
The pointer already rests on the trigger from line 91, so this hover fires no mouseenter, and the card Escape just dismissed never reopens; line 98 times out on every run. Move the pointer off first:
await pictureInPicture.mouse.move(0, 0);
await trigger.hover();With that line all three cases pass.
| /** | ||
| * The document the card lives in: its trigger's, which may be an iframe or a | ||
| * Picture-in-Picture window. This page's before a trigger attaches; null with | ||
| * no DOM. | ||
| */ |
There was a problem hiding this comment.
Non-blocking: the second sentence restates the two branches below. Keeping only the reason: "The card lives in its trigger's document. That can be an iframe or a Picture-in-Picture window."
…e hovering it again CodeBunny on #9592: the second hover landed where the pointer already rested, so no mouseenter fired; the case moves the pointer away first. The changeset notes that a card inside a focus-trap container now takes Escape before it, and ownerDocumentOf's comment keeps only its reason.
|
/approve-spec e53260c |
There was a problem hiding this comment.
Changes requested: the second-document fix works in an iframe and a Picture-in-Picture window (the new jsdom cases fail on main and pass here, and all three Chromium cases pass), but two things need fixing first.
- A
useHoverCardtrigger rendered after the hook mounts, inside a second document, leaves the card registered in the page, so Escape pressed in the page closes that card and Escape in its own document does nothing; register once the trigger exists, in its document, and add a case for it. - The changeset says a card inside a focus-trap container now takes Escape first, but no test covers it and the description still says nothing changes in the page, so add a case and update the description.
[Automated review]
| isActive: true, | ||
| // The stack listens for Escape in the trigger's own document, which may be | ||
| // an iframe or a Picture-in-Picture window rather than this page. | ||
| getContainer: () => triggerRef.current, |
There was a problem hiding this comment.
The stack reads this once, when the hook registers on mount, and keeps that document. A useHoverCard trigger rendered in a later commit inside an iframe leaves the card registered in the page. isPresent below now finds the card in the frame, so Escape pressed in the page closes it, and Escape pressed in the frame does nothing. Register only once the trigger is attached, for example by gating isActive on the trigger's document held in state from interactionRef, and add a case with a late trigger.
User impact
An app renders a
HoverCardinto a second document, such as a same-origin iframe or a Document Picture-in-Picture window, and the user works there:In the page itself, nothing changes.
Problem and solution fit
getContainer.useHoverCardpassed none, so the stack listened only in the page's document. The hook also looked up its card, and the focused element, in the page's document. In a second document the card is never found, so the hook reads it as closed, and focus moving into it reads as focus leaving.Expected behavior and authority
component:HoverCardFR6 (current): Escape closes a visible card through the shared dismissal stack. The record lists the other-document failure as an implementation gap under Current-state impact, and AR2 cites WCAG 1.4.13.family:overlay-dismissal: the stack routes a press to the top-most layer of the document it arrived in.Smallest restoration
useHoverCardregisters withgetContainer: () => triggerRef.current. It also reads the trigger'sownerDocument(a smallownerDocumentOfhelper) in the four places that read the page'sdocument: Escape presence, the Escape focus check, the controlled-close focus check, and the focus-into-card check.family:overlay-dismissaldrops the Tooltip/HoverCard row's sentence that HoverCard reads the page's document, and its FR8/FR9 evidence row names HoverCard's second-document checks.Evidence
HoverCard.test.tsx("in a second document", a real iframe document) fail on main:useLayerDismissal.test.tsxpass unchanged (93 tests).HoverCardSecondDocument.a11y.chromium.spec.tsdrives a new no-visual story in Chromium, in a same-origin iframe and in a Document Picture-in-Picture window (Pop out): Escape pressed there closes the card, and Escape from inside the card returns focus to the trigger.Scope
Testing
pnpm vitest run packages/core/src/HoverCard packages/core/src/Layer/useLayerDismissal.test.tsx: 93 passed.pnpm check:knowledge: aligned.Writing rubric results
Run over the FR6 row, AR2, Current-state impact, the FR4–FR6 verification row, and
family:overlay-dismissal's Tooltip/HoverCard and FR8/FR9 rows (docs/contributing/spec-writing.md):family:overlay-dismissal.