[lexical-rich-text][lexical-plain-text] Bug Fix: Fix Firefox drag-and-drop by calling event.preventDefault() in dragover#8663
Conversation
…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.
sahiee-dev
requested review from
acywatson,
etrepum,
fantactuka,
ivailop7,
potatowagon and
zurfyx
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
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.
- 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)
|
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
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 |
3 participants
Description
Per the HTML5 DnD spec, an element must call
event.preventDefault()in itsdragoverhandler to signal that it accepts drops, without it, the browser never fires thedropevent. The native auto accept exception applies only to<textarea>and<input type="text">, not tocontenteditabledivs. Chrome papers over this with internal contenteditable handling; Firefox enforces the spec strictly.Current behavior:
registerRichText'sDRAGOVER_COMMANDhandler only calledevent.preventDefault()when the drag target was aDecoratorNode. For plain text drags it did nothing, so Firefox silently refused the drop and$handleRichTextDropnever ran.registerPlainTexthad noDRAGOVER_COMMANDhandler at all, producing the same failure.TextDragDrop.spec.mjsdispatches .........dragstart→dropdirectly, bypassingdragoverentirely, so the browser gate never came into play.Changes:
registerRichText: add unconditionalevent.preventDefault()after the existing file transfer guard inDRAGOVER_COMMAND. The decorator specific call that was already there is now a harmless no op.registerPlainText: add a newDRAGOVER_COMMANDhandler with the same file transfer guard followed byevent.preventDefault().TextDragDrop.spec.mjs: add a test that dispatches a syntheticdragoverwithtext/plaindata and assertsevent.defaultPrevented === true, covering the gap the existing tests could not catch.Closes #8014
Test plan
Before
Drag and drop of selected text in a Lexical editor on Firefox: text stays in place, is not moved to the drop location. The
dropevent never fires becausedragovernever calledpreventDefault().Automated:
TextDragDrop.spec.mjs, new testdragover calls event.preventDefault() for text dragsfails (returnsfalse).After
Drag-and-drop of selected text works in Firefox: text is cut from the source position and inserted at the drop location, matching Chrome behavior.
Automated: all five tests in
TextDragDrop.spec.mjspass across Chromium, Firefox, and WebKit.