githubnext/chopinPublic

Let's compose rich plans together

Fix touch ergonomics on phones #299

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

    ## Integration and validation

    Keep current proper-name tooltip casing and the 16px coarse-pointer text floor while adding keyboard-only focus tooltips, invisible touch targets, content-fitting comment sheet detents and visible sheet titles. Retain the resize toggle and current frame behavior.

    Reviewed the comment-layer title-only change before preserving its geometry boundary pin. Formatting/lint/design checks, types, nine focused tests and all 15 responsive-comment/interface browser tests pass locally. Rebased onto current main; final required CI running.

    <details> <summary>Original proposal and screenshots</summary>

    ## Why On phones, tapping opens the sidebar or a comment and a tooltip ("Close Sidebar", "Resize Sheet") appears from programmatic focus. The comment sheet is a fixed height, leaving blank space under a short thread, and has no title. Small controls (send reply 28px, Dismiss 24px tall) are under the 44px touch target.

    ## What changed - `icon-tooltip.tsx`: focus tooltips only when the target matches `:focus-visible` (same guard line as #284, so the two rebase trivially); hover tooltips never on coarse pointers. Pure `tooltipTrigger` is unit-tested. - Comment sheet: the medium detent now fits the content (20%-85% of the viewport), keeps the drag/resize handle and the 92% detent, and has a visible title row ("Comment", "N comments", "New comment") in text-sm weight 600. Reveal scrolling uses the real detent. - `theme.css`: one central coarse-pointer rule (in the components layer so utilities like `absolute` still win) grows an invisible 44px hit area around `btn-icon`, `btn-sm` and the sidebar action buttons without changing their visual size.

    ## Screenshots Before (left) / after (right), iPhone 13 emulation (touch, mobile):

    ![Sheet before and after](https://github.com/user-attachments/assets/0ad079bb-96bc-46b5-bc6d-7a8a4a58cf8a)

    ![Sidebar before and after](https://github.com/user-attachments/assets/ce763886-4913-4046-bf5d-a2fac9ab4df6)

    Measured hit areas after: send reply 28x28 -> 44x44, Dismiss 84x24 -> 86x44.

    ## Testing - `bun test packages/editor/src/comment-sheet.test.ts apps/web/src/icon-tooltip.test.ts`: pass - `bun run types`: pass; `bun run fix`: clean - `bun run ci`: fails only the design-contract review pins below - E2E left to CI (local suspended).

    ## Needs a design-contract review `packages/editor/src/comment-layer.tsx` (sha256 `0a9568e339e8d8b34d8100d2c68f2b528de9809f0c48f6e909bebb652b0b8a7b`) is pinned in `scripts/design-contract/exceptions/dynamic-editor.json` (dynamic owners at lines 165, 171, 220, 228, 684, 708). The edit only adds a `compactTitle` string passed to the sheet; no style or class logic, but lines shifted.

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

    </details>

  2. MA
    Author

    **Review:** Changes needed

    Tooltip logic, the 44px hit-area rule and the content-fit sheet all look right in a real touch context (iPhone-size, 390x844, touch; also 320x568, 430x932, 768x1024, 1024x768). One regression breaks e2e, and one overlap needs a z-order decision.

    **Blocking**

    1. **Dialog accessible name changed; 5 e2e tests fail** (`comment-sheet.tsx` ~L131, `<Drawer.Title className="plan-comment-sheet-title">{title ?? label}</Drawer.Title>`). The dialog is named by its `Drawer.Title`, which used to be the sr-only `label` ("Comment thread", "Orphaned comments", "New comment"). It is now "Comment" / "3 comments" / "New comment", so `getByRole("dialog", { name: "Comment thread" })` no longer matches. CI e2e failures, all this cause: - `responsive-comments.e2e.ts:9` (320x568 drawer) and `:44` (representative compact viewport) - `responsive-workspace.e2e.ts:593` (keyboard composer) - `sidecar.e2e.ts:1012` apply / dismiss a compact comment Fix: keep the accessible name stable. Either leave `Drawer.Title` sr-only with `label` and render the visible heading as a separate `<div aria-hidden>`, or give `Drawer.Popup` `aria-label={label}` and keep the visible title non-naming. Don't change the 7 test names. Name stability also matters to AT users, who currently lose "thread" vs "draft" vs "orphaned" context.

    2. **Hit-area overlap: code toggle steals the comment marker tap** (`theme.css` ~L655, enlarged `.plan-code-toggle`/`.btn-icon`). At 768x1024 and 1024x768 touch, the 44px `::after` of "Show source" (28px toggle at the code-block right edge) covers the centre of `.plan-comment-button` (44px margin button, 712..756 x 547..591 vs toggle box 699..743 x 532..576). Tapping the visual centre of the comment button hits "Show source" instead; with `::after` disabled the tap goes to the comment button (main behaviour). The two already overlapped visually on main (comment marker sits over the toggle), but now the toggle wins taps. Also visible at 390 as the marker covering the `<>` glyph. Fix: give `.plan-comment-button` (and `.plan-comment-orphan-button`) a z-index above in-block controls, or exclude `.plan-code-toggle` from the rule and only grow vertically in the code header.

    **Overlap audit (the key risk)** - I scanned every `.btn-icon/.btn-sm/.project-sidebar-*` after-box against every other control and tapped every control's visual centre via `touchscreen.tap` (click capture): - Selection toolbar (T B I S U <> link): not affected. Those are `.plan-menu-cell` (already 44px), no overlap, all taps land correctly (the right-most comment cell is clipped by overflow-x, pre-existing). - Table menu (Add/Remove/Move/Align) and table cells: not `btn-*`; fine. - Code-block header: language chip (46x24 / 70x24 -> 44 tall) and toggle (28 -> 44) are far apart horizontally; header is 37px so the box spills ~4px into the body and the margin above. No control stolen. Only issue is #2. - Decision card Discard/Save: already 44px tall, 6px gap, widths >=44, so no horizontal growth. Fine. - Composer send (44px already), header Document/Decisions tabs (83x24 and 113x24 -> 44 tall, widths unchanged; only vertical growth, no neighbours), sidebar actions (already 44px on phone), thread Dismiss (84x24 -> 44) and Send reply (28 -> 44): all inside the sheet, no neighbour stolen. - Small note: Dismiss's box (86x44) reaches the "Chat" bottom tab only behind the modal sheet, harmless. Recommendation if other dense toolbars appear later: clamp horizontal growth (`width: 100%`) and only grow vertically, or `min(44px, 100% + gap)`.

    **Other checks (pass)** - Desktop 1440x900: `data-plan-coarse-pointer` unset, `.btn-icon::after { content: none }`, hover tooltip shows ("Show Source"), unchanged. - Keyboard: Shift then `focus()` / Shift+Tab shows the tooltip (`:focus-visible` true); mouse-click then focus shows none; scroll re-shows for the keyboard-focused control. Touch tap on code toggle and sidebar button: no tooltip (previously the lingering "Close Sidebar" bug). - Sheet sizing (data-fit): 1-note thread 0.30 of 844 (0.49 at 568), draft sheet ~0.44, grabber toggles 0.30 <-> 0.92 and back, title row reads "Comment" / "New comment". Looks right and compact; the fit is the content, no blank space. - `bun test` for comment-sheet, icon-tooltip, theme: 36 pass. `bun run ci`: only the design-contract review pin on `comment-layer.tsx` (documented in the PR body), as allowed.

    **Minor** - `icon-tooltip.tsx` `focusIn`: `target.matches(":focus-visible")` has no try/catch, unlike `keyboardFocused()` (L58-L63). Share one helper for consistency. - `keyboardFocused()` passes a meaningless `coarse: false` into `tooltipTrigger("focus", ...)`; just return `focusVisible ? button : null`. - `comment-sheet.tsx`: the `setOpen(true)` effect now re-runs on `[content, body]` (was `[]`); it should stay `[]`. - `commentSheetTop(viewportHeight, fit = 0.55)` default is stale next to the new 0.85 detent; make `fit` required.

  3. MA
    Author

    Addressed review: dialog accessible name restored (sr-only Drawer.Title keeps 'Comment thread'/'Orphaned comments'; the visible title row is aria-hidden), markers get z-index 1 and .plan-code-toggle grows vertically only, tooltip nits fixed (try/catch focus-visible helper, setOpen deps restored to [], commentSheetTop default uses the fit range). Re-ran the tap audit at 768x1024 and 1024x768: all 4 markers 44x44 and win taps at center. e2e expectations in responsive-comments updated for the content-sized detent. CI: e2e pass, container pass; 'format, lint, types, tests' fails only on the design-contract pin for comment-layer.tsx (unchanged since the PR body).

  4. lavaman131referenced d06fccef3e99last month on GitHub

  5. MaggieAppletonhead-ref-force-pushed 4852dad985aflast month on GitHub

  6. MaggieAppletonhead-ref-force-pushed f557a96e90d1last month on GitHub

  7. MaggieAppletonhead-ref-force-pushed 69c93c3e630flast month on GitHub

  8. MA
    Author

    CI babysitter rechecked the current failed `ci` run for `69c93c3e630f6f8587c6c8556856e2e5b2c35d96`. The head still matches the recorded candidate, `e2e` and `container` are green, and the latest failure still stops in `Format, lint, and enforce design tokens` before types/tests run. From the current diff and the PR's existing notes, the first meaningful blocker is still the reviewed design-contract hash for `packages/editor/src/comment-layer.tsx`; this looks like review metadata rather than a source defect in the touch-ergonomics changes themselves.

    I did not queue a commit from this runner because `bun` is not installed here, so I could not run the required local reproduction and verification commands (`bun install --frozen-lockfile`, `bun run fix`, `bun run ci`). Next action for a human: on a machine with Bun 1.4.2 available, rerun those commands on this head, confirm the exact design-contract failure text, and refresh the `dynamic-editor` reviewed hash only if the changed class ownership in `comment-layer.tsx` is approved.

    `babysit-head:69c93c3e630f6f8587c6c8556856e2e5b2c35d96`

    > Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37619036193) · gpt54 · 39 AIC · ⌖ 4.24 AIC · ⊞ 16.4K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-call-id%3A+githubnext%2Fchopin%2Fpr-ci-fix%22&type=issues)

    <!-- gh-aw-agentic-workflow: PR CI fixer, engine: codex, model: gpt-5.4, id: 37619036193, workflow_id: pr-ci-fix, run: https://github.com/githubnext/chopin/actions/runs/37619036193 --> <!-- gh-aw-workflow-call-id: githubnext/chopin/pr-ci-fix -->

  9. MaggieAppletonhead-ref-force-pushed 949862f4a48dlast month on GitHub

  10. MaggieAppletonhead-ref-force-pushed 126bd60bdd1alast month on GitHub

  11. MA
    Author

    Commit pushed: [`cd04a32`](https://github.com/githubnext/chopin/commit/cd04a329b01e8f77a7058e6e3efd0d88570ed581)

    > Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37627133303) · gpt54 · 245.6 AIC · ⌖ 9.38 AIC · ⊞ 16.4K

  12. MA
    Author

    Root cause: `bun scripts/check-design-contract.ts` fails because `packages/editor/src/comment-layer.tsx` changed shape and the reviewed dynamic-owner hash in `scripts/design-contract/exceptions/dynamic-editor.json` was stale, while the reviewed owner boundaries themselves were unchanged.

    Changed file: `scripts/design-contract/exceptions/dynamic-editor.json`.

    Verification: `bun scripts/check-design-contract.ts` passes locally. `bun run fix` and `bun run ci` were attempted but both stop in this runner on a `dprint` plugin download proxy failure before branch-specific checks complete. The `[ci-fix]` commit is queued for guarded application; GitHub CI still needs to confirm it.

    [babysit] head=126bd60bdd1a947c3ae2c458d2b105fcbc27565d

    > [!WARNING] > <details> > <summary>Firewall blocked 1 domain</summary> > > The following domain was blocked by the firewall during workflow execution: > > - `releaseassets.githubusercontent.com` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "releaseassets.githubusercontent.com" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details>

    > Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37627133303) · gpt54 · 245.6 AIC · ⌖ 9.38 AIC · ⊞ 16.4K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-call-id%3A+githubnext%2Fchopin%2Fpr-ci-fix%22&type=issues)

    <!-- gh-aw-agentic-workflow: PR CI fixer, engine: codex, model: gpt-5.4, id: 37627133303, workflow_id: pr-ci-fix, run: https://github.com/githubnext/chopin/actions/runs/37627133303 --> <!-- gh-aw-workflow-call-id: githubnext/chopin/pr-ci-fix -->

  13. MaggieAppletonhead-ref-force-pushed ac98ac5b319alast month on GitHub

  14. MaggieAppletonreferenced this in Hide tooltips over the menus they open #334last month on GitHub

  15. MaggieAppletonhead-ref-force-pushed b15c7c68dae8last month on GitHub

  16. MaggieAppletonmerged 922c8e444fcalast month on GitHub

  17. MaggieAppletonclosedlast month on GitHub

  18. MaggieAppletonhead-ref-deletedlast month on GitHub

  19. MaggieAppletonreferenced this in Quiet accepted-comment row in the document #357last month on GitHub

  20. MaggieAppletonreferenced this in Make the decision card the one place to settle, and keep reopening copy truthful #375last month on GitHub

  21. MaggieAppletonreferenced this in Make tabs authorable in place and label empty table headers #382last month on GitHub