slides: mount editor on mobile + Pointer Events migration#261
Conversation
Spike on the iPhone 16 Pro simulator (Task 0) confirmed the SlidesEditor mounts cleanly on the mobile shell but that iOS Safari does not synthesize mousemove during a touch drag — selection works on tap, but drag/resize/rotate never receive move events. Long-press callout also isn't a contextmenu event, so the spike's onContextMenu suppression was a no-op. Resolution chosen at the gate: split Task 1 into 1a (migrate the slides editor from Mouse Events to Pointer Events — strict superset, no state-machine change, desktop unaffected) and 1b (the mobile-mount work plus -webkit-touch-callout: none CSS). Pen tablet / stylus input becomes supported as a side-effect. Lands the design doc, multi-PR todo, and lessons file with the matrix that drove the decision. No code changes yet.
Replaces every mousedown/mousemove/mouseup/mouseenter/mouseleave listener in the slides editor module with the pointer-event equivalent. PointerEvent is a strict superset of MouseEvent (it inherits from it in TS), and browsers synthesize pointer events from both mouse and touch — so this rename costs nothing on desktop but is the prerequisite for touch drag on iOS Safari, which never synthesizes mousemove during a touch drag (only mousedown/mouseup for taps). Pen tablets and stylus input get supported as a side-effect. Scope is the slides editor module only (editor.ts, thumbnail- panel.ts, context-menu.ts, layout-picker.ts). The text-box-editor delegates click handling to the docs TextEditor, which still uses mouse events; tests that target it keep MouseEvent dispatches. The presenter module (read-only playback) is also out of scope. Phase B spike findings that drove this: docs/tasks/active/20260517-slides-mobile-edit-lessons.md
The mobile shell needs fingertips to land on 8px handles reliably. Add a `tolerance` parameter to `handleHitTest` (default 0 — desktop unchanged) that expands each handle's hit rectangle by that many pixels on every side. `SlidesEditor` accepts the matching `touchHandleTolerance` option and forwards it to every hit-test call. 22px expansion around 9 handles on a small selection makes their hit rectangles overlap, so a flat first-match-wins iteration picked by DOM order and felt non-deterministic from a fingertip. Switch to closest-center: among all handles whose expanded rect contains the point, pick the one whose center is nearest. Two new unit tests cover the overlap path and confirm desktop tolerance=0 behavior when only one rect matches stays equivalent.
Adds a `mode: 'view' | 'edit'` prop to MobileSlidesView, defaulting to 'edit'. The 'view' branch keeps Phase A's read-only SlideRenderer path with swipe nav. The 'edit' branch builds a canvas + overlay and mounts the desktop SlidesEditor against them with touchHandleTolerance=22 forwarded — touch drag/resize/rotate work out of the box via the Pointer Events migration in the previous commit (one-back). Two iOS-specific guards on the canvas-host: touch-action:none gives the editor sole ownership of horizontal pointer (no competition from browser swipe nav, double-tap zoom, pinch); -webkit-touch-callout + user-select kill the long-press callout preview that contextmenu blocking does NOT cover. slides-detail passes mode="edit" explicitly. Permission-gated 'view' fallback for shared-link viewers is the Task 4 wiring. Phase B Task 1b — see docs/tasks/active/20260517-slides-mobile-edit-todo.md.
Three findings from the sim smoke pass: (1) flat 22px hit tolerance around 9 close-packed handles needed closest-center disambiguation to feel deterministic — fix in the previous slides commit. (2) iOS Hangul IME composition in the docs text-box mounted from slides behaves differently from desktop; deferred to Task 2 (text-edit PR) since that's where docs-side IME work belongs. (3) The _spike-login.html cookie installer is the repeatable trick for dropping the iPhone sim into an authenticated session — local-only, never committed, recreate per spike.
Self-review caught two issues in the new edit-mode mount: 1. **Listener leak (Critical):** the cleanup dropped editorRef but never called editor.detach(), so every mount cycle leaked the editor's document-level pointer/key listeners, any in-flight RAF, and an active text-box. Desktop slides-view.tsx:479 already tears down the same way — mobile now matches. 2. **Stale-closure setState (Important):** the onCurrentSlideChange callback compared the editor's reported slide id against currentSlideId captured in closure at mount time, which never refreshed because the effect intentionally omits currentSlideId from its dep list (so footer-arrow taps don't tear down the editor). Switch to the prev-state reducer form so we always compare against the latest React state.
Code review surfaced two non-blocking items kept out of this PR
but worth recording for the next person:
- The `[data-handle] { pointer-events: auto !important }` style is
injected as a global selector by BOTH desktop (slides-view.tsx)
and mobile (mobile-slides-view.tsx). The mobile mount inherits
the desktop pattern verbatim; isMobile branches so they never
fight. A follow-up should scope both. Logged in lessons.
- jsdom defaults PointerEvent's pointerId to 0; real browsers use
1+. Not a present bug because the editor doesn't read pointerId,
but a trap for any future PR that adds setPointerCapture. Logged.
Also drops a confusing tie-break paragraph in the closest-center
overlap test — the test uses biased points that make the tie
analysis irrelevant.
📝 WalkthroughWalkthroughThis PR implements Phase B of mobile-editing support by establishing Pointer Events touch handling, improving touch-target hit-testing, and mounting the desktop SlidesEditor on mobile in an optional edit mode, with comprehensive design documentation and task sequencing for multi-PR rollout. ChangesMobile Light Edit (Phase B)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Verification: verify:selfResult: ✅ PASS in 215.6s
Verification: verify:integrationResult: ✅ PASS |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/slides/slides-mobile-edit.md`:
- Around line 79-88: The fenced code blocks that contain the ASCII diagrams (the
block showing "before (Phase A) ... after (Phase B)" and the later similar
block) are missing language identifiers; update each opening triple-backtick to
include a language tag (e.g., ```text) so markdownlint MD040 is satisfied and
the docs remain lint-clean.
In `@docs/tasks/active/20260517-slides-mobile-edit-todo.md`:
- Around line 145-147: The grep in the checklist only scans packages/slides/src
and can miss test files; update the command used in the doc (the grep invocation
that searches for new MouseEvent or dispatchEvent.*mouse) to search repo-wide or
include both packages/slides/src and packages/slides/test (for example by
removing the directory argument to search the repo or adding --include for both
paths) so test-event checks catch real test files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2126e311-7680-4628-a388-e94f94fa0467
📒 Files selected for processing (18)
docs/design/README.mddocs/design/slides/slides-mobile-edit.mddocs/tasks/README.mddocs/tasks/active/20260517-slides-mobile-edit-lessons.mddocs/tasks/active/20260517-slides-mobile-edit-todo.mdpackages/frontend/src/app/slides/mobile-slides-view.tsxpackages/frontend/src/app/slides/slides-detail.tsxpackages/slides/src/view/editor/context-menu.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/hit-test.tspackages/slides/src/view/editor/layout-picker.tspackages/slides/src/view/editor/thumbnail-panel.tspackages/slides/test/view/editor/context-menu.test.tspackages/slides/test/view/editor/editor.test.tspackages/slides/test/view/editor/hit-test.test.tspackages/slides/test/view/editor/layout-picker.test.tspackages/slides/test/view/editor/text-box-editor.test.tspackages/slides/test/view/editor/thumbnail-panel.test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Two minor markdownlint / accuracy nits: - Tag the two ascii-diagram fences in slides-mobile-edit.md with ```text so MD040 stops warning. - Widen the Task 1a checklist's grep to scan packages/slides (not just packages/slides/src) so the test-event audit catches files under packages/slides/test — which is the directory the migration actually had to touch.
These seven slides + PPTX tasks shipped (PRs #238, #240, #241, #255, #259, #260, #261) but their checklist boxes were never ticked during execution, so the archive script left them in active/. Sweep the checkboxes to reflect what actually landed, add lessons files for the two that were missing (keyboard-shortcuts, presentation-mode), and convert the keyboard-shortcuts onLinkRequest popover bullet from a checkbox to a "Deferred (follow-up)" note so the gate stops blocking on work that is intentionally out of scope. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Summary
Phase B Task 1a + 1b of Slides Mobile Light Edit. Splits cleanly into two cohesive halves:
Task 1a — Pointer Events migration (slides package only)
mouse*event listeners topointer*across the slides editor module (editor.ts,thumbnail-panel.ts,context-menu.ts,layout-picker.ts). 5 test files updated to dispatchPointerEventinstead ofMouseEvent.PointerEventis a strict superset ofMouseEvent(it inherits from it in TS); browsers synthesize pointer events from both mouse and touch input. Desktop behavior is unchanged.mousemoveduring a touch drag (onlymousedown/mouseupfor taps). Pen tablets and stylus input get supported as a side-effect.Task 1b — Mount full SlidesEditor on mobile
MobileSlidesViewgains amode: 'view' | 'edit'prop.'view'keeps Phase A's read-onlySlideRendererwith swipe nav.'edit'(default) builds canvas + overlay and mounts the desktopSlidesEditor.touchHandleTolerance?: numberoption onSlidesEditor.handleHitTestnow expands hit rectangles by that many pixels on each side AND switches to closest-center disambiguation when expanded rects overlap (fingers on small selections). Desktop default is 0; mobile passes 22 (≈ 44px Apple HIG min).touch-action: none(canvas owns horizontal pointer — no browser swipe/pinch competition) and-webkit-touch-callout: none+user-select: none(kills long-press preview thatcontextmenublocking doesn't cover).SlidesStore, so Yorkie sync and undo/redo are inherited from desktop with zero new mutation surface.Permission-gated
'view'fallback for shared-link viewers is Task 4 (separate PR). Bottom-sheet text formatting + slide-ops FAB are Tasks 2-3.Test plan
pnpm verify:fastgreen (792 slides tests + 2 new hit-test tolerance/overlap tests).PointerEvent, drag a shape 100px, verify nw handle moved and screenshot confirms element followed.Self-review fixes already applied
Pre-push review (via superpowers:requesting-code-review) caught and fixed:
editor.detach()to matchslides-view.tsx:479— was leaking document-level pointer/key listeners per mount cycle.onCurrentSlideChangecallback uses functionalsetStateto avoid stale-closure compare againstcurrentSlideId.Deferred with rationale (logged in lessons):
[data-handle]style selector is a pre-existing desktop pattern (slides-view.tsx:301-303); follow-up scopes both.PointerEvent.pointerIddefaults to 0 vs real-browser 1+ — only matters if a future PR addssetPointerCapture.Design + planning
docs/design/slides/slides-mobile-edit.mddocs/tasks/active/20260517-slides-mobile-edit-todo.md(Tasks 0/1a/1b done; 2-4 follow as separate PRs)docs/tasks/active/20260517-slides-mobile-edit-lessons.mdSummary by CodeRabbit
New Features
Improvements
Documentation