lexical icon indicating copy to clipboard operation
lexical copied to clipboard

Bug: Cannot toggle editor isEditable using react + react-dom v19 beta

Open acusti opened this issue 1 year ago • 6 comments

Lexical version: v0.14.5

Steps To Reproduce

  1. install react + react-dom v19 beta (latest dependency string is 9.0.0-beta-5d29478716-20240506)
  2. use editor.setEditable() API to toggle editable state

Link to code example:

https://stackblitz.com/~/github.com/acusti/lexical-rich-toggle-editable

if you switch the initial isEditable value from false to true, the editor’s content editable DOM element will remain always contenteditable="true"

if you swap the react + react-dom dependency strings from the v19 beta to v18.3.1, the example behaves correctly

(note that i had to update the npm install script to npm install --legacy-peer-deps in order to prevent issues with lexical’s peer dependencies that specify react 18.x, but that configuration change is already applied in the stackblitz code example above)

The current behavior

the contenteditable state of the editor’s <ContentEditable /> component never changes from its initial value, even when editor.setEditable() is called with a changed value. this is because the registered editable listener is never invoked, though when i create a local listener via initialConfig’s editorState callback, that listener is invoked every time the editor’s editable state changes.

The expected behavior

i expect the ContentEditable component to get the update editable state, as occurs when not using the v19 beta.

acusti avatar May 06 '24 16:05 acusti

note that i believe this isn’t specific to changes to the editor’s editable but is actually a more fundamental issue where a number of the editor closure utilities don’t work as expected. for example:

import { $selectAll } from 'lexical';
// …
editor.update(() => {
   $selectAll();
});

doesn’t work using the react v19 beta but does work using v18.3.1. however, toolbars that rely on const [editor] = useLexicalComposerContext(); to apply changes to the editor do work in the v19 beta.

acusti avatar May 06 '24 17:05 acusti

I suspect what's happening here is due to some behavior change in React.StrictMode and the order of effects since it runs them all twice. If you build it and preview it works. In your listener it's not registering for updates during an effect so it's not subject to the double-running.

etrepum avatar May 06 '24 18:05 etrepum

To be clear what I think is happening is this:

  1. Effect A.dev is called, registering an editable listener
  2. Effect A.dev listener is unregistered
  3. Effect B.dev is called, setting the editor state, triggering any editable listeners (but not A.dev because it already unregistered)
  4. Effect A.prod is called, registering an editable listener
  5. Effect B.prod is called, setting the editor state, NEVER triggering any editable listeners because the editor state already matches due to B.dev

I suspect the order of 2. and 3. are probably swapped in React <19

etrepum avatar May 06 '24 18:05 etrepum

@etrepum that seems plausible to me. however, i just re-read the react 19 beta upgrade guide and didn’t see any mention of changes to the order of effect / effect cleanup invocation. they do mention “StrictMode Changes”, but it doesn’t really seem related to me.

When double rendering in Strict Mode in development, useMemo and useCallback will reuse the memoized results from the first render during the second render. Components that are already Strict Mode compatible should not notice a difference in behavior.

As with all Strict Mode behaviors, these features are designed to proactively surface bugs in your components during development so you can fix them before they are shipped to production. For example, during development, Strict Mode will double-invoke ref callback functions on initial mount, to simulate what happens when a mounted component is replaced by a Suspense fallback.

also, do you think i should open a bug on facebook/react repo about this?

acusti avatar May 06 '24 18:05 acusti

Probably, I guess? If this is indeed the case I'm sure it would break other code that is otherwise correct as well

etrepum avatar May 06 '24 19:05 etrepum

Okay looking closer at the React changes and the bug it's definitely a StrictMode issue and it's caused by the change that you referenced:

useMemo and useCallback will reuse the memoized results from the first render during the second render

Previously, separate LexicalEditor instances would be created for first and second renders, but now they are sharing the same LexicalEditor so chaos ensues. Perhaps a workaround that could happen in the core would be to change the 'unsafe' x = useMemo(fn) case(s) to [x] = useState(fn). Other than disabling strict mode, or not using React 19, I'm not sure what you could do to sort this out until some workaround is in place in @lexical/react.

etrepum avatar May 06 '24 19:05 etrepum

Mmmm.... Sounds like a storm that's coming... @etrepum do you think it's gonna be a big issue when React 19 comes out of beta?

StyleT avatar May 06 '24 20:05 StyleT

@StyleT yes, I think it's probably already an issue for next.js that we have had reports about but I think people just did their workarounds (disabling strict mode probably) and left it alone. Currently looking at a fix, I think it is probably as simple as switching to useState, should have a PR to start testing today

etrepum avatar May 06 '24 20:05 etrepum

I think the example code is also subtly wrong here with regard to effects and StrictMode because of the editorRef is being passed around with a Ref and the effect does not depend on the editor, so if the editor changes then the effect doesn't run again.

etrepum avatar May 06 '24 21:05 etrepum

I think we can fix the issue of people using the function version of editorState in this way, but any plug-in that mutates the editor in a way that is not undone by its cleanup function is going to be problematic, because those children component are going to be running their effects twice with the same editor. Not much we can do about that other than warn people about this classic trap.

const [editor] = useLexicalComposerContext();
useEffect(() => {
  // this happens twice in StrictMode… which is expected, but problematic if this is a mutation
}, [editor]);

etrepum avatar May 06 '24 22:05 etrepum

I think the example code is also subtly wrong here with regard to effects and StrictMode because of the editorRef is being passed around with a Ref and the effect does not depend on the editor, so if the editor changes then the effect doesn't run again.

i based the example on code examples i found of initialConfig usage where the editorState callback was used to capture a ref to the editor instance, so i assumed the editor instance was stable for the lifetime of the LexicalComposer component. @etrepum is that not a safe assumption? if so, is there a way to include logic in the component that operates on the editor instance, or does all such logic need to move into a child component that can then use const [editor] = useLexicalComposerContext()?

update i modified the code example to introduce EditablePlugin.tsx, which takes props.isEditable and updates the editor in a useEffect:

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();

    useEffect(() => {
        editor.setEditable(isEditable);
    }, [editor, isEditable]);

    return null;
}

acusti avatar May 06 '24 22:05 acusti

All logic in a react app should be using const [editor] = useLexicalComposerContext(), there's an EditorRefPlugin for that purpose. Is that example in the docs or the repo somewhere?

etrepum avatar May 06 '24 22:05 etrepum

@etrepum couldn’t find the example digging through my browser history just now but i will let you know if i find it!

acusti avatar May 06 '24 23:05 acusti

@etrepum just saw your comment closing #6041:

After thinking about this some more I think the approach we have is probably correct enough

i updated my code example to use the const [editor] = useLexicalComposerContext() approach (see EditablePlugin.tsx), and the example is still quite broken with the react v19 beta (editor.setEditable() does nothing, editor.update(() => { $selectAll(); }) has no effect, probably other things…). consider that, it seems to me that the approach isn’t correct enough yet. am i misunderstanding your comment?

acusti avatar May 07 '24 17:05 acusti

Can you describe exactly what the problem is with your current example? If you click on it, it becomes editable, although a second click is required to give the editor focus with the way that this example is implemented.

Selection does not reconcile to the DOM if the editor is not editable, which is probably the reason why you think that is broken.

etrepum avatar May 07 '24 18:05 etrepum

@etrepum apologies for the inaccurate comment, i hadn’t realized that the contenteditable HTML attribute is now updating. i’ve been trying to keep my actual reproduction in my own app and the minimal reproducible demo in sync but i hadn’t restored the focus logic to the minimal repro since the last refactor. thanks for your assistance with figuring out the core issue!


fwiw, i added the editor focus / $selectAll logic to EditablePlugin.tsx and no matter where i attempted to apply it in the editor lifecycle, i couldn’t get it to work. i tried invoking editor.focus() immediately after calling editor.setEditable(true), i tried putting it in a editor.update(() => { editor.focus() }) closure, and i tried putting it in a registerEditableListener. here’s what that looked like:

import { useLexicalComposerContext } from '@lexical/react/LexicalComposerContext';
import { $selectAll } from 'lexical';
import { useEffect } from 'react';

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();

    useEffect(() =>
        editor.registerEditableListener((_isEditable) => {
            if (_isEditable) {
                editor.focus();
                editor.update(() => {
                    // editor.focus(); // also tried focussing here with no luck
                    $selectAll();
                });
            }
        }),
        [editor],
    );

    useEffect(() => {
        editor.setEditable(isEditable);
        /* I first tried to focus from here, but it didn’t work, so I tried the listener
        if (isEditable) {
            editor.focus();
            editor.update(() => {
                // editor.focus(); // also tried focussing here with no luck
                $selectAll();
            });
        }
        */
    }, [editor, isEditable]);

    return null;
}

i did wind up getting it to work, but it’s a classic “this seems wrong and fragile” solution, so i’m not delighted. i had to wrap the editor.focus() invocation in a setTimeout. is there a better way?

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();

    useEffect(() =>
        editor.registerEditableListener((_isEditable) => {
            if (_isEditable) {
                setTimeout(() => {
                    editor.focus();
                    editor.update(() => {
                        $selectAll();
                    });
                }, 0); // next tick
            }
        }),
        [editor],
    );

    useEffect(() => {
        editor.setEditable(isEditable);
    }, [editor, isEditable]);

    return null;
}

acusti avatar May 07 '24 20:05 acusti

I think your bug boils down to the fact that the editor is already editable before the listener is attached because it was marked editable during the first run of the effect in StrictMode, so the listener isn't triggered. An easier approach to get correct in StrictMode is demonstrated by the useLexicalEditable() hook and you could trigger an effect based on that without manually registering any listeners.

etrepum avatar May 07 '24 20:05 etrepum

EditableState.tsx

import { useLexicalComposerContext } from '@lexical/react/LexicalComposerContext';
import useLexicalEditable from '@lexical/react/useLexicalEditable';
import { $getSelection, $selectAll } from 'lexical';
import { useEffect } from 'react';

export default function EditablePlugin({ isEditable }: { isEditable: boolean }) {
    const [editor] = useLexicalComposerContext();
    const editableState = useLexicalEditable();
    useEffect(() => {
        editor.setEditable(isEditable);
    }, [editor, isEditable]);
    useEffect(() => {
        if (editableState) {
            // Note this is basically just editor.focus() but
            // with a different default selection. If you want
            // to select the beginning or end just use that.
            editor.update(() => {
                const selection = $getSelection();
                if (selection) {
                    // Force selection to be reconciled
                    selection.dirty = true;
                } else {
                    $selectAll();
                }
            });
        }
    }, [editor, editableState]);

    return null;
}

etrepum avatar May 07 '24 21:05 etrepum

@etrepum that works great, thanks for the followup!

I think your bug boils down to the fact that the editor is already editable before the listener is attached because it was marked editable during the first run of the effect in StrictMode, so the listener isn't triggered

i don’t think that’s the case, because i’m defaulting the editor to not editable, so the editor.focus() / $selectAll() code isn’t executing at all during initial render(s) and is only triggered after all listeners have been attached and the user clicks on the editor to start interacting with it. instead, it seems like the editable listener is being triggered before the editor has actually become editable (or the changes have been updated to / reconciled with the DOM). with the useEffect-based approach where editableState is in the dependency array (and with my setTimeout hack), on the other hand, the logic is triggered after the react render lifecycle has completed with the editable state change, so manipulating the selection succeeds. (just my not well-informed speculation, of course!)

either way, thanks again for all your help with this.

acusti avatar May 07 '24 21:05 acusti