Fix text drag crash in attachment drag handler#969
Conversation
Native text drag-and-drop within contenteditable fires insertFromDrop then deleteByDrag, which can leave Lexical's selection referencing nodes removed during the operation. This causes "Point.getNode: node not found" crashes on subsequent actions like Enter or code formatting. Prevent text D&D at DRAGSTART_COMMAND (NORMAL priority) so the browser never initiates the drag. Attachment D&D is unaffected — it's handled at HIGH priority by AttachmentDragAndDrop, which returns true and stops the command chain before this handler runs.
Manual Validation: ValidatedTyped text in the editor, selected a word, and dispatched a Production (https://basecamp.github.io/lexxy/try-it.html): The Local (http://lexxy.localhost:3100 on branch |
When dragging text (not an attachment), event.target can be a text node which has no .closest() method. Return false early to let the command chain continue to the normal-priority handler that prevents native text drag-and-drop.
c0f0b7e to
80b9a0a
Compare
Use optional chaining on event.target.closest() in the attachment drag handler to gracefully handle text node targets (which lack the .closest method). Remove the DRAGSTART_COMMAND handler that was blocking all native text drag-and-drop.
When dragging selected text, event.target can lack .closest() (e.g. text nodes). Use optional chaining in the attachment drag handler to gracefully skip non-element targets instead of crashing. Remove the DRAGSTART_COMMAND handler that blocked all native text D&D — the actual fix is in the attachment handler, not in preventing drag.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the attachment drag handler that could occur when dragging selected text (where event.target may be a Text node without .closest()), and adds a browser regression test for the scenario.
Changes:
- Guard attachment drag-start logic by using optional chaining on
.closest()calls to avoidTypeErroron non-Element targets. - Add a Playwright test covering “text drag-and-drop followed by code formatting” to prevent regressions.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/editor/attachments/drag_and_drop.js |
Prevents crashes by safely calling .closest() when event.target isn’t an Element. |
test/browser/tests/formatting/drag_and_drop_formatting.test.js |
Adds a regression test for the text-drag + code-formatting crash case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Perform a drag gesture using Playwright mouse API | ||
| const wordBound = await page.evaluate(() => { | ||
| const sel = window.getSelection() | ||
| if (!sel.rangeCount) return null | ||
| const range = sel.getRangeAt(0) | ||
| const rect = range.getBoundingClientRect() | ||
| return { x: rect.x + rect.width / 2, y: rect.y + rect.height / 2 } | ||
| }) | ||
|
|
||
| if (wordBound) { | ||
| await page.mouse.move(wordBound.x, wordBound.y) | ||
| await page.mouse.down() | ||
| await page.mouse.move(wordBound.x + 30, wordBound.y, { steps: 5 }) | ||
| await page.mouse.up() | ||
| } | ||
|
|
There was a problem hiding this comment.
This test can pass without ever exercising a text drag: the drag gesture is conditional on wordBound, and there’s no assertion that a dragstart actually fired. That makes the regression coverage for the original “closest is not a function” crash unreliable/flaky. Consider (1) failing the test if the selection bounds can’t be computed, and (2) explicitly triggering/observing a dragstart from a Text node (e.g., dispatching a bubbling DragEvent with a DataTransfer on the selected range’s text node) so the handler path is guaranteed to run.
| // Perform a drag gesture using Playwright mouse API | |
| const wordBound = await page.evaluate(() => { | |
| const sel = window.getSelection() | |
| if (!sel.rangeCount) return null | |
| const range = sel.getRangeAt(0) | |
| const rect = range.getBoundingClientRect() | |
| return { x: rect.x + rect.width / 2, y: rect.y + rect.height / 2 } | |
| }) | |
| if (wordBound) { | |
| await page.mouse.move(wordBound.x, wordBound.y) | |
| await page.mouse.down() | |
| await page.mouse.move(wordBound.x + 30, wordBound.y, { steps: 5 }) | |
| await page.mouse.up() | |
| } | |
| await page.evaluate(() => { | |
| window.__textDragStart = { | |
| fired: false, | |
| targetNodeType: null, | |
| } | |
| document.addEventListener( | |
| "dragstart", | |
| (event) => { | |
| window.__textDragStart = { | |
| fired: true, | |
| targetNodeType: event.target?.nodeType ?? null, | |
| } | |
| }, | |
| { capture: true, once: true } | |
| ) | |
| }) | |
| const wordBound = await page.evaluate(() => { | |
| const sel = window.getSelection() | |
| if (!sel || !sel.rangeCount) return null | |
| const range = sel.getRangeAt(0) | |
| const rect = range.getBoundingClientRect() | |
| if (!rect || (rect.width === 0 && rect.height === 0)) return null | |
| return { | |
| x: rect.x + rect.width / 2, | |
| y: rect.y + rect.height / 2, | |
| hasTextTarget: range.startContainer?.nodeType === Node.TEXT_NODE, | |
| } | |
| }) | |
| expect(wordBound).not.toBeNull() | |
| expect(wordBound?.hasTextTarget).toBe(true) | |
| const dragStartDispatched = await page.evaluate(() => { | |
| const sel = window.getSelection() | |
| if (!sel || !sel.rangeCount) return false | |
| const range = sel.getRangeAt(0) | |
| const textNode = range.startContainer | |
| if (!textNode || textNode.nodeType !== Node.TEXT_NODE) return false | |
| const event = new DragEvent("dragstart", { | |
| bubbles: true, | |
| cancelable: true, | |
| dataTransfer: new DataTransfer(), | |
| }) | |
| return textNode.dispatchEvent(event) | |
| }) | |
| expect(dragStartDispatched).toBe(true) | |
| const dragStartState = await page.evaluate(() => window.__textDragStart) | |
| expect(dragStartState).toEqual({ | |
| fired: true, | |
| targetNodeType: 3, | |
| }) | |
| // Perform a drag gesture using Playwright mouse API as part of the | |
| // end-to-end interaction, but do not rely on it for coverage. | |
| await page.mouse.move(wordBound.x, wordBound.y) | |
| await page.mouse.down() | |
| await page.mouse.move(wordBound.x + 30, wordBound.y, { steps: 5 }) | |
| await page.mouse.up() |
Summary
event.targetcan be a text node which lacks.closest(). The attachment drag handler calledevent.target.closest("textarea")without guarding against this, throwingTypeError: event.target.closest is not a function.event.target.closest?.()) to gracefully skip non-element targets instead of crashing.DRAGSTART_COMMANDto block all native text D&D) which broke text drag-and-drop entirely.Fixes Code formatting not working