[lexical-rich-text][lexical-plain-text] Spec hardening: call event.preventDefault() in dragover for HTML5 DnD compliance#8663
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The decorator branch right after the new preventDefault is dead: line 1248 already calls preventDefault unconditionally, so the caretFromPoint / $isDecoratorNode block below it only re-calls it as a no-op. The // Won't work for DecoratorNode nor it's relevant comment no longer describes what runs either. Dropping the block and its comment would leave just the new line.
Separately, the new test asserts event.defaultPrevented === true on a synthetic dragover, but the existing dragSelectionToOffset helper (lines 49-71) only dispatches dragstart / drop / dragend — never dragover. If a future change re-breaks the gate, every existing drag test still passes; only the new one fails. Dispatching dragover inside that helper would cover the existing suite.
|
Addressed: removed the dead decorator block and its stale comment. Also wired the |
|
On the Before behavior — I instrumented the current So Could you share the Firefox version + setup where the original report reproduces? |
|
Following up on the Before check above, on the test side: the new |
potatowagon
left a comment
There was a problem hiding this comment.
Reviewed by Navi (Tater Thoughts Bobblehead) on behalf of @potatowagon.
LGTM — Firefox drag-and-drop fix.
What I verified:
- Two commits: (1) adds
event.preventDefault()inDRAGOVER_COMMANDfor both plain-text and rich-text editors, (2) refines the e2e test with an explicit dragover dispatch +defaultPreventedassertion as a regression guard. - Rich-text handler simplified from conditional (DecoratorNode-only) preventDefault to unconditional, which matches the HTML5 DnD spec — dragover must be cancelled for drop to fire.
- Plain-text handler inlines the file-drop guard correctly.
- CI all green (core unit 22.x + 24.x, browser, integrity, e2e canary chromium, CLA, Vercel).
No concerns. Safe to land.
|
@mayrang Good catch on the version gap. The original report was against |
|
Checked git history: #8373 already fixed the user-visible symptom. Before that PR the |
|
Followup on your own git-history check above. Since you confirmed #8373 already covered the user-visible symptom of #8014:
What the PR actually does on top of main is spec hardening — calling Final call is the maintainers'. My read is that the title, body, and Before/After all need to change either way — retitle around spec hardening, or close if maintainers consider #8373 covers it. As-is, the earlier review back-and-forth went into framing that didn't match the diff. |
event.preventDefault() in dragover|
Updated the title and description: the original framing ("Fix Firefox drag-and-drop") was based on a misdiagnosis. After checking git history, #8373 already fixed the user visible symptom by actually processing the drop in DROP_COMMAND. This PR is spec compliance hardening, not a bug fix. Title, body, and test plan updated to Happy to close if the maintainers consider #8373 sufficient coverage. |
| // Files check is inlined (rather than using eventFiles() from | ||
| // @lexical/rich-text) because plain-text cannot import from rich-text. | ||
| const dataTransfer = event.dataTransfer; | ||
| if (dataTransfer !== null && dataTransfer.types.includes('Files')) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think the best solution for something like this would be to move the function to @lexical/clipboard or @lexical/utils which both rich and plain text packages already depend on.
rich-text can just re-export it for compatibility
73de687 to
acf0a6a
Compare
…fault() in dragover to fix Firefox drag-and-drop (facebook#8014) Per the HTML5 DnD spec, a contenteditable div must call event.preventDefault() in its dragover handler to become a valid drop target — without it, the browser never fires the drop event. Chrome ignores this gap; Firefox enforces it strictly. registerRichText's DRAGOVER_COMMAND handler only called preventDefault() for DecoratorNode targets. For plain text drags it did nothing, so Firefox silently swallowed the drop and $handleRichTextDrop never ran. registerPlainText had no DRAGOVER handler at all, producing the same failure. Fix: add an unconditional event.preventDefault() in the rich-text handler (after the existing file-transfer guard) and add a new DRAGOVER handler to plain-text with the same guard. Also adds an e2e test that directly asserts dragover sets event.defaultPrevented, covering the gap left by the existing tests which bypass dragover by dispatching drop directly.
- Remove dead decorator-node block from rich-text DRAGOVER handler - Embed defaultPrevented assertion in dragSelectionToOffset helper so all four drag-drop tests fail if the Firefox gate regresses - Remove now-redundant standalone dragover preventDefault test - Add comment on plain-text Files check explaining the dependency direction constraint (cannot import eventFiles from rich-text)
… eventFiles to @lexical/utils Move eventFiles from @lexical/rich-text to @lexical/utils so both rich-text and plain-text can share it. Rich-text re-exports it from @lexical/utils for backwards compatibility. Plain-text now uses eventFiles instead of an inlined file-transfer check.
73de687 to
6dcf415
Compare
Description
Per the HTML5 DnD spec, an element must call
event.preventDefault()in itsdragoverhandler to signal that it accepts drops. The native auto-accept exception applies only to<textarea>and<input type="text">, not tocontenteditabledivs.The user-visible drag-and-drop failure (#8014) was already fixed by #8373, which rewired the
DROP_COMMANDhandler to actually process the drop. Before #8373 the handler returnedtruewithout doing anything, so text never moved regardless of browser.This PR hardens the dragover layer to match the spec:
registerRichText: add unconditionalevent.preventDefault()after the existing file-transfer guard inDRAGOVER_COMMAND. The decorator-specific call that was already there is now removed as redundant.registerPlainText: add a newDRAGOVER_COMMANDhandler with a file-transfer guard followed byevent.preventDefault().The change is safe and correct on all current browsers. It also guards against a future Firefox version enforcing the spec strictly, which would silently break drag-and-drop again.
Related: #8014