Skip to content

feat(studio): marquee multi-selection + off-canvas indicators#1693

Merged
miguel-heygen merged 6 commits into
mainfrom
feat/marquee-multi-selection
Jun 24, 2026
Merged

feat(studio): marquee multi-selection + off-canvas indicators#1693
miguel-heygen merged 6 commits into
mainfrom
feat/marquee-multi-selection

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Marquee (rubber-band) multi-selection and off-canvas element indicators.

  • Marquee selection: click+drag on empty canvas draws a dashed selection rectangle. On release, all elements whose OBB intersects are group-selected via SAT (Separating Axis Theorem) — handles rotated, scaled, and skewed elements accurately.
  • Shift+marquee: adds to existing group selection.
  • Click-to-deselect: clicking empty canvas without dragging clears selection.
  • Off-canvas indicators: elements with center outside the composition bounds show dashed teal outlines in the gray zone. Clickable to select. Dashed border only shows outside the canvas; solid inside (clip-path polygon with evenodd hole).
  • Gray zone interaction: removed the outsideComp guard so elements outside the canvas can be hovered/clicked.
  • Drag off-canvas: selection no longer clears when releasing a drag in the gray zone (suppressNextBoxClickRef).
  • Pointer capture: marquee drag continues tracking even when pointer exits overlay bounds.
  • 12 SAT geometry unit tests.

Test plan

  • Click+drag on empty canvas draws dashed teal rectangle
  • Releasing selects all elements whose visual OBB intersects
  • Shift+marquee adds to existing selection
  • Clicking without dragging deselects
  • Rotated elements: only selected when visual OBB intersects
  • Full-bleed wrappers excluded
  • Off-canvas elements show dashed indicators (clickable)
  • Dragging element off-canvas keeps selection
  • bun test packages/studio/src/utils/marqueeGeometry.test.ts — 12 pass

miguel-heygen commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marquee multi-selection + off-canvas indicators — review

Head: e26eaf81abcadf0329b48c3789714cf11752cb25
Base: fix/runtime-set-tween-resilience (stacked)
Scope: +707/-52 across 10 files. New marqueeGeometry.ts (SAT helper, 12 unit tests), marqueeCommit.ts (drag/commit hook), and off-canvas indicator rendering in DomEditOverlay.tsx.

The SAT machinery and the marquee pointer flow are well-formed. Nice work choosing real OBB intersection over AABB-only — the rotated-square test cases prove it. Two findings worth talking about; nothing blocking.

Finding 1 — PR body / behavior contract drift: "center outside" vs "partially outside" (NIT, doc)

PR description says:

elements with center outside the composition bounds show dashed teal outlines in the gray zone

Implementation in packages/studio/src/components/editor/DomEditOverlay.tsx (off-canvas effect, the partiallyOutside predicate):

const partiallyOutside =
  r.left < compRect.left ||
  r.left + r.width > compRect.left + compRect.width ||
  r.top < compRect.top ||
  r.top + r.height > compRect.top + compRect.height;
if (partiallyOutside) { rects.push(...); }

That fires whenever any edge crosses the composition border — so a 1000×500 element that's mostly inside the comp but pokes 4px past the right edge gets the off-canvas treatment too. The visual is tame here because clipOutside (the polygon(evenodd, ...) cutout) trims away the in-canvas portion of the dashed border, so what the user sees is only the protruding sliver — but the predicate name and the PR contract don't match. Pick one:

  • If "any-edge-outside" is the actual intent, update the PR body and rename the local to something like extendsOutsideComp.
  • If "center outside" was the intent (centroid-based predicate, matches the natural mental model of "this element lives in the gray zone"), the predicate needs to compute cx = r.left + r.width/2 etc. and test those vs. compRect.

Worth pinning down before merge — it shows up in the test plan too ("Off-canvas elements show dashed indicators" is ambiguous under the current predicate).

Finding 2 — Off-canvas indicator suppression ignores group selection (NIT)

Same file, the render loop for off-canvas indicators:

{offCanvasRects
  .filter((r) => {
    const selEl = selection?.element;
    return !selEl || offCanvasElementsRef.current.get(r.key) !== selEl;
  })
  .map((r) => { ... })}

This suppresses the indicator for the single primary selection.element only — not for the rest of groupSelections. After a marquee multi-select that captures three partially-outside elements, the primary gets the normal selection box and the other two render both the group rect (groupOverlayItems loop above) AND the off-canvas indicator. Two visual layers on the same item.

