[drag-engine] Add drag and drop primitives - #5487
Conversation
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Bundle size
PerformanceTotal duration: 1,063.33 ms -118.91 ms(-10.1%) | Renders: 76 (+0) | Paint: 1,723.65 ms -202.00 ms(-10.5%)
…and 1 more (+9 within noise) — details Metric alarms
Check out the code infra dashboard for more information about this PR. |
mnajdova
left a comment
There was a problem hiding this comment.
Some useful feedback extract from initial AI review.
| // Breaking layouts get a new shared-slot protocol; additive fields must be | ||
| // backfilled instead, because bumping the protocol would split the live store | ||
| // and registries between the two copies. | ||
| slot.sourceStore ??= new Store<DragSource | null>(slot.store.state?.source ?? null); |
There was a problem hiding this comment.
AI review worth consindering:
Hand-maintained mirror store sourceStore duplicates what a useStore selector on the main store already provides. Severity: 🟠 significant waste. The file keeps a second Store<DragSource | null> plus sourceSnapshot/sourceVersion bookkeeping, synced by a module-scope subscription (slot.sourceStoreSubscription ??= slot.store.subscribe((state) => {... slot.sourceStore.setState(source); })). failure_scenario: ~30 lines of mirror plumbing plus an import-time subscription that runs on every session update forever, existing only because updateDragSourceElement mutates state.source.element in place and then must force a wake via slot.sourceStore.setState({ ...state.source }) — an Object.is selector on the main store would miss the mutation. Smaller form: make the retarget immutable (setState({ ...state, source: { ...state.source, element: newElement } })) and let consumers use the existing useStore(dragSessionStore, (s) => s?.source ?? null) selector path (packages/utils/src/store/useStore.ts already skips re-renders on unchanged selection); sourceStore, sourceSnapshot, sourceVersion syncing, and the backfill block all disappear.
There was a problem hiding this comment.
Thanks, I checked this. I think sourceStore is worth keeping. The main session store changes whenever the hovered targets change, but source consumers only need updates when a drag starts, ends, or its source node is replaced. A selector would prevent React re-renders, but it would still wake every subscriber on each target change. The displacement code also subscribes to this store directly.
The in-place source.element update is intentional too. The lifecycle and sensors keep the original DragSource object and read it later. Replacing only the source in the session store would leave that code pointing at the old, detached element after a virtualized row is remounted.
sourceVersion is separate from this mirror and is needed for target state updates in React 18. I think the right follow-up is to keep the current design and add a short comment and focused test to make these constraints clearer.
🤖 Comment generated with Codex
| * A per-target session view. Movement wakes only targets in the old/new hover | ||
| * stack instead of synchronously running selectors for every target on the page. | ||
| */ | ||
| export function createDragTargetStateStore(): DragTargetStateStore { |
There was a problem hiding this comment.
AI review worth considering:
Third hand-rolled pub/sub in the same file: per-element listener buckets (targetListeners: Map<Element, Set<() => void>>, allTargetListeners) plus a bit-packed numeric snapshot with a version multiplier (value + slot.sourceVersion * dragTargetStateStride). Severity: 🟡 moderate. failure_scenario: one file now contains three notification mechanisms (session Store, mirror Store, manual keyed emitter), and setDragSession manually fans out to the right buckets (~35 lines of listener-set union logic). The targeted-wake design is genuinely performance-motivated (avoids running N selectors per pointer frame), so this cannot literally be one Store — but the get-or-create/delete-when-empty Set-in-Map bucket code is written twice within the file (subscribe and setElement both inline let set = slot.targetListeners.get(...); if (!set) { set = new Set(); ... }). Smaller form: a tiny keyed-emitter helper (addToSetMap/removeFromSetMap, or a KeyedStore<Element, number> built on Store) so the bucket maintenance exists once; the sourceVersion * stride trick would then live behind one named API instead of being spread across getSnapshot, setDragSession, and the consumer's selector mask.
There was a problem hiding this comment.
Good point on the repeated bucket setup. I extracted that part into a local addToElement helper in 64062a0, next to the existing removal helper.
I kept the targeted fan-out and sourceVersion logic. They avoid waking unrelated drop targets during pointer movement and make the accepting state update correctly in React 18. A broader KeyedStore abstraction would add more code than this small case needs.
I verified the change with the drag session and drop target tests, Prettier, ESLint, and TypeScript.
🤖 Comment generated with Codex
| settings: ResolvedDragPreview<any> | null; | ||
| } | ||
|
|
||
| const slot = getSharedSlot<ActivePreviewSlot>('activeDragPreview', () => ({ |
There was a problem hiding this comment.
AI review worth considering:
Three separate global slots all track "the active drag's preview": the activeDragPreview slot ({ handle, settings }), the per-provider Store<DragPreviewState | null>, and the dragPreview.lastPublished slot ({ store }). Severity: 🟡 moderate. failure_scenario: overlapping caches of the same drag-start resolution — activePreview stores settings: ResolvedDragPreview explicitly "so React never resolves them a second time", while DragPreviewState re-carries offset/sourceRect/input that were part of, or derived alongside, that same resolution; and clearPublishedDragPreview exists only because the clearing code can't reach whichever provider store received the content ("Clear the preview the previous drag published, whichever store it went to"). Smaller form: a single global Store<ActivePreview | null> whose state carries a provider identity (the context object) alongside handle/settings/node/host; each PreviewOverlayRenderer subscribes to the one store and renders only when state?.provider === myContext. That collapses createDragPreviewStore, publishDragPreview's dual bookkeeping, and the lastPublished slot, and makes drag-end clearing a plain setState(null).
There was a problem hiding this comment.
Fixed in 17d294888. I consolidated the React-rendered preview into one shared store. Each PreviewProvider now renders only the preview that belongs to it, so the separate provider stores and the extra “last published” tracker are gone.
I kept activeDragPreview separate because it has a different job. The drag engine creates that handle before a drag is accepted. If a second pickup is refused, it must restore the previous handle without changing the preview already on screen. It also updates and removes the actual preview element, while the shared store only chooses which React content to render.
I added coverage for switching the active preview between providers and clearing it. The preview, sensor, and store tests pass in jsdom and Chromium. Prettier, ESLint, and TypeScript also pass.
🤖 Comment generated with Codex
| ref(node); | ||
| if (node) { | ||
| const observer = new (ownerWindow(node).MutationObserver)(refreshAutoScroll); | ||
| observer.observe(node, { |
There was a problem hiding this comment.
AI review worth considering:
always-on per-container MutationObserver overlapping the engine's drag-scoped idle observer - the hook wires new (ownerWindow(node).MutationObserver)(refreshAutoScroll) with { attributes, attributeFilter: ['class','style'], childList: true, subtree: true } for the whole time the component is mounted, while autoScroller.ts observeIdleMutations() (L852–876) already installs an observer with the identical config (attributeFilter: ['class','style'], childList: true, subtree: true) on doc.documentElement — a superset subtree — whenever the loop parks during a drag.
You can likely drop the hook-level observer entirely and let the engine's idle observer (which already covers this element's subtree and is scoped to parked-during-drag) be the single wake path; if a non-parked mid-drag restyle must be caught, attach/detach the hook's observer from the engine's drag start/end instead of component mount/unmount.
There was a problem hiding this comment.
Fixed in 6afef7575. The main concern was valid: the per-container observer no longer runs for the whole time DragAutoScroll.Root is mounted. The engine now starts it with a pointer drag and disconnects it when the drag ends. Both observer paths also share one options object.
I did not remove the per-container observer completely. The parked document observer cannot see inside shadow roots, and it is not present while auto-scroll is already moving. A class or style change during that time still needs to clear the cached overflow and direction values.
I added tests for the observer lifetime and for a container changing from scrollable to hidden during an active drag. The root suite passes in jsdom and Chromium, the manager registration suite passes, and Prettier, ESLint, and TypeScript pass.
🤖 Comment generated with Codex
| * here would strand the sensor and refuse every later pickup. Degrade to "nothing | ||
| * under the pointer" instead. | ||
| */ | ||
| export function deepElementFromPoint( |
There was a problem hiding this comment.
This method re-implements the jsdom-safe elementFromPoint wrapper that exists as getElementAtPoint. Can we update the method so it can be used here as well?
mnajdova
left a comment
There was a problem hiding this comment.
Some runtime performance considerations brought up by AI:
- autoScroller.ts:852-876 — parked-loop MutationObserver resets style caches + rebuilds the scroller chain on any page class/style mutation during a drag.
- cloneDragPreview.ts:184-193 — full computed-style copy per custom-element node can hitch the pickup frame on web-component-heavy sources.
- autoScroller.ts:584-604 — O(registered scrollers) rect reads per awake scroll frame; adopt the keyboard sensor's invalidation-based rect cache.
- utils.ts:126-160 — document/warn the per-frame double hit-test cost when preview content sets pointer-events: auto.
|
I followed up on this performance review in
🤖 Comment generated with Codex |
Deduplicate tab reordering, capture-handler prologues, and canDrop callbacks; use clamp and fastObjectShallowCompare from @base-ui/utils; read keyboard direction from the event instead of cursor deltas; move tree updates and queries into the model; only scan folders while the "Move to" submenu is open; stop re-creating the overflow ResizeObserver on every tree change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # docs/scripts/generateLlmTxt/index.mjs
Remove dead code (unused observeElement channel, getDeclaredDropTargetPayload, selectDragSource, documentBinding defer options, test-only preview shim), collapse duplicated logic (data attribute constants, terminal-leave payload, scroll notification, kind constructors, latched cleanups), and trim per-frame work (lazy consumer error message, scroll-extent check before computed style). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documentation
Feature comparison with dnd kit, React Aria, and Pragmatic drag and drop
Summary
Adds unstyled drag-and-drop primitives for React:
Draggable.Rootcreates a pointer and keyboard drag source.DropTarget.Rootaccepts matching drag kinds.DragAutoScroll.Rootconfigures a scroll container or implements custom scrolling.useDragMonitorobserves matching drags across the page.useDragDropManagerregisters sources, targets, monitors, and auto-scrollers imperatively.Basic usage
Draggable.Rootsupports keyboard dragging by default. Sources registered withuseDragDropManagermust be made focusable by the application.Drag state is exposed through data attributes and CSS variables. Movement modifiers apply to pointer and keyboard drags. Once auto-scroll is enabled, Base UI detects scrollable containers without individual registrations.