feat(ui): prototype shared shadow overlay for portals (YPE-5138) - #350
feat(ui): prototype shared shadow overlay for portals (YPE-5138)#350abharms wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 1ef7d48 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
cameronapak
left a comment
There was a problem hiding this comment.
Spec
Review of 1ef7d48 vs journey-to-the-shadow-dom. Spec is YPE-5138 (spike) under YPE-4113 (host CSS must not distort Bible UI). No GEAR tickets exist.
ShadowRootHost
component shadow root
trigger
ShadowPortalContext
shared overlay shadow root (document.body)
Radix popover / dialog
issue (must-fix): Public BibleLanguagePickerContent TabsList uses var(--yv-spacing). The SDK does not define that token. Tailwind still uses --spacing. The host-spacing story can pass because the invalid var() is ignored.
suggestion (worth): PopoverContent now sets max-h-[min(66svh,var(--radix-popover-content-available-height))] for every consumer. The ticket asked to route isolated popovers. This height change is extra.
suggestion (worth): A patch changeset ships overlay routing and the global height change. ADR 0005 says this is a checkpoint, not a release.
nitpick: Scrolling is manual Chromium only. The ADR already says that.
Pass: shared overlay, conditional portals, public picker not auto-isolated, Chromium proofs, AT documented as unproven.
Standards
No hard documented-standard break.
suggestion (worth): KeyboardFocusAndEscapeCrossShadowRoots and PopoverDialogSemanticsAndFocusAcrossShadowRoots repeat one play script. docs/testing.md prefers fewer, longer workflow tests.
suggestion (worth): Every ShadowRootHost eagerly creates the overlay root, including leaves that never portal.
nitpick: Overlay stylesheet adopt is copied from the component host. Custom tab trap is thinner than Radix. ADR says Radix focus does not cross shadows.
Summary
- Standards: 0 must-fix, 2 worth, 3 nits. Worst: two play stories repeat one keyboard workflow.
- Spec: 1 must-fix, 2 worth, 1 nit. Worst: undefined
--yv-spacingon a public picker.
Code Reviewer bot, sent on behalf of Cam.
| onValueChange={setLanguageTab} | ||
| > | ||
| <TabsList className="yv:mx-4 yv:w-[calc(100%-4*var(--spacing)*2)]"> | ||
| <TabsList className="yv:mx-4 yv:w-[calc(100%-4*var(--yv-spacing)*2)]"> |
There was a problem hiding this comment.
issue: --yv-spacing is not an SDK token.
BibleLanguagePickerContent is a public export. The TabsList width used var(--spacing) (Tailwind theme). This change points at var(--yv-spacing), which global.css never defines.
An invalid var() drops the width declaration. HostSpacingCustomPropertyDoesNotAffectOverlay can pass for that reason. Host --spacing no longer reaches a designed token.
YPE-4113: partners must see the designed Bible UI. Restore a real token, or keep --spacing and isolate it inside the overlay stylesheet.
Code Reviewer bot, sent on behalf of Cam.
| collisionPadding={16} | ||
| className={cn( | ||
| 'yv:bg-popover yv:text-popover-foreground yv:data-[state=open]:animate-in yv:data-[state=closed]:animate-out yv:data-[state=closed]:fade-out-0 yv:data-[state=open]:fade-in-0 yv:data-[state=closed]:zoom-out-95 yv:data-[state=open]:zoom-in-95 yv:data-[side=bottom]:slide-in-from-top-2 yv:data-[side=left]:slide-in-from-right-2 yv:data-[side=right]:slide-in-from-left-2 yv:data-[side=top]:slide-in-from-bottom-2 yv:z-50 yv:origin-(--radix-popover-content-transform-origin) yv:outline-hidden yv:grid yv:grid-rows-[auto_1fr_auto] yv:p-0 yv:h-full yv:max-h-[66svh] yv:max-sm:max-w-[calc(100vw-2rem)] yv:w-sm yv:sm:max-w-sm yv:overflow-hidden yv:rounded-2xl yv:border-0 yv:shadow-lg', | ||
| 'yv:bg-popover yv:text-popover-foreground yv:data-[state=open]:animate-in yv:data-[state=closed]:animate-out yv:data-[state=closed]:fade-out-0 yv:data-[state=open]:fade-in-0 yv:data-[state=closed]:zoom-out-95 yv:data-[state=open]:zoom-in-95 yv:data-[side=bottom]:slide-in-from-top-2 yv:data-[side=left]:slide-in-from-right-2 yv:data-[side=right]:slide-in-from-left-2 yv:data-[side=top]:slide-in-from-bottom-2 yv:z-50 yv:origin-(--radix-popover-content-transform-origin) yv:outline-hidden yv:grid yv:grid-rows-[auto_1fr_auto] yv:p-0 yv:max-h-[min(66svh,var(--radix-popover-content-available-height))] yv:max-sm:max-w-[calc(100vw-2rem)] yv:w-sm yv:sm:max-w-sm yv:overflow-hidden yv:rounded-2xl yv:border-0 yv:shadow-lg', |
There was a problem hiding this comment.
suggestion: This max-h applies to every Radix popover, not only isolated ones.
YPE-5138 asked to route isolated popovers and validate clipping. The shared primitive now uses min(66svh, --radix-popover-content-available-height) and drops h-full.
If the height change is required for the spike, say so in the ADR. If it is not, keep it off the default primitive.
Code Reviewer bot, sent on behalf of Cam.
| }, | ||
| }; | ||
|
|
||
| export const KeyboardFocusAndEscapeCrossShadowRoots: Story = { |
There was a problem hiding this comment.
suggestion: KeyboardFocusAndEscapeCrossShadowRoots and PopoverDialogSemanticsAndFocusAcrossShadowRoots repeat one play script.
docs/testing.md prefers fewer, longer workflow tests. Keep the extra aria-* / role="dialog" asserts in one play.
Code Reviewer bot, sent on behalf of Cam.
|
thought: If this spike started again with a clean slate, I would not rebuild the shared overlay shadow. The #350 split is the pain. The trigger lives in one shadow root. The overlay lives in another. Chromium then cannot resolve Shape I would buildOne shadow tree per island. Keep the popover in that same tree. Use the top layer to escape clipping. - component shadow → shared overlay shadow → Radix Portal
+ component shadow → top-layer node in the same rootWhat I would keepI would keep Shadow DOM. Host unlayered CSS still wins over cascade layers and
What I would drop
TokensI would own theme tokens on GuessRadix portal-to-body is the part to replace, not Radix itself. Native This remains a spike checkpoint, not a release path. Grok Bot Code Reviewer, sent on behalf of Cameron. |
Summary
Scope
This is an architecture spike, not a BibleVersionPicker rollout. The public BibleVersionPicker export is unchanged; the ShadowRootHost wrapper exists only in the spike stories.
Findings
The Chromium proof covers host-CSS isolation, ancestor and viewport clipping, internal scrolling, positioning, focus, Escape and outside-click dismissal, shared-overlay reuse, and actual SignInDialog composition. Cross-root aria-controls resolution and real VoiceOver/NVDA behavior remain explicit follow-up gates.
Known limitation
If the shared overlay host is externally removed, new isolated mounts create a replacement root. Already-mounted providers retain their original portal container. Reconnecting the detached root was explored but caused test/document-reset isolation problems.
Verification
The root pnpm test command could not complete locally because the core test setup requires YVP_API_HOST, which was not available. The complete UI suite passed independently.
Greptile Summary
The PR introduces a shared styled shadow overlay for isolated Radix portals while retaining document-level portal behavior outside
ShadowRootHost.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Host[ShadowRootHost] --> ComponentRoot[Component Shadow Root] ComponentRoot --> Trigger[Dialog or Popover Trigger] ComponentRoot --> Context[ShadowPortalContext] Context --> SharedOverlay[Shared Document-Level Overlay Shadow Root] SharedOverlay --> Portal[Radix Portal Content] SharedOverlay --> Styles[SDK Shadow Styles] Outside[Components outside ShadowRootHost] --> Body[document.body portal fallback]Reviews (3): Last reviewed commit: "fix(ui): support ES2020 dialog build" | Re-trigger Greptile