Also the effect that builds offCanvasRects has dep array [iframeRef, compRect, activeCompositionPath, selection] — it doesn't observe groupSelections, so even if you extend the filter to cover the group, the indicator set won't recompute on group changes until something else triggers it. Fix is both:

  1. Extend the filter to walk groupSelections (or build a Set<HTMLElement> once from the active selection set).
  2. Add groupSelections (or its derived selection-element set) to the dep array.

Things checked and clean

  • SAT correctness: 2 AABB axes + 2 OBB edge normals is the right optimization for two rectangles (parallel sides → one normal per pair). The 45°-rotated-square AABB-overlaps-but-OBB-doesn't test is the right canary. marqueeIntersectsObb handles degenerate edges (len < 1e-9) and zero-width marquees correctly.
  • Modifier semantics: Plain marquee → replace; Shift+marquee → add (event.shiftKey threaded through commitMarqueeapplyMarqueeSelection). Click without drag (!pastThreshold) → clear via onMarqueeSelectRef.current?.([], false). Matches PR body. No contradictory rules.
  • Pointer capture: setPointerCapture on marquee start, releasePointerCapture in onPointerUp + try/catch for already-released. Marquee continues past overlay bounds. Good.
  • coversComposition guard in runMarqueeIntersection: full-bleed wrappers correctly excluded from marquee hits.
  • clipPath: polygon(evenodd, ...): valid CSS syntax (<fill-rule> is a legal first arg to polygon() per CSS spec).
  • No console. introduced*: clean wrt #1691.
  • identity-transform fast path in elementObbCorners: skips DOMMatrix walk when transform is identity-ish — sensible perf.

CI

Required checks green at read-time; "Preview parity" + "Graphite / mergeability_check" still in progress (mergeability blocked on downstack which is expected for stacked PRs).

Verdict

NIT — Finding 1 is a contract-clarification ask, Finding 2 is a small visual-double-up that only surfaces with multi-selection of partially-outside elements. Neither blocks merge. The geometry layer is solid and the integration plumbing through applyMarqueeSelection / DomEditActionsContext looks correct.

Review by Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 @ e26eaf81 — Rames D Jusso review

Layering on Via's review (which I've read at HEAD). The SAT geometry, modifier semantics, and pointer-capture flow all check out — agree with everything Via flagged on F1 (partiallyOutside vs PR-body "center outside" contract drift) and F2 (group-selection ignored in off-canvas indicator filter + missing groupSelections dep). These are real and I won't restate them. Below are findings I caught from a different angle.


🛑 — none

⚠️ Concerns

C1 — useDomEditPreviewSync.ts:70-72 silently drops the no-element-found clear

const nextElement = findElementForSelection(doc, currentSelection, activeCompPath);
if (!nextElement) {
-  applyDomSelection(null, { revealPanel: false });
  return;
}

The PR scope is canvas/gray-zone selection plumbing; this hunk is broader — it changes behavior on every preview re-sync where the previously-selected element no longer resolves (composition reload, hot reload, swapping activeCompPath, document replacement after a save). Previously selection was cleared in that case; now domEditSelectionRef.current keeps pointing at a stale HTMLElement from a previous document. The references in boxRef / overlayRect rendering will still attempt geometry on a node that's no longer in the live DOM.

If this was intentional to fix the "drag-off-canvas releases clear selection" symptom mentioned in the PR body, the cleaner fix is in the canvas/box click suppression (you already added suppressNextBoxClickRef for that). Suggest reverting this hunk and adding a comment if there's a specific repro the canvas-path doesn't cover.

C2 — applyMarqueeSelection bypasses STUDIO_INSPECTOR_PANELS_ENABLED and applyDomSelection's side effects

useDomSelection.ts:423-456 implements applyMarqueeSelection as a parallel selection-write path that directly calls setDomEditSelection / setDomEditGroupSelections / setSelectedTimelineElementId — it does not go through applyDomSelection. Two consequences:

  1. The if (!STUDIO_INSPECTOR_PANELS_ENABLED) short-circuit in applyDomSelection (line 145) is skipped. If that flag is ever flipped off in prod for a rollback, marquee will still land selections while the inspector panel UI is suppressed — divergent state.
  2. The revealPanel / setRightCollapsed(false) / setRightPanelTab("design") behavior is skipped. Marquee multi-select silently leaves the right panel collapsed even when the user just intentionally selected things. Single-click selection reveals the panel; multi-marquee doesn't. Worth verifying intent — could be deliberate ("don't shift the UI on bulk select") or an oversight.

Suggest at minimum honoring the flag check; the panel-reveal is a product call.

C3 — Off-canvas indicator click-target uses clipPath for hit-suppression, which doesn't suppress hit-testing

DomEditOverlay.tsx:589-594. The dashed-indicator div is pointer-events-auto over the FULL bounding rect, with clipPath: polygon(evenodd, ...) visually removing the in-canvas portion. In Chromium, clip-path is purely visual — pointer hit-testing still uses the element's bounding box. So clicking inside the in-canvas portion of an off-canvas element's bounding rect hits this indicator's onClick and the underlying overlay handler at the same coordinates. The e.stopPropagation() inside handleClick prevents the conflict from bubbling, but means a user clicking the in-canvas portion of a partially-off-canvas element always selects via the off-canvas indicator path (which uses skipSourceProbe: true) rather than the normal canvas selection path. Different code path → potentially different DomEditSelection shape for the same element.

Either restrict the overlay's positioned rect to the actually-outside region (compute the outside fragments and create one positioned div per fragment), or accept the duplication and add a comment.

🟡 Nits

N1 — style={marquee.marqueeRef.current?.pastThreshold ? { cursor: "crosshair" } : undefined} reads a ref during render

DomEditOverlay.tsx:410. marqueeRef is a useRef — mutating pastThreshold does not trigger a re-render. The cursor only updates because setMarqueeRect also fires on the same onPointerMove tick, which causes a re-render that happens to re-read the ref. Brittle by construction; if a future refactor delays the setMarqueeRect update or batches it differently, the cursor change desyncs. Promote pastThreshold to a piece of state on the hook return, or read it from a stable indicator like marqueeRect != null.

N2 — Off-canvas indicator is not keyboard-accessible

DomEditOverlay.tsx:589-594. The dashed click target is a <div> with onClick only — no role="button", no tabIndex, no key handler, no aria-label (just title, which screen readers may or may not announce). Studio is a creative tool, so the keyboard-a11y bar is lower than general web, but pure-mouse-only off-canvas reachability means power users can't tab to them. Trivial fix: role="button" tabIndex={0} onKeyDown={(e) => (e.key === "Enter" || e.key === " ") && handleClick(e as any)}.

N3 — Off-canvas effect re-runs on every selection change

