Skip to content

Dock phone selection actions and give the comment sheet detents - #474

Merged
MaggieAppleton merged 5 commits into
mainfrom
mobile/comment-flow
Oct 11, 2026
Merged

MaggieAppleton merged 5 commits into
mainfrom
mobile/comment-flow

Conversation

@MaggieAppleton

@MaggieAppleton MaggieAppleton commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Why

On phones, selecting text raised a floating formatting bubble under the selection that covered the next lines and mixed formatting with commenting (M-29). The comment sheet sized itself to its content, so with the keyboard open it covered the passage being discussed (M-31). Send was a 32px disc, Send to Chopin an 18px checkbox, and margin markers showed about 22px of target (M-26).

What changed

Presentation only. Comment creation is unchanged: the quote locator and server-minted positions are untouched.

  • Selection action bar (phone only). A selection now raises a bottom bar with Comment and Copy (icon and label, 44px targets). It sits above the safe area and above --phone-bottom-chrome (defaults to 0, so the tab bar slice can claim room). It rides on the keyboard via the visual viewport, rises with a smooth-out transition and only fades under reduced motion. Bottom docking keeps it clear of iOS's own callout. It has no formatting buttons, because on a phone formatting belongs to the edit mode slice. Escape lets go of the selection, as before. Ask Chopin is left out because the app has no way to quote a selection into Chat yet.
  • Comment sheet detents. The sheet opens at a medium detent: half the space above the keyboard, but never less than its header, a 64px peek of the notes and the composer. The reveal hook scrolls the passage above that detent rather than above the full sheet. Dragging the grabber or header up goes to the large detent, and dragging or flicking down dismisses it, using Base UI's snap points with velocity and rubber-banding. The finger drives the sheet with no transition lag. The backdrop dims more as the sheet moves from medium to large. The reply composer stays pinned to the visible bottom edge at either detent.
  • Pager. Shows "‹ 1 of N ›" across the document's open threads in reading order. It is hidden when the heading is "All N comments" (block lists keep their Back row). The arrows are disabled at the ends instead of wrapping.
    • Each new thread opens at the medium detent. A tap on the header of a short, one-detent sheet no longer counts as a pull to the large detent.
    • The backdrop reads the drawer's progress only while dragging, so it stays put after paging between threads of different heights.
    • Closing after paging returns focus to the marker of the thread shown and keeps the document there.
  • Selections low on the screen. When the bar appears, a selection it would cover scrolls clear of it, and the document scroller gets a matching scroll-padding-bottom. The document's existing 60vh trailing space lets even the last line rise. "Copied" is announced through a visually hidden status region.
  • Hover on touch. The btn-* hover backgrounds in apps/web/src/theme.css are now inside @media (hover: hover), so a tapped button no longer stays grey on touch. Desktop is unchanged.
  • Targets. Send is a full 44px disc. Send to Chopin is a 44px pill that fills when on. Resolve and More actions were already 44px and now have 18px icons. Margin markers on touch reach 44px tall in the empty margin beside the first line without covering any more text.
  • The phone condition (coarse pointer, ≤430px) now lives in one place, packages/editor/src/phone.ts, ready to be replaced by the shared phone class.
  • New CopyIcon.

Screenshots

iPhone 15 Pro, BEFORE (main, left) and AFTER (right).

Selection
Selection before and after

New comment
New comment before and after

Thread sheet
Thread sheet before and after

Reply with the keyboard open (viewport cut to 360px). This passage is the document title, and the tall top chrome leaves no room above the sheet. With a deeper passage it stays in view. The composer is pinned at the medium detent.
Reply with keyboard before and after

AFTER: medium detent (passage above), large detent (backdrop deeper), keyboard on a longer thread
Detents

AFTER: a selection near the bottom scrolls clear of the bar
Low selection

Desktop is unchanged (bubble still floats over the selection)
Desktop selection

Testing

  • bun run types: passes
  • bun test packages/editor packages/icons: 683 pass. New unit tests cover the medium detent, including its floor.
  • bun run ci: passes (after the hash renewal below)
  • E2E, not run locally because the shared ports are in use, so this relies on CI:
    • comment-sheet.e2e.ts adds these tests:
      • A phone selection shows the bottom bar and no format bubble. Its targets are at least 44px, it sits below the selection, and Comment opens the sheet with the passage above the sheet top.
      • A desktop selection still shows the bubble.
      • A long thread opens at the medium detent, drags up to large (the backdrop gets darker) and flicks down to dismiss, with focus returning to the marker.
      • The pager steps between threads and stops at its ends, and closing returns focus to the thread shown.
      • Paging short to long keeps the medium detent with the passage above the sheet. Paging back after a pull to large restores the resting backdrop.
      • A selection near the bottom scrolls clear of the bar.
      • The bar stands on the emulated keyboard.
      • The margin reach of a marker covers ±20px beside the chip, but not the end of the second line.
    • toolbar.e2e.ts: the phone touch test now checks the action bar. A 768px touch test keeps the bubble coverage, and the bubble-follows-scroll test runs at 600px touch.
    • responsive-comments.e2e.ts: the old check that the Comment icon is centred becomes an icon-plus-label check.

