Plain/RichTextPlugin to handle initialEditorState + editor.isReadOnly #1358
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/fbopensource/lexical/Ei8Tcoczik1aX7Rwj3yxqZ4SEDmf |
facebook-github-bot
added
the
CLA Signed
| @@ -98,6 +100,7 @@ export default function Editor(): React$Node { | |||
| <RichTextPlugin | |||
| contentEditable={<ContentEditable />} | |||
| placeholder={placeholder} | |||
| initialEditorState={isCollab ? emptyFn : null} | |||
There was a problem hiding this comment.
Why an empty function? Surely null should mean "do not initialize any editor state"?
There was a problem hiding this comment.
Also what if it's not initialEditorState? But instead, it can take a given editorState and update if it changes.
zurfyx
changed the title
| break; | ||
| } | ||
| case 'function': { | ||
| editor.update(initialEditorState); |
| @@ -544,15 +539,16 @@ export function addRootElementEvents( | |||
|
|
|||
| for (let i = 0; i < rootElementEvents.length; i++) { | |||
| const [eventName, onEvent] = rootElementEvents[i]; | |||
| const isReadOnly = editor.getReadOnly(); | |||
There was a problem hiding this comment.
Shouldn't this be inside the event listener itself? Otherwise it will be stale.
| switch (typeof initialEditorState) { | ||
| case 'string': { | ||
| const parsedEditorState = editor.parseEditorState(initialEditorState); | ||
| editor.setEditorState(parsedEditorState, {tag: 'history-merge'}); |
There was a problem hiding this comment.
I mean you can have const options = {tag: 'history-merge'} at the top of the module, and pass it in. Saves bytes and is faster.
| @@ -74,6 +74,7 @@ export default function Editor(): React$Node { | |||
| !isRichText ? 'plain-text' : '' | |||
| }`}> | |||
| <AutoFocusPlugin /> | |||
| <LexicalClearEditorPlugin /> | |||
There was a problem hiding this comment.
For future consideration: I'm wondering if this really needs to be a separate plugin. If so, can it be returned from the RichText or PlainText plugin? We allow composing plugins like that, right?
There was a problem hiding this comment.
@acywatson - Good point, this has always been a hot topic. It used to live in PlainText/Plugin but does it really make to have it there? At the end of the day this is a custom command that we happen to use in the playground and some WWW applications but you don't necessarily have to support clear. For example, you could just destroy and reinstantiate the editor.
At the same time, the reason why we merged BootstrapPlugin and RichTextPlugin together was mostly to solve initialization race conditions. Clearing the editor now sounds like an addon that doesn't conflict with the lifecycle.
Can we bring this up in our next sync? It might be worth a quick debate but personally I wouldn't like to clutter RichTextPlugin again.
zurfyx
changed the title
Same problem as #1335, different approach
Problem:
Potential race condition between initializing the content, event handlers (through commands) and the user typing on the editable, currently handled by BootstrapPlugin, Rich/PlainTextPlugin and LexicalContentEditable
Solution:
Have the Rich/PlainTextPlugin handle the content editable creation and events. Additionally, add a
_isReadOnlyproperty to be able to orchestrate React's lifecycle with thecontenteditable. (readonly also serves its most obvious purpose, be able to mark the editor as readonly)How does this work?
React SSR does not load
useLayoutEffectnoruseEffect. Technically,useLayoutEffectdoes run but we do replace it foruseEffectinuseLayoutEffect.js. (We are particularly interested inuseLayoutEffectbecause of its synchronous behavior before render)At the same, the editor is unusable before hydration so we have to make sure that the editor is
contenteditable="false"before hydration.So we can leverage
useLayoutEffectto perform the swap on the client-side. LexicalComposer will always set readonly to true (meaning contenteditable = false) after editor creation and that's the value that LexicalContentEditable will see. LexicalComposer will then have auseLayoutEffectthat will swap this readonly value to false.What if the user lazy loads Rich/PlainTextPlugin?
That's no problem! RichTextPlugin is now the responsible for both the event handling through commands and the rendering of the LexicalContentEditable, so even if readonly is false the user won't be able to interact without a
<div contenteditableon the DOM.For the same reason, we can always ensure that the Rich/PlainTextPlugin event handling through commands will always happen first before rendering.
Kudos to @trueadm and @acywatson for the proposal and @trueadm for helping me understand how this all connects together through React