DomEditOverlay.tsx:263 dep array [iframeRef, compRect, activeCompositionPath, selection]. Every single-element selection change re-walks collectDomEditLayerItems, computes isElementComputedVisible + toOverlayRect for every item, and re-renders the indicators. With ~50 elements in a composition this is ~50 getBoundingClientRect + getComputedStyle reads per selection change. The dep on selection is only needed to drive the filter (don't show indicator for the selected element); the position computation is independent of selection. Split: one memo computes raw offCanvasRects, a derived list applies the selection filter. Cuts the cost to per-element-list-change.

🟢 What's clean

  • SAT machinery + identity-transform fast path in elementObbCorners is right (agree with Via).
  • setPointerCapture / releasePointerCapture with try/catch is correct.
  • coversComposition guard against full-bleed wrappers.
  • runMarqueeIntersection uses skipSourceProbe: true consistently with the off-canvas click handler — same selection shape.
  • Hover-rect rendering simplification (dropping the per-element borderRadius computed-style lookup) is fine; the loss in fidelity is minor and the perf win on hover is real.
  • 12 marquee geometry tests including the 45°/30° rotated cases and degenerate-OBB edge case — strong coverage.

CI snapshot

Preview-parity ✓, Player perf ✓, regression ✓. Graphite mergeability still in-progress (expected, stacked PR).


— Rames D Jusso

Stamp routing: @rames Jusso once findings addressed (or punted with rationale).

miguel-heygen and others added 6 commits June 24, 2026 17:07
…tice

- Delete empty if-blocks left after console removal (snapTargetCollection,
  Player asset-poll, useTimelineSyncCallbacks 5s probe, useGestureRecording
  dev guard + now-unused isDevBuild) and the stale "surface in dev" comment.
- Drop the dangling no-console pragma + dead duplicate-id branch in sourcePatcher.
- Restore the one-time telemetry consent disclosure in showNoticeOnce (kept
  behind a pragma — it is a user-facing notice, not debug noise).
- Remove the missed timelineIcons console.warn while preserving the
  `tag || "div"` null-safety fallback.
- Route caption auto-save failures (a data-loss path) through telemetry
  instead of swallowing silently.
- Restore the accidentally-clobbered css-var-fonts output.mp4 fixture.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…zation

- Set tweens now emit immediateRender:true so they render on page load
  without requiring the runtime to seek past position 0
- Runtime IIFE normalizes array timelines (window.__timelines = [tl])
  to keyed objects, and auto-adds data-start on root elements
- Drag teardown clears translate:none to prevent #1673 fly-off
- Position-only set tweens hidden from timeline diamonds (3 cache paths)
- Parser: ease-only keyframe update preserves existing properties
…b restore

- Restore the #1651 skipForInjectedVideo gate in media.ts that was dropped on
  restack — avoids ~2400 wasted per-tick seeks on video-heavy renders.
- Restore the console.debug body + docstring bullet of swallow() in
  diagnostics.ts: the __hfDebug opt-in debug surface had been gutted to an
  empty if-block.
- Rebind: after the progress-cycle set() kick, seek to state.currentTime via
  totalTime() instead of snapping to 0, so a rebind after scrub / soft-reload
  restore keeps the playhead.
- Array __timelines normalization + data-start default now resolve the root
  via a shared findRootCompositionEl() that honors data-root="true" first
  (matches resolveRootCompositionElement, which now delegates to it).
- Ease-only keyframe update leaves a primitive (non-object) keyframe value
  untouched instead of wiping it to {}; add a preservation unit test.
- Document the boundDuration<=0 progress(1) kick + restore the STATIC-case
  comment in gsapRuntimeBridge.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Click+drag on empty canvas draws dashed selection rectangle
- SAT/OBB intersection handles rotated/scaled/skewed elements
- Shift+marquee adds to existing selection
- Click on empty canvas deselects
- Off-canvas elements show dashed outline indicators (clickable)
- Dashed border only shows outside canvas, solid inside (clip-path)
- 12 geometry unit tests
- Off-canvas indicator suppression now skips every selected element (primary
  AND marquee group members), not just the primary, so group members no longer
  render a doubled overlay (group rect + dashed indicator).
- Drop selection from the off-canvas layout effect deps; the selected-element
  filter runs at render time. Avoids re-walking geometry on each selection change.
- applyMarqueeSelection now honors STUDIO_INSPECTOR_PANELS_ENABLED.
- Restore the stale-selection clear in useDomEditPreviewSync when the selected
  element no longer resolves after a re-sync. Drag-release stays handled by
  suppressNextBoxClickRef.
- Off-canvas indicator is keyboard-accessible; canvas cursor driven by marquee
  rect state, not a render-time ref read.
- Rename partiallyOutside -> extendsOutsideComp + comment the clip-path hit-test.
- Extract OffCanvasIndicators into its own component (DomEditOverlay was already
  over the 600-LOC cap on this branch; extraction brings it under).
- Declare onUpdateKeyframeEase on PropertyPanelProps so this branch typechecks
  standalone (handler + wiring already here; only the type had leaked upstack).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@miguel-heygen miguel-heygen force-pushed the fix/runtime-set-tween-resilience branch from 7498be8 to d680397 Compare June 24, 2026 21:08
@miguel-heygen miguel-heygen force-pushed the feat/marquee-multi-selection branch from e26eaf8 to 441f841 Compare June 24, 2026 21:08
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks @vanceingalls / @james-russo-rames-d-jusso — addressed in the latest push.

Fixed

  • F2 — off-canvas indicator ignores group selection — the render-time filter now suppresses the indicator for every selected element (primary and marquee group members), so group members no longer render a doubled overlay.
  • N3 — effect re-walks geometry on every selection change — dropped selection from the off-canvas layout-effect deps (positions depend on layout, not selection; the selected-element suppression is a render-time filter). This also satisfies F2's dep concern without re-walking.
  • F1 — partiallyOutside vs PR-body "center outside" — the intent is any-edge-outside (the in-canvas portion is clipped away). Renamed to extendsOutsideComp + comment; PR body now matches.
  • C1 — useDomEditPreviewSync dropped the no-element clear — restored. It was an unmentioned hunk; the drag-release-in-gray-zone case is handled by suppressNextBoxClickRef, and the dragged element still resolves, so the clear is safe and prevents geometry on a detached node.
  • C2 — applyMarqueeSelection bypassed the inspector flag — now honors STUDIO_INSPECTOR_PANELS_ENABLED, mirroring applyDomSelection.
  • N1 — pastThreshold ref read during render — cursor now driven by marquee.marqueeRect state.
  • N2 — keyboard a11y — the indicator is now role="button" + tabIndex + Enter/Space handler + aria-label.

Accepted, with comment

  • C3 — clip-path hit-testing covers the full bounding rect — documented in code. It resolves the same element the canvas path would (just with skipSourceProbe, since the element is already known); fragment-splitting wasn't worth the complexity here.

Also

Base automatically changed from fix/runtime-set-tween-resilience to main June 24, 2026 21:53
@miguel-heygen miguel-heygen merged commit 8ae010b into main Jun 24, 2026
20 checks passed
@miguel-heygen miguel-heygen deleted the feat/marquee-multi-selection branch June 24, 2026 21:53
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.

3 participants