fix(web): make share image previews interactive - #1229
Conversation
There was a problem hiding this comment.
Findings
- One Major inline finding.
Questions
- None.
Summary
Review mode: initial
The image lightbox bypasses the existing Radix modal stack, breaking keyboard focus and Escape behavior inside the share preview.
Testing
- Repository CI
test: passed. - Not run locally (automation; PR code was not executed).
- Add Playwright coverage for Tab focus entering the lightbox and Escape closing only the lightbox.
HAPI Bot
| </div> | ||
| </div> | ||
| </div> | ||
| {previewImage ? createPortal( |
There was a problem hiding this comment.
[MAJOR] Mount the lightbox as a nested Radix dialog
The share preview is already a modal DialogContent (web/src/components/ui/dialog.tsx:20), so Radix traps focus inside that content and treats it as the active dismissable layer. This raw portal mounts a plain role="dialog" under document.body, outside the focus scope: keyboard users cannot Tab into the zoom/close controls, while Escape is still handled by the outer share dialog and closes the share flow instead of only the image viewer.
Suggested fix:
// ZoomableLightbox.tsx
import * as DialogPrimitive from "@radix-ui/react-dialog"
return (
<DialogPrimitive.Root open={open} onOpenChange={(next) => {
if (!next) onClose()
}}>
<DialogPrimitive.Portal>
<DialogPrimitive.Content
className="fixed inset-0 z-50 h-[100dvh] w-full bg-black text-white"
aria-label={ariaLabel}
>
{/* current viewport and toolbar */}
</DialogPrimitive.Content>
</DialogPrimitive.Portal>
</DialogPrimitive.Root>
)
// ShareTurnDialog.tsx: render <ZoomableLightbox /> directly; remove createPortal().Also let the nested Radix dialog own Escape dismissal (remove the lightbox window-handler Escape branch), and cover Tab/Escape behavior in the Playwright test.
There was a problem hiding this comment.
Fixed in eee3671. The lightbox now mounts as a nested Radix Dialog, lets Radix own Escape dismissal and focus trapping, and no longer uses a raw createPortal from the share dialog. The Playwright scenario now verifies that Tab focus remains inside the lightbox and that Escape closes only the lightbox while leaving the share preview open. bun typecheck, bun run test:web, bun run build:web, and all 8 share-turn Playwright scenarios pass.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No issues found in the latest full diff. The previous modal focus/Escape finding is resolved by mounting the shared lightbox as a nested Radix dialog and adding keyboard coverage. Residual risk: the shared ZoomableLightbox change also affects the existing Mermaid viewer, while this PR’s browser coverage exercises only the nested share-preview path.
Testing
- Not run locally (automation; PR code was not executed).
- GitHub test check pending at review time.
- Suggested: retain or add browser coverage for opening, tabbing within, and closing the standalone Mermaid lightbox.
HAPI Bot
Summary
Testing
bun typecheckbun run test:webbun run build:webPLAYWRIGHT_WEB_PORT=5196 bunx playwright test e2e/share-turn.spec.tsThe share-turn Playwright coverage verifies: