Defer initialization to the next rAF#1044
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a defer attribute to <lexxy-editor> to postpone Lexical root attachment and initial value loading until the next animation frame, reducing layout-flush lag during DOM morphing (e.g., Turbo morph navigation).
Changes:
- Introduces
deferhandling inLexicalEditorElementto delaysetRootElement, initial value load, autofocus, and initialization dispatch sequencing. - Updates editor internals to bind certain event listeners to
editorContentElementinstead ofeditor.getRootElement()(supports deferred root attachment). - Adjusts drag-and-drop handler registration to use
editorContentElementas the event target.
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 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/elements/editor.js | Adds defer attribute behavior and gates root/value initialization accordingly. |
| src/editor/selection.js | Registers internal move-selection listener on editorContentElement instead of root element. |
| src/editor/command_dispatcher.js | Registers drag/drop listeners on editorContentElement to work when root is deferred. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.#handleAutofocus() | ||
| if (this.hasAttribute("defer")) { | ||
| requestAnimationFrame(() => { | ||
| if (!this.isConnected) return |
There was a problem hiding this comment.
I don't love these early returns, I would just use the positive condition branch explicitly: if (this.connected)...
There was a problem hiding this comment.
Also do we need this isConnected check in this branch at all?
There was a problem hiding this comment.
Oh, I thought because it was the first in the callback it would be fine.
That guards against stale fires of the callback - the editor connecting and disconnecting before the callback fires.
After the last commit that addresses the rAF leak that Copilot raised we no longer need this. Removing.
| } | ||
|
|
||
| disconnectedCallback() { | ||
| this.#cancelDeferredInit() |
There was a problem hiding this comment.
I think this could be too defensive (cancel the animation frame on disconnect)
There was a problem hiding this comment.
Removed it. I though it would be semantically cleaner - everything that's in-flight is stopped on disconnect.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/editor/command_dispatcher.js:333
#registerDragAndDropHandlers()now binds drag/drop listeners toeditorContentElement(to support deferredsetRootElement()), but the drag handlers still callthis.editor.getRootElement().classList...without a null check. Withdefer,getRootElement()will be null until the next rAF, so a drag event during that window would throw. UseeditorElement.editorContentElement(or therootvariable) consistently, or guardgetRootElement()against null.
#registerDragAndDropHandlers() {
if (this.editorElement.supportsAttachments) {
this.dragCounter = 0
const root = this.editorElement.editorContentElement
this.#listeners.track(
registerEventListener(root, "dragover", this.#handleDragOver.bind(this)),
registerEventListener(root, "drop", this.#handleDrop.bind(this)),
registerEventListener(root, "dragenter", this.#handleDragEnter.bind(this)),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/editor/command_dispatcher.js:335
- Drag/drop listeners are now attached to
editorContentElementbefore Lexical’s root element is set, but the handlers still callthis.editor.getRootElement().classList...(e.g. in#handleDragEnter/#handleDrop). BecausegetRootElement()is null until the first rAF, a drag event before that frame will throw. UseeditorElement.editorContentElement(or the capturedroot) for class toggles, or guard against a null root element.
#registerDragAndDropHandlers() {
if (this.editorElement.supportsAttachments) {
this.dragCounter = 0
const root = this.editorElement.editorContentElement
this.#listeners.track(
registerEventListener(root, "dragover", this.#handleDragOver.bind(this)),
registerEventListener(root, "drop", this.#handleDrop.bind(this)),
registerEventListener(root, "dragenter", this.#handleDragEnter.bind(this)),
registerEventListener(root, "dragleave", this.#handleDragLeave.bind(this))
)
monorkin
changed the title
samuelpecher
left a comment
samuelpecher
left a comment
There was a problem hiding this comment.
Great find! I've just got one suggestion about hooking into Lexical's root element registration rather than holding the reference on the element.
| if (this.editorElement.supportsAttachments) { | ||
| this.dragCounter = 0 | ||
| const root = this.editor.getRootElement() | ||
| const root = this.editorElement.editorContentElement |
There was a problem hiding this comment.
This gives me pause. Holding the reference to the content element separately from Lexical breaks the concepts of observing Lexical's RootElement.
I think the right move is to defer the registration of listeners by registering a root element listener, where Lexical will provide the listener upon registration. That will ensure the listeners are attached to the linked root element and the timing of listener registration is all controlled through setRootElement.
So roughly:
#registerSomeDomHandlers() {
this.#listeners.track(this.editor.registerRootElementListener(({ rootElement }) => {
// attach handlers to root element
}))
}
|
Putting this on hold for a bit. I did an extensive test yesterday and this seems to only help in the worst-case scenario. Something else is causing the calculations to hang. This change might not be wroth the performance gain it brings. |
There was a problem hiding this comment.
@monorkin I think initializing event listeners via a root listener is worth it on it's own, and performance gains at the worst end of the tail are the most appreciated. Unless this causes us other issues I'd be for a merge.
@jorgemanrubia would you concur?
| this.#setItemPositionValues() | ||
| this.#monitorSelectionChanges() | ||
| this.#monitorHistoryChanges() | ||
| this.#refreshToolbarOverflow() |
There was a problem hiding this comment.
Oh great! I'll pop my last commit.
Fixes the possibility of an external piece of code reading and setting the value only for it to get overridden one init occurs. And removes the defensive rAF cleanup - if it fires nothing happens, and on reconnect a new rAF is scheduled.
Move #loadInitialValue, form-value sync, validity, and #initialValue capture back to synchronous #initialize. Only setRootElement (the layout-flush culprit) and the post-attach work (autofocus, lexxy:initialize dispatch) stay in the rAF. This fixes three issues Codex's review surfaced: - Background tabs paused rAF could leave forms with empty values and stale validity, since form-value sync ran from the rAF's update listener. - formResetCallback used #initialValue, which was set inside the rAF's #loadInitialValue. Reset before the first frame blanked the editor. - A stale rAF firing between disconnect and reconnect re-ran #loadInitialValue on the disposed editor and cleared valueBeforeDisconnect, poisoning #valueLoaded and the saved value across the cycle. Wrap setRootElement with editor.setEditable(false/true) in #mountRoot. Lexical clears _updateTags between commits, so the SKIP_DOM_SELECTION_TAG that `set value` adds is gone by the time setRootElement commits; toggling editable short-circuits the DOM-selection sync block in $commitPendingUpdates instead, preventing focus theft when the editor state has a selection. Switch selection.js and command_dispatcher.js listener registrations to editor.registerRootListener so handlers are wired by Lexical when it announces the root, instead of pre-binding to a content element we only know is the future root. Split #initializeEventDispatched and #editorInitializedDispatched so lexxy:initialize fires when registerAdapter races with the rAF (the single-flag version silently suppressed it). Add regression tests for sync value load, formResetCallback before first frame, stale-rAF poisoning, registerAdapter race, and external-value focus theft.
Immediate initialization can sometimes cause lag spikes if the parent element is being morphed at the same time. This PR moves initialization to the next animation frame to give the DOM time to flush the pending layout before Lexical attempts to compute geometry.
The root cause of the lag is a forced layout flush caused by Lexical. It has a selection handler that calls
Selection.anchorNodeduringsetRootElement. This forces the browser to immediately flush the layout, persiste pending mutations, and perform style invalidation, all of which pushes the paint back causing causing a visual hang.This often triggers on navigation with Turbo morph enabled. As morph updates the DOM, Lexxy initializes, and forces the browser to flush all the changes and perform style invalidation, causing the navigation to lag by 50-100ms.
By deferring initialization, navigation is able to complete as normal, the page is laid out and painted, and then Lexxy initializes, and does so much quicker as it doesn't have to perform additional layouts.