Design-contract review

Reviewed and renewed in Renew design-contract review hashes for comment-layer.tsx: packages/editor/src/comment-layer.tsx (both entries in dynamic-editor.json). The only new style is the margin reach span, which uses measured numeric geometry like the existing chip span; the reviewed preview/surface style and presence-class expressions are unchanged.

Rebase notes

Main's comment-marker-line work changed the compact sheet key to depend on the marker. This PR's constant "pinned" key is kept (it is what keeps the detent steady while paging across markers), along with main's Line import.

Needs a real device

  • How the iOS keyboard interacts with the medium detent and the bar's keyboard inset (here it is emulated with a viewport resize).
  • Whether the bar's touch preventDefault keeps the native selection on iOS when Comment or Copy is tapped.
  • How the drag feels.

🤖 Generated with Claude Code

@MaggieAppleton

Copy link
Copy Markdown
Collaborator Author

Review: Changes needed

I checked this on a seeded build (iPhone 15 Pro emulation, plus 1280×800, 768×1024 and 420px with a mouse). Comment creation is unchanged: SelectionBar calls the same comment() path, and nothing touches the locator or anchors. Desktop and narrow fine-pointer windows are unchanged: the bubble shows, and there is no bar, reach span or sheet. The bar, the 44px targets, the reach span (aria-hidden, margin only, x 357–381, no text covered), the direct medium/large/flick flow and focus return all work. Three problems break the core promise that the passage stays visible and the sheet behaves consistently.

Must fix

  1. Paging from a short thread to a long one leaves the sheet at the large height while its state says medium (comment-sheet.tsx:343-347). Steps: tap a 1-note marker, then tap Next to reach a long thread. The sheet top lands at y=88 and --drawer-snap-point-offset stays at 0px, with detent === "medium" and the backdrop at 0.36. The passage (y 139–216) is hidden under the sheet. useCommentSheetReveal places it against the 330px medium it was told about, so it scrolls the passage to a spot the sheet already covers. Dragging down once makes it snap to the correct 240px offset. Base UI doesn't recompute the offset when the popup grows while snapPoint stays the same. Fix: on a thread change, reset detent to medium and make the drawer re-resolve the snap point after the new content is measured. For example, briefly set snapPoint to null and then to medium, or set it again once floor and tall settle. Add an e2e test that pages short to long and checks that the sheet top is about medium and the passage is above it.
  2. The backdrop disappears after paging from a two-detent thread to a short one (styles.css:1382). Steps: open the long thread (medium), then tap Previous to a 1-note thread. The backdrop opacity becomes 0. data-plan-comment-sheet-detents goes away, so the base rule 0.24 * (1 - var(--drawer-swipe-progress)) takes over, and the progress is still 1 from the medium detent. The base rule needs to read progress differently when snap points exist, or the progress needs resetting. Test the backdrop opacity after paging.
  3. The docked bar covers selections in the bottom ~60px of the screen (selection-bar.tsx, styles.css:4891). With a selection at y 628–648 on a 659px viewport, the bar sits at 593–647, on top of the selection and its handles. The PR's "never over the selection" claim only holds because each test selects text near the top: expect(barBox.y).toBeGreaterThan(selection) is true by construction there. When the bar shows and the range's bottom is below the bar's top, scroll the range clear. Also give the document scroller a bottom scroll-padding or end space equal to the bar, so the last lines can be lifted above it. Add a test that selects near the bottom.

Nice to have

  • Sticky hover on the pager steps. btn-ghost has an ungated &:hover (apps/web/src/theme.css:188). After a tap, Previous/Next keep a grey square that sits partly under "2 of 4". Override it in the sheet with :active feedback only, or gate the hover.
  • Closing after paging sends the reader back to where they started. Focus returns to the marker that opened the sheet, and the reveal restores the original scroll. After paging 3 → 1 and closing, the document jumped back to thread 3. When paging, update the return target and focus to the current thread's marker.
  • The pager wraps silently (4 of 4 → 1 of 4). iOS pagers disable at the ends, so disable Previous on the first thread and Next on the last.
  • Copy feedback: aria-live on the Copy button (selection-bar.tsx:911) is unusual. Put "Copied" in a visually hidden status element.
  • The new-comment and reply textareas are still 13.4px, so iOS will zoom on focus. Slice 1 owns M-30, but this is the flow's main input, so make sure slice 1 lands first or together with this one.
  • With the keyboard emulated (viewport 420), only the passage's first line stays above the draft sheet. This was already the reveal's behaviour, but it is worth noting.
  • Missing tests: the bar's keyboard inset (visualViewport), paging between threads of different heights, and the backdrop after paging.

Design feel

The bar matches the prototype's docked pill (Ask Chopin is left out for a good reason). The medium detent on a directly opened long thread feels right: half height, composer pinned, rubber-banding, and drag-to-large deepens the backdrop. The pager header is close to the 06 target. The two snap-point bugs above are what make it feel inconsistent: the sheet's height and dimming depend on which thread you paged from.

