githubnext/chopinPublic

Let's compose rich plans together

Make the child document overlay a single sheet #339

MergedMaggieAppletonmerged 4 commits intomainfromrefs/pull/339/head
  1. MA

    ## Why Escape closed the child document even while typing in its editor. The overlay also showed a card inside a card (inner frame with a square-cornered outline and 12px inset, plus a focus ring), and the parent workspace was scaled and blurred behind it, which is costly on a full editor.

    ## What changed - Escape inside an input, textarea, select, contenteditable, menu, listbox, dialog or combobox now only blurs that field. A second Escape closes the child. - The child workspace frame now fills the sheet edge to edge (no margin, outline or radius); the sheet's own edge, radius and `--shadow-overlay` are the single frame. The programmatic focus ring on the sheet is removed. - The parent blur and scale are replaced with a calm opacity dim (a pseudo-element, 12% text-primary). - Enter and exit use the modal motion tokens (`--modal-open-dur`, `--modal-close-dur`, `--modal-ease`, `--modal-scale`) on transform and opacity only; reduced motion keeps a fade.

    ## Screenshots Before: ![before](./before-desktop-open.png) After: ![after](./after-desktop-open.png) Phone after: ![phone after](./after-phone-open.png)

    ## Testing - `bun run types` passes. Browser check against a seeded child (fake GitHub, port 8818): Escape in the child editor keeps it open, the second Escape returns to the parent. Before: the first Escape closed it. - `bun run ci` fails only the design contract (see below). Local e2e is suspended for this run; CI runs the browser suite.

    ## Needs a design-contract review `scripts/design-contract/exceptions/preserved-values.json` has five now-stale `apps/web/src/navigation.css` motion entries (the 240ms/170ms transitions and animations this PR removes). They should be deleted from that exception entry; I did not edit the exception file. No new literal values were added.

    🤖 Generated with [Claude Code](https://claude.com/claude-code)

    ![before](https://github.com/user-attachments/assets/1998dde9-7392-4a7a-913b-7fb846ae0392)

    ![after](https://github.com/user-attachments/assets/bd11bd48-fce1-4890-9d81-bf7dd003ba77)

    ![phone after](https://github.com/user-attachments/assets/a253a964-b053-4c36-a732-d8fa7b0ecf35)

  2. MA
    Author

    **Review:** Changes needed

    The design and behaviour are right; the PR breaks two e2e tests that still assert the old blur/scale, and adds no test for the new Escape behaviour.

    Checked in a browser (seeded child, headless Chromium, desktop 1440x900 and phone 390x844, against main on 8795):

    - **Escape:** caret in child prose, first Escape blurs and the child stays open; second closes. Escape in an open slash menu closes only the menu. Before, the first Escape closed the child. - **Sheet:** one surface now. The child frame has margin 0, radius 0, no border, no outline, no shadow, and the sheet edge, radius and overlay shadow are the only frame. No more card-in-card. - **Dim:** 12% text-primary over the parent, no filter. Calm and readable, and the parent strip stays legible. Fine as is. - **Motion (rAF-sampled):** enter ~250ms, exit ~150ms, translateY 8px and scale 0.96 to 1, dim opacity in lockstep with the sheet. Symmetric, no stray frame. Matches modal tokens. - **Phone:** full-bleed, no radius or border, dim present, Escape semantics unchanged.

    **Must fix** 1. `e2e/research-child-surface.e2e.ts:135-136`: asserts `.workspace-frame` has `filter: blur(3px)` and `opacity: 0.68`. Both are gone by design. Assert the new behaviour instead (filter `none`, `::after` dim visible, e.g. via `getComputedStyle(parent, "::after").opacity === "1"`). CI test "a parent-owned child keeps the parent chrome and nested geometry" fails here. 2. `e2e/research-child-surface.e2e.ts:805-806`: reduced-motion test expects the parent frame `transition-property: none`. The rule now targets `.anchored-child-parent::after`, so the frame reports `all`. Move the assertion to the `::after` pseudo. CI test "a direct compact child fills the canvas and reduces motion to a crossfade" fails here. 3. Add an e2e for the new behaviour: focus the child editor, press Escape, expect the surface still visible and the editor blurred; press Escape again, expect it closed. Optionally a menu case.

    **Nits (non-blocking)** - `document-workspace-host.tsx:~268`: after `target.blur()`, focus drops to `body`. A keyboard user's next Tab restarts from the top of the page. Consider moving focus to the sheet (`tabIndex=-1`, already focused on open) instead of a bare blur. - `[role="dialog"]` in `DISMISSIBLE_TARGET` is safe today (the sheet is `role=region`), but would make Escape never close the child if the sheet ever becomes a dialog. A short comment would protect it.

    **CI state:** `bun run ci` fails only on the 5 stale `navigation.css` design-exception entries, which the PR body already flags (not touching hashes, acceptable). `container` passes. `e2e` fails on exactly the two tests above (369 passed, 2 failed).

  3. MA
    Author

    Review addressed: e2e assertions moved to the ::after dim, added the Escape sequence e2e (slash menu, then sheet focus, then close), first Escape now focuses the sheet instead of blurring, and a comment explains [role=dialog]. e2e and container pass. The validation job fails only on the five stale navigation.css entries in preserved-values.json (see PR body).

  4. MaggieAppletonhead-ref-force-pushed 60619839f345last month on GitHub

  5. MaggieAppletonmerged d50d33ffa433last month on GitHub

  6. MaggieAppletonclosedlast month on GitHub

  7. MaggieAppletonhead-ref-deletedlast month on GitHub