Add support for coordinates on dragstart, and related drag_to adjustments to work with react-dnd's HTML5 implementation#2837
Open
ragesoss wants to merge 5 commits into
Open
Conversation
Author
|
Hmm... i see this didn't pass in CI, so i'll try to get that sorted. |
…raggables While fixing a WikiEduDashboard feature spec for react-dnd drag-and-drop (teamcapybara#2662, react-dnd/react-dnd#1195), we found three bugs in Capybara's HTML5 drag emulation that prevent it from working with libraries like react-dnd that rely on DragEvent coordinates and modern component wrapper patterns. A workaround helper (`html5_drag_to`) was added downstream, but these fixes belong upstream so `drag_to` just works. ## Changes - `lib/capybara/selenium/extensions/html5_drag.rb`: - (Fix A) Set `clientX`/`clientY` on `dragstart` from the source element's bounding rect center. Previously defaulted to 0,0. - (Fix B) Set `clientX`/`clientY` on `dragenter` using the entry point already computed for `dragover`. Fixes teamcapybara#2662. - (Fix C) Check descendants for `[draggable="true"]` before walking up ancestors, so selecting a wrapper element (e.g. `<li>` around a draggable `<div>`) works instead of crashing. - (Fix D) Same descendant check in `LEGACY_DRAG_CHECK` auto-detection, saving the original element reference to avoid sloppy-mode JS `arguments` aliasing (the `do/while` loop reassigns the parameter, which mutates `arguments[0]` in non-strict functions). - `lib/capybara/spec/public/test.js`: Add `dragstart` coordinate logger, update `dragenter` logger to include coordinates, and extend all `dragstart`/`dragend` selectors to include `#drag_html5_wrapped`. - `lib/capybara/spec/views/with_js.erb`: Add `#drag_html5_wrapper` containing `#drag_html5_wrapped` test fixture for descendant lookup. - `lib/capybara/spec/session/node_spec.rb`: Three new specs — `dragstart` coordinates, `dragenter` coordinates, and wrapper-to-target drag — in the existing HTML5 context block. ## Process Sage had previously worked with Claude to fix a pending drag-and-drop spec in WikiEduDashboard by writing a custom `html5_drag_to` helper that dispatches real DragEvents with coordinates and resolves draggable descendants. That downstream fix revealed three upstream Capybara bugs. In a new session, Claude applied the plan from the prior session (which had been saved as a Claude Code plan file). All existing drag tests passed on the first run. The new wrapper test failed because the `LEGACY_DRAG_CHECK` IIFE used `arguments[0]` after a `do/while` loop had reassigned the named parameter to null — in sloppy-mode JS, `arguments` is a live alias to named parameters, so `arguments[0]` was also null. Saving to `originalEl` before the loop fixed it. Second run: 24/24 drag tests green. Three user messages, all terse. (Commit message written by Claude Code.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The dragLeave function unconditionally dispatched a dragleave event before conditionally dispatching drop, producing the sequence dragstart → dragenter → dragover → dragleave → drop → dragend. In real browser HTML5 drag-and-drop, dragleave does not fire on the drop target before drop — the correct sequence is dragstart → dragenter → dragover → drop → dragend. dragleave only fires when dragging away without dropping. This incorrect ordering breaks react-dnd, whose handleTopDragLeave calls this.actions.hover([]) to clear all hover targets. By the time the drop event fires, react-dnd has no target to drop onto, so the drop is silently ignored. This was confirmed by instrumentation: dragover events had defaultPrevented=true (react-dnd processing them), but the subsequent drop had defaultPrevented=false (react-dnd had already disengaged). The fix makes dragleave and drop mutually exclusive: fire drop when the target accepted the drag (dragover was defaultPrevented), fire dragleave otherwise. The existing "preserve clientX/Y from last dragover event" spec is updated to no longer assert on DragLeave during a successful drop, matching the corrected behavior. ## Breaking change note This changes the emulated event sequence to match real browser behavior. Tests that assert on DragLeave appearing before Drop in a successful drag-and-drop will need updating. This is unlikely to affect most users since dragleave also fires when dragging away without dropping, making it an unreliable signal for drop completion. ## Process Sage tested the prior commit's coordinate and descendant fixes against the WikiEduDashboard react-dnd timeline spec and found that drops were still silently ignored. Instrumentation revealed the dragleave → drop ordering as the cause. One Capybara spec failed after the fix (it asserted on the old incorrect sequence); updated it. Second run: 24/24 drag tests green. (Commit message written by Claude Code.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In real browser drag-and-drop, drag events (dragenter, dragover, drop, dragleave) fire on the element actually under the pointer, not the top-level drop zone element. They then bubble up to ancestor listeners. Capybara's emulation dispatched all target-side events directly on the target argument, which meant events never reached listeners on descendant elements — they bubbled up, not down. This breaks react-dnd, which attaches dragover/drop listeners on the specific component element (often a child of the Capybara-selected wrapper). The dragover handler would call preventDefault() on the component's own element, but since Capybara dispatched on the wrapper, the component's listener never fired and the drop was rejected. ## Changes - `lib/capybara/selenium/extensions/html5_drag.rb`: Add `dispatchOnElement(pt, event)` helper that uses `document.elementFromPoint(pt.x, pt.y)` to find the actual element under the pointer, falling back to `target` when the point is outside the target (e.g. at the exact edge of the bounding rect). Replace all five `target.dispatchEvent(...)` calls in `dragEnterTarget`, `dragOnTarget`, and `dragLeave` with `dispatchOnElement` calls. ## Process Sage tested the prior two commits (coordinates + dragleave fix) against the WikiEduDashboard react-dnd timeline spec from a parallel session. The drag events had correct coordinates and the dragleave ordering was fixed, but drops were still silently rejected. Instrumentation showed dragover's defaultPrevented was false — react-dnd's listener on the descendant element never received the event because it was dispatched on the ancestor wrapper. The fix to use elementFromPoint was identified in the other session. First attempt here dispatched on the raw elementFromPoint result, which broke three existing specs: the entry point coordinates land on the exact edge of the target rect, and elementFromPoint at that pixel can return an element outside the target. Adding the `target.contains(el)` guard fixed it. Second run: 24/24 drag tests green. (Commit message written by Claude Code.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
react-dnd's handleTopDragStart defers source publication via setTimeout(fn, 0) — `publishDragSource` triggers a React re-render that transitions the source component to isDragging: true, which can replace DOM nodes. Capybara's script scheduled dragEnterTarget with setTimeout(fn, step_delay) directly after dragstart, so with the default 50ms delay, publishDragSource fired during the gap and replaced DOM nodes that the script still held references to. The dragend event was dispatched on a detached node and never bubbled to the window. This was confirmed by varying the delay parameter: delay: 0 passed (all setTimeout(0) callbacks fire in FIFO order after publishDragSource), while delay: 0.01 and the default 0.05 both failed. ## Changes - `lib/capybara/selenium/extensions/html5_drag.rb`: After dispatching dragstart, yield once to the event loop with setTimeout(fn, 0) before scheduling the delayed dragEnterTarget. This ensures any library work deferred via setTimeout(0) after dragstart (like react-dnd's publishDragSource) completes before the drag sequence continues. ## Process Sage tested the prior three commits against WikiEduDashboard's react-dnd timeline spec from a parallel session. The drag initiated correctly but the drop still failed intermittently. Instrumentation showed dragend events disappearing from the trace — dispatched on a detached DOM node after react-dnd's publishDragSource re-rendered the component tree during the 50ms gap. The double-setTimeout pattern (yield once, then schedule with delay) was identified as the fix. One run: 24/24 drag tests green. (Commit message written by Claude Code.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
react-dnd's scheduleHover uses requestAnimationFrame to batch hover actions. Each dragover triggers RAF → React state update → re-render → useEffect cleanup (removes drop target listeners) → useEffect re-attach. With the default delay of 50ms between events, each gap allows RAF and effects to run, so events arriving during listener reattachment miss the per-target handlers entirely. With delay: 0, all events fire synchronously in one burst before any RAF or effect runs, keeping old listeners intact throughout the sequence. Rather than changing the default (which could affect other use cases that rely on the delay for testing intermediate drag states), this adds documentation to the :delay option on drag_to noting that libraries using requestAnimationFrame for hover scheduling may require delay: 0. ## Changes - `lib/capybara/node/element.rb`: Expand the :delay YARD doc to note that react-dnd and other RAF-based libraries may require `delay: 0` to prevent listeners from being removed and reattached mid-sequence. ## Process Sage tested the prior commits against WikiEduDashboard's react-dnd timeline spec from a parallel session. With the default delay (0.05), drops still failed — instrumentation showed RAF-scheduled hover actions and React effects running between events, detaching and reattaching listeners mid-sequence. With delay: 0, all five timeline specs passed. With this full set of fixes and `delay: 0`, Capybara's built-in `drag_to` works end-to-end with react-dnd — the downstream `html5_drag_to` workaround helper is no longer needed. Sage decided to document the workaround rather than change the default. (Commit message written by Claude Code.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ragesoss
force-pushed
the
drag-to-coordinates
branch
from
bf2fb16 to
e7004af
Compare
Author
|
Okay! Here's a passing build that has this set of changes on top of the separate PR focused on making the build work: https://github.com/ragesoss/capybara/actions/runs/24291646486 And here's the build with just the changes from this PR, which fails but I think only for already-broken things: https://github.com/ragesoss/capybara/actions/runs/24291591529 |
Author
|
@simi as with the other commit, this one is all agentic code. i've tested it against my use case (and it doesn't break existing Capybara tests), but if it's not worth it to review, i completely understand; feel free to close. |
Contributor
|
@ragesoss I'll check, seems like a good feature. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've had some drag-and-drop capybara specs in my test suite for https://github.com/WikiEducationFoundation/WikiEduDashboard that I had to disable years ago when we upgraded
react-dndto a version that uses HTML5. At the time, I couldn't find a way to make those specs work. I took a pass with Claude Code today to see if I could find a way to get the specs working, and ended up with a drag helper that worked, in place of Capybara'sdrag_to.I'm happy to keep that, but I figured since I had a good test case, I would try implementing an upstream fix to make Capybara's
drag_tocompatible with how my app usesreact-dnd. Here's where I ended up. (Happy to squash these if you'd prefer, but I thought it better to start with the full history of the AI-written commits to be as transparent about the process for getting here as possible.)I've tested this change locally, and I'm able to successfully run my long-disabled drag-and-drop spec, as long as
delay: 0is added to thedrag_tocalls. It tool several iterations going between making changes to Capybara, making sure the old and new Capyabara specs still worked, and testing the changes against the spec in my project.I think this fixes #2662, which was part of the root problem (but not all of it) for why my spec didn't work with HTML5 react-dnd.
Note the potential breaking change in the second commit: b305e29
I don't know enough about Capybara or drag-and-drop to be confident about whether this is safe or reasonable, but it seems to make sense and I've watched both my own tests and the affected parts of the Capybara test suite run successfully with these changes.