Design-contract hash failure on comment-layer.tsx only, as listed. Acceptable.

@MaggieAppleton

Copy link
Copy Markdown
Collaborator Author

Review: Changes needed (one small fix left)

I re-checked head 67933477 on a fresh build, using the same seeded document with four threads, one of them long. Phone was iPhone 15 Pro emulation; desktop was 1280×800 with a mouse.

Verified fixed

  • Short to long paging: the sheet now rests at medium. Its top is at 328 with a 240px snap offset and backdrop 0.24, and the passage stays visible above it. A long thread opened directly still goes medium → large (0.36) → back to medium.
  • The backdrop stays at 0.24 when paging long to short. It no longer drops to 0.
  • A low selection scrolls clear of the bar. A selection at 628–648 moved to 561–581, with the bar at 593.
  • The pager stops at its ends: Previous is disabled on the first thread and Next on the last. Tapping an arrow no longer leaves a grey hover square.
  • Closing after paging keeps the reader on the current thread. Focus goes to the current thread's marker and there is no scroll jump back.
  • Copy has a visually hidden status, and aria-live is no longer on the button.
  • The btn-* hover gating doesn't change desktop. At 1280 with a mouse (hover: hover) matches and ghost buttons still get their hover background; the one control that showed none is disabled, and main behaves the same. Touch laptops keep hover because the gate is (hover: hover), not pointer: fine.
  • CI: e2e passes on this head. The only failure is the design-contract hash on comment-layer.tsx (lines 276/282/332/341/1141), as listed. The earlier e2e failure on fb0e692c was the new low-selection test, which 67933477 fixes. I didn't see a responsive-workspace failure on this PR. The phone-workspace.e2e.ts "top bar hides" failure appears on mobile/bottom-tabs (run 38120167773), not here.

Must fix

  • Paging shows a stray preview card of the thread you just left (comment-layer.tsx, step in the pager block around line 1062). Tap a marker, then tap Next. The .plan-comment-preview card of the first thread then shows over the document above the sheet, covering the area the medium detent is meant to keep visible. It happens because preview (set by the tap) stays as it was, and pinned !== preview becomes true once paging moves pinned on. Clear the preview in step (and whenever the sheet's pinned thread changes), and add an assertion to the pager e2e test that .plan-comment-preview has a count of 0 after Next.

Nice to have (also on main)

  • Closing the sheet leaves a preview card on the marker that gets focus back. Main does this too on the same flow (checked on the 8811 reference). On a phone, focus coming back to a marker shouldn't open a hover preview.

MaggieAppleton and others added 5 commits October 11, 2026 09:59
On a phone, a text selection now raises a bottom action bar (Comment,
Copy) docked above the safe area and any bottom chrome, instead of a
floating format bubble that covered the next lines and mixed formatting
with commenting. Desktop and wider touch screens keep the bubble.

The phone comment sheet opens at a medium detent of the space above the
keyboard, so its passage stays in view, and drags up to a large detent
or down to dismiss. The reply composer stays on the visible edge at
either detent, the backdrop deepens as the sheet rises, a pager steps
through the document's threads, and send, Send to Chopin and the margin
markers are full 44px targets. Comment creation is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ctions

A tap on a short sheet's header made the drawer report its only snap point
as the large detent, so the next, longer thread opened full height with the
passage hidden under it. Only a real pull now reaches the large detent, and
each new thread opens at medium. The backdrop reads the drawer's progress only
while dragging, so it no longer vanishes after paging from a two-detent
thread to a one-detent one.

The pager stops at its ends, and closing after paging returns focus and the
document to the thread shown. A selection low on the screen scrolls clear of
the docked bar, Copy announces through a status region, and button hover is
gated to devices that can hover, so a tapped control does not stay grey.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…election test

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- packages/editor/src/comment-layer.tsx: adds a thread pager, a detent
  callback, a touch-only focus guard, and a margin touch-target span whose
  style is measured numeric geometry like the reviewed chip and hit styles;
  the reviewed preview/surface style and presence-class expressions are
  unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaggieAppleton
MaggieAppleton merged commit 1d64740 into main Oct 11, 2026
3 checks passed
@MaggieAppleton
MaggieAppleton deleted the mobile/comment-flow branch October 11, 2026 09:15
MaggieAppleton added a commit that referenced this pull request Oct 11, 2026
The selection bar from #474 now takes its gate from selectionOffer(), so a
reader's lock no longer hides it; the reading spec asserts the bar itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaggieAppleton added a commit that referenced this pull request Oct 11, 2026
The selection bar from #474 now takes its gate from selectionOffer(), so a
reader's lock no longer hides it; the reading spec asserts the bar itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaggieAppleton added a commit that referenced this pull request Oct 11, 2026
The selection bar from #474 now takes its gate from selectionOffer(), so a
reader's lock no longer hides it; the reading spec asserts the bar itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant