From 96529388b57aaa29e0dc43759fbdd81f808f8809 Mon Sep 17 00:00:00 2001 From: Austin Date: Wed, 16 Sep 2026 14:44:52 -0500 Subject: [PATCH 1/2] fix(ui): restore focus after rapid shadow dialog reopen YPE-5889 --- .changeset/quiet-dialogs-return.md | 5 + ...07-prototype-shadow-dom-style-isolation.md | 12 +- docs/shadow-dom-isolation-plan.md | 17 ++- ...oduction-seam.shadow-isolation.stories.tsx | 105 ++++++++++++++++-- .../components/ui/use-shadow-dialog-focus.ts | 2 +- packages/ui/src/lib/shadow-root-host.tsx | 14 ++- 6 files changed, 127 insertions(+), 28 deletions(-) create mode 100644 .changeset/quiet-dialogs-return.md diff --git a/.changeset/quiet-dialogs-return.md b/.changeset/quiet-dialogs-return.md new file mode 100644 index 00000000..cb4176a1 --- /dev/null +++ b/.changeset/quiet-dialogs-return.md @@ -0,0 +1,5 @@ +--- +'@youversion/platform-react-ui': patch +--- + +Restore the original Shadow DOM opener after a dialog closes and reopens during its exit animation, while ignoring disconnected or cross-tree focus targets. diff --git a/docs/adr/0007-prototype-shadow-dom-style-isolation.md b/docs/adr/0007-prototype-shadow-dom-style-isolation.md index a54cb786..e0c80b6d 100644 --- a/docs/adr/0007-prototype-shadow-dom-style-isolation.md +++ b/docs/adr/0007-prototype-shadow-dom-style-isolation.md @@ -148,14 +148,14 @@ dismissal and restore focus into the remaining parent overlay. Either nested-overlay teardown order also works with the current architecture. Separate component shadow roots own distinct portal containers, but do not isolate Radix outside interaction: opening a peer popover dismisses the existing peer across -the same or separate roots. A verse action popover does not restore final focus -after its nested dialog and then the popover close, and rapid dialog close/reopen -also loses final focus restoration. +the same or separate roots. Nested dialog-to-popover dismissal restores focus in +order, and final dismissal returns to the original outside control. A dialog +that closes and reopens during retained exit presence also preserves and restores +its original opener; disconnected or cross-tree restore targets are ignored. These observations do not select or design production overlay coordination. -YPE-5356 owns deciding whether and how to support concurrent peers and exact -final focus restoration. The detailed Chromium evidence and remaining -validation live in the rollout plan. +YPE-5356 owns deciding whether and how to support concurrent peers. The detailed +Chromium evidence and remaining validation live in the rollout plan. Radix's development-only relationship checks can also emit warnings for valid IDs inside a shadow root because those checks query the document rather than diff --git a/docs/shadow-dom-isolation-plan.md b/docs/shadow-dom-isolation-plan.md index dfd3b97d..6f241554 100644 --- a/docs/shadow-dom-isolation-plan.md +++ b/docs/shadow-dom-isolation-plan.md @@ -24,17 +24,17 @@ This is a working plan, not approval for package-wide rollout. - Nested and concurrent overlays within and across component shadow roots were exercised through the production `ShadowRootHost` seam (YPE-5355). The Shadow DOM ADR records the architectural boundary; the results below record the - supported contract and peer-dismissal and final focus-restoration gaps. Runtime - changes remain YPE-5356. + supported contract and the remaining peer-dismissal gap. Runtime changes remain + YPE-5356. ## Nested and concurrent overlay evidence | Scenario | Result | Evidence and limitation | | --- | --- | --- | -| Verse action popover opens the highlights permission dialog | Partially supported | Both render in the shadow-local top layer. The dialog receives focus and the wrapper is inert. A first Escape closes only the dialog and restores focus inside the popover; a second closes the popover. Chromium spike observation found no final focus restoration. Automated evidence covers the stable dismissal, containment, and teardown contract without requiring that known gap to remain. | +| Verse action popover opens the highlights permission dialog | Supported | Both render in the shadow-local top layer. The dialog receives focus and the wrapper is inert. A first Escape closes only the dialog and restores focus inside the popover; a second closes the popover and restores the original outside control. | | Dialog contains a popover | Supported | The popover is interactive and receives focus. Escape closes it first and restores its trigger while the dialog remains modal. A second Escape closes the dialog and restores its opener. | | Two independent popovers in the same or separate roots | Unsupported as concurrent peers | Chromium spike observation found that opening a popover dismisses an existing peer through Radix outside interaction, including when the peers use separate component shadow roots. Automated evidence covers the stable contract that the new peer receives focus, remains non-modal, tears down cleanly, and that separate roots remain usable after primary teardown. | -| Rapid dialog close/reopen during exit | Unsupported | The first dialog remains connected in its closed state when it is reopened. The reopened dialog receives focus and the wrapper remains inert. Chromium spike observation found that final dismissal loses the opener; automated evidence requires safe modal release without locking in that focus loss. | +| Rapid dialog close/reopen during exit | Supported | The first dialog remains connected in its closed state when it is reopened. The reopened dialog receives focus and the wrapper remains inert. Final dismissal releases the modal state and restores the original opener. Disconnected or cross-tree restore targets are ignored. | Separate `ShadowRootHost` instances use different portal containers and tear those containers down independently. This provides lifecycle isolation, not @@ -55,7 +55,7 @@ open. | Portal lifecycle | Unit and browser coverage exercise lazy creation, exit-animation retention, cleanup, immediate reopen behavior, and the direct-Radix `VerseActionPopover` consumer. | Validated for shared primitives and the known bypass | Repeat the consumer audit when adding another direct overlay primitive. | | Dialog relationships | Chromium resolves title and description relationships inside the component tree. | Validated in Chromium | Verify announcements with real assistive technology. | | Dialog keyboard containment | Browser coverage exercises initial focus, programmatic escape redirection, forward and reverse traversal, radio-group collapsing, negative `tabindex`, and wraparound. | Validated in Chromium | Expand the browser and assistive-technology matrix. | -| Dialog modal lifetime | Coverage verifies inert background content while open and through staggered Content and Overlay exit animations. YPE-5355 also exercises both unmount orders for overlapping popover and dialog exits. | Validated for order-independent teardown | YPE-5356 owns peer concurrency across component roots and final focus-restoration gaps. Expand browser and assistive-technology coverage. | +| Dialog modal lifetime | Coverage verifies inert background content while open and through staggered Content and Overlay exit animations. YPE-5355 also exercises both unmount orders for overlapping popover and dialog exits. | Validated for order-independent teardown | YPE-5356 owns peer concurrency across component roots. Expand browser and assistive-technology coverage. | | Dialog dismissal and restoration | Coverage exercises Escape, backdrop click, full-viewport hit testing, overlay-only focus, and restoration after both modal nodes unmount. | Validated in Chromium | Verify real screen-reader and cross-browser behavior. | | Consumer form participation | Chromium coverage verifies that a light-DOM form does not own or serialize a native control inside an SDK shadow root. | Unsupported across tree scopes | Use an explicit component contract if a rollout target requires outer-form participation. | | Consumer labels and ARIA ID references | Chromium coverage verifies that external native labels, `aria-labelledby`, and `aria-describedby` relationships do not resolve to controls inside the root. | Unsupported across tree scopes | Keep relationships in one tree scope; verify real assistive technology separately. | @@ -84,10 +84,9 @@ decision. YPE-5356 owns whether and how to implement that coordination. - Apply [ADR 0007's client-only SSR and hydration contract](adr/0007-prototype-shadow-dom-style-isolation.md#ssr-and-hydration-contract) per rollout component. YPE-5356 decides whether its first-paint, layout, and no-JavaScript limitations are acceptable for that component. -- Resolve the YPE-5355 peer-dismissal and final focus-restoration gaps before - shipping nested and concurrent overlays (YPE-5356). The decision must consider - trigger-time peer dismissal as well as overlay order and restore targets; - ADR 0007 records the gaps but does not select a coordination design. +- Resolve the YPE-5355 peer-dismissal gap before shipping concurrent peer + overlays (YPE-5356). The decision must consider trigger-time peer dismissal + and overlay order; ADR 0007 records the gap but does not select a coordination design. Cross-browser and assistive-technology coverage still remain. - Keep the YPE-5400 custom-property contract and compiled-stylesheet prevention guard green as component styles change. The audit below closes the known diff --git a/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx b/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx index 446a955b..be8c3aa8 100644 --- a/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx +++ b/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx @@ -369,7 +369,7 @@ function manualWorkflow( export const PopoverOpensDialog = manualWorkflow( 'Popover opens dialog — manual workflow', - 'Focus Prior control, click First verse, choose a highlight color, then press Escape twice. Focus returns inside the popover after the first Escape; after the second, confirm it does not return to Prior control.', + 'Focus Prior control, click First verse, choose a highlight color, then press Escape twice. Confirm focus returns inside the popover after the first Escape and to Prior control after the second.', ); export const DialogContainsPopover = manualWorkflow( 'Dialog contains popover — manual workflow', @@ -381,11 +381,11 @@ export const TwoIndependentOverlays = manualWorkflow( ); export const RapidCloseReopenDuringExit = manualWorkflow( 'Rapid close/reopen during exit — manual workflow', - 'Focus Run rapid close/reopen and press Enter once. The dialog opens, closes, and reopens during retained exit presence. Press Escape after it reopens; confirm the page unlocks but focus does not return to the Run button.', + 'Focus Run rapid close/reopen and press Enter once. The dialog opens, closes, and reopens during retained exit presence. Press Escape after it reopens; confirm the page unlocks and focus returns to the Run button.', { enableRapidReopen: true }, ); -/** YPE-5355 stable regression evidence; unsupported observations remain in the decision docs. */ +/** YPE-5355 regression evidence with YPE-5889 final-focus coverage. */ export const PopoverOpensDialogEvidence: Story = { tags: ['!dev'], play: async ({ canvasElement, step }) => { @@ -453,8 +453,7 @@ export const PopoverOpensDialogEvidence: Story = { await waitFor(() => { void expect(topLayer.querySelector('[data-slot="verse-action-popover"]')).toBeNull(); void expect(topLayer.matches(':popover-open')).toBe(false); - const focused = root.activeElement; - void expect(focused === null || !topLayer.contains(focused)).toBe(true); + void expect(root.activeElement).toBe(priorControl); }); }); @@ -701,7 +700,7 @@ export const TwoIndependentOverlaysEvidence: Story = { }, }; -/** YPE-5355 stable regression evidence; final-focus observations remain in the docs. */ +/** YPE-5355 lifecycle evidence with YPE-5889 final-focus coverage. */ export const RapidCloseReopenDuringExitEvidence: Story = { tags: ['!dev'], args: { enableRapidReopen: true }, @@ -765,9 +764,99 @@ export const RapidCloseReopenDuringExitEvidence: Story = { void expect(getPermissionDialog(topLayer)).toBeNull(); void expect(contentWrapper.inert).toBe(false); void expect(topLayer.matches(':popover-open')).toBe(false); - const focused = root.activeElement; - void expect(focused === null || !topLayer.contains(focused)).toBe(true); + void expect(root.activeElement).toBe(runRapidCloseReopen); + }); + }); + } finally { + exitAnimationStyle.remove(); + } + }, +}; + +export const RapidCloseReopenIgnoresCrossTreeOpenerEvidence: Story = { + tags: ['!dev'], + args: { enableRapidReopen: true }, + play: async ({ canvasElement, step }) => { + const { contentWrapper, root } = await getPrimaryHarness(canvasElement); + const runRapidCloseReopen = await waitForElement( + root, + '[data-testid="run-rapid-close-reopen"]', + 'rapid close/reopen control not rendered', + ); + const secondaryIsland = await waitForElement( + canvasElement, + '[data-testid="secondary-island"]', + 'secondary island not rendered', + ); + const secondaryRoot = await waitFor(() => requireShadowRoot(secondaryIsland)); + const exitAnimationStyle = installUnequalExitDurations(root, { + dialog: 400, + overlay: 800, + popover: 180, + }); + let topLayer!: HTMLElement; + + try { + await step('Reopen the dialog before its retained exit completes', async () => { + await userEvent.click(runRapidCloseReopen); + topLayer = await waitForElement( + root, + '[data-yv-shadow-local-overlay]', + 'shadow-local top layer not created', + ); + const firstDialog = await waitFor(() => { + const dialog = getPermissionDialog(topLayer); + if (!dialog) throw new Error('permission dialog did not open'); + return dialog; + }); + await waitFor(() => void expect(firstDialog).toHaveAttribute('data-state', 'closed')); + await waitFor(() => { + const dialog = getPermissionDialog(topLayer); + if (!dialog || dialog.getAttribute('data-state') === 'closed') { + throw new Error('permission dialog did not reopen during exit'); + } + void expect(contentWrapper.inert).toBe(true); + }); + }); + + await step('Ignore the opener after it moves to another shadow root', async () => { + secondaryRoot.append(runRapidCloseReopen); + void expect(runRapidCloseReopen.getRootNode()).toBe(secondaryRoot); + + await userEvent.keyboard('{Escape}'); + await waitFor(() => { + void expect(getPermissionDialog(topLayer)).toBeNull(); + void expect(contentWrapper.inert).toBe(false); + void expect(topLayer.matches(':popover-open')).toBe(false); + }); + void expect(secondaryRoot.activeElement).not.toBe(runRapidCloseReopen); + }); + + await step('Ignore the opener after it disconnects', async () => { + contentWrapper.append(runRapidCloseReopen); + await userEvent.click(runRapidCloseReopen); + const firstDialog = await waitFor(() => { + const dialog = getPermissionDialog(topLayer); + if (!dialog) throw new Error('permission dialog did not reopen for disconnected target'); + return dialog; + }); + await waitFor(() => void expect(firstDialog).toHaveAttribute('data-state', 'closed')); + await waitFor(() => { + const dialog = getPermissionDialog(topLayer); + if (!dialog || dialog.getAttribute('data-state') === 'closed') { + throw new Error('permission dialog did not rapidly reopen for disconnected target'); + } + }); + + runRapidCloseReopen.remove(); + void expect(runRapidCloseReopen.isConnected).toBe(false); + await userEvent.keyboard('{Escape}'); + await waitFor(() => { + void expect(getPermissionDialog(topLayer)).toBeNull(); + void expect(contentWrapper.inert).toBe(false); + void expect(topLayer.matches(':popover-open')).toBe(false); }); + void expect(root.activeElement).not.toBe(runRapidCloseReopen); }); } finally { exitAnimationStyle.remove(); diff --git a/packages/ui/src/components/ui/use-shadow-dialog-focus.ts b/packages/ui/src/components/ui/use-shadow-dialog-focus.ts index 57d7711c..eaadac16 100644 --- a/packages/ui/src/components/ui/use-shadow-dialog-focus.ts +++ b/packages/ui/src/components/ui/use-shadow-dialog-focus.ts @@ -120,7 +120,7 @@ export function useShadowDialogFocus({ capturedRestoreFocusRef.current = false; return; } - if (!container || capturedRestoreFocusRef.current) return; + if (!container || capturedRestoreFocusRef.current || restoreFocusRef.current) return; const shadowRoot = getOwnShadowRoot(container); const activeElement = shadowRoot diff --git a/packages/ui/src/lib/shadow-root-host.tsx b/packages/ui/src/lib/shadow-root-host.tsx index cf561be9..1f7d05df 100644 --- a/packages/ui/src/lib/shadow-root-host.tsx +++ b/packages/ui/src/lib/shadow-root-host.tsx @@ -227,6 +227,12 @@ export function ShadowRootHost({ children, portalStrategy }: ShadowRootHostProps [hideIfIdle], ); + const canRestoreFocus = useCallback( + (target: HTMLElement): boolean => + target.isConnected && target.getRootNode() === shadowRootRef.current, + [], + ); + const setModalPresent = useCallback( (instanceId: string, present: boolean): void => { const ids = presentModalIdsRef.current; @@ -239,9 +245,9 @@ export function ShadowRootHost({ children, portalStrategy }: ShadowRootHostProps const target = pendingFocusTargetRef.current; pendingFocusTargetRef.current = null; - if (target?.isConnected) target.focus(); + if (target && canRestoreFocus(target)) target.focus(); }, - [], + [canRestoreFocus], ); const restoreFocusWhenModalReleased = useCallback((target: HTMLElement): void => { @@ -250,8 +256,8 @@ export function ShadowRootHost({ children, portalStrategy }: ShadowRootHostProps return; } - if (target.isConnected) target.focus(); - }, []); + if (canRestoreFocus(target)) target.focus(); + }, [canRestoreFocus]); const getLastFocusedElement = useCallback( (): HTMLElement | null => lastFocusedElementRef.current, From a7cfe2850b2e62e0a5dd12d0e15602c677e78ef2 Mon Sep 17 00:00:00 2001 From: Austin Date: Wed, 16 Sep 2026 16:17:03 -0500 Subject: [PATCH 2/2] fix(ui): reset prevented shadow focus restoration YPE-5889 --- ...oduction-seam.shadow-isolation.stories.tsx | 28 +++++---- packages/ui/src/components/ui/dialog.test.tsx | 59 +++++++++++++++++++ .../components/ui/use-shadow-dialog-focus.ts | 6 +- 3 files changed, 81 insertions(+), 12 deletions(-) diff --git a/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx b/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx index be8c3aa8..98dd4914 100644 --- a/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx +++ b/packages/ui/src/components/shadow-overlay-production-seam.shadow-isolation.stories.tsx @@ -1,7 +1,7 @@ import type { Meta, StoryObj } from '@storybook/react-vite'; import { http, HttpResponse } from 'msw'; import { useCallback, useEffect, useRef, useState } from 'react'; -import { expect, userEvent, waitFor } from 'storybook/test'; +import { expect, spyOn, userEvent, waitFor } from 'storybook/test'; import { ShadowRootHost } from '../lib/shadow-root-host'; import { requireShadowRoot } from '../test/dom-stubs'; import { HighlightPermissionDialog } from './highlight-permission-dialog'; @@ -773,7 +773,7 @@ export const RapidCloseReopenDuringExitEvidence: Story = { }, }; -export const RapidCloseReopenIgnoresCrossTreeOpenerEvidence: Story = { +export const RapidCloseReopenIgnoresInvalidOpenerEvidence: Story = { tags: ['!dev'], args: { enableRapidReopen: true }, play: async ({ canvasElement, step }) => { @@ -848,15 +848,21 @@ export const RapidCloseReopenIgnoresCrossTreeOpenerEvidence: Story = { } }); - runRapidCloseReopen.remove(); - void expect(runRapidCloseReopen.isConnected).toBe(false); - await userEvent.keyboard('{Escape}'); - await waitFor(() => { - void expect(getPermissionDialog(topLayer)).toBeNull(); - void expect(contentWrapper.inert).toBe(false); - void expect(topLayer.matches(':popover-open')).toBe(false); - }); - void expect(root.activeElement).not.toBe(runRapidCloseReopen); + const focus = spyOn(runRapidCloseReopen, 'focus'); + try { + runRapidCloseReopen.remove(); + void expect(runRapidCloseReopen.isConnected).toBe(false); + await userEvent.keyboard('{Escape}'); + await waitFor(() => { + void expect(getPermissionDialog(topLayer)).toBeNull(); + void expect(contentWrapper.inert).toBe(false); + void expect(topLayer.matches(':popover-open')).toBe(false); + }); + void expect(focus).not.toHaveBeenCalled(); + void expect(root.activeElement).not.toBe(runRapidCloseReopen); + } finally { + focus.mockRestore(); + } }); } finally { exitAnimationStyle.remove(); diff --git a/packages/ui/src/components/ui/dialog.test.tsx b/packages/ui/src/components/ui/dialog.test.tsx index a58b0826..939fd9da 100644 --- a/packages/ui/src/components/ui/dialog.test.tsx +++ b/packages/ui/src/components/ui/dialog.test.tsx @@ -1,10 +1,50 @@ import { render, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import { useState, type ReactNode } from 'react'; import { describe, expect, it, vi } from 'vitest'; import { ShadowRootHost } from '@/lib/shadow-root-host'; import { requireShadowRoot } from '@/test/dom-stubs'; import { Dialog, DialogContent, DialogTitle } from './dialog'; +function PreventedCloseAutoFocusHarness(): ReactNode { + const [open, setOpen] = useState(false); + const [preventCloseAutoFocus, setPreventCloseAutoFocus] = useState(false); + + return ( + + + + + { + if (preventCloseAutoFocus) event.preventDefault(); + }} + > + Title + + + + + ); +} + describe('Dialog shadow portal coordination', () => { it('preserves light-DOM state and ref behavior without changing overlay focusability', async () => { const cleanup = vi.fn(); @@ -109,4 +149,23 @@ describe('Dialog shadow portal coordination', () => { }); expect(wrapper?.inert).toBe(false); }); + + it('captures a new opener after a consumer prevents close autofocus', async () => { + const user = userEvent.setup(); + const { container } = render(); + const shadowRoot = requireShadowRoot(container); + const [firstOpener, secondOpener] = Array.from(shadowRoot.querySelectorAll('button')); + + await user.click(firstOpener!); + await waitFor(() => expect(shadowRoot.querySelector('[role="dialog"]')).not.toBeNull()); + await user.keyboard('{Escape}'); + await waitFor(() => expect(shadowRoot.querySelector('[role="dialog"]')).toBeNull()); + + await user.click(secondOpener!); + await waitFor(() => expect(shadowRoot.querySelector('[role="dialog"]')).not.toBeNull()); + await user.keyboard('{Escape}'); + await waitFor(() => expect(shadowRoot.querySelector('[role="dialog"]')).toBeNull()); + + expect(shadowRoot.activeElement).toBe(secondOpener); + }); }); diff --git a/packages/ui/src/components/ui/use-shadow-dialog-focus.ts b/packages/ui/src/components/ui/use-shadow-dialog-focus.ts index eaadac16..aadf643c 100644 --- a/packages/ui/src/components/ui/use-shadow-dialog-focus.ts +++ b/packages/ui/src/components/ui/use-shadow-dialog-focus.ts @@ -134,7 +134,11 @@ export function useShadowDialogFocus({ const onCloseAutoFocus = React.useCallback( (event: Event): void => { - if (event.defaultPrevented || container === undefined) return; + if (container === undefined) return; + if (event.defaultPrevented) { + restoreFocusRef.current = null; + return; + } event.preventDefault(); const restoreFocusTo = restoreFocusRef.current;