Fix insertion when the existing anchor node is orphaned

[birtles]

Fixes #3460.

Consists of three changes:

1. In #3429 we tweaked $shouldPreventDefaultAndInsertText so that the check for replacing a range with a single character or grapheme is only applied for input events (not beforeinput events) unless we determine that it is a dangling input event but we likely meant to && that condition with the remaining conditions (not || it).

   See analysis here: https://github.com/facebook/lexical/issues/3460#issuecomment-1334744872

   This patch effectively changes the || to and && (and drops a few no-longer-needed braces in the process).

2. We have observed in some circumstances (thought to be when using a newer version of Playwright) that Webkit can end up in a state where, during the input event, the DOM node for our lexical selection anchor has been orphaned.

   The second change in this patch makes $shouldPreventDefaultAndInsertText detect this case and return true (i.e. cause us to dispatch a controlled text insertion in this case).

3. Finally, in order to ensure that we do, in fact, dispatch a controlled text insertion in the case described in (2), this patch also alters the condition inside onInput to ensure we perform the insertion when the DOM node for our lexical anchor is orphaned.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Dec 7, 2022 at 9:39AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Dec 7, 2022 at 9:39AM (UTC)

[birtles]

Wow, that's ironic. The code that was meant to fix #3460 made the (previously passing) test for it fail.

[birtles]

This is so detailed and well executed. Thank you for digging into this. Did you confirm this works with browser extensions still too? I’ll check Grammarly later and the macOS keyboard text replacement tool and merge if I don’t find any issues

Thanks! Unfortunately it looks like I have more work to do just getting the tests to pass first. After that I'm happy to check Grammarly. Are there any specific STR to try?

[trueadm]

This is so detailed and well executed. Thank you for digging into this. Did you confirm this works with browser extensions still too? I’ll check Grammarly later and the macOS keyboard text replacement tool and merge if I don’t find any issues

Thanks! Unfortunately it looks like I have more work to do just getting the tests to pass first. After that I'm happy to check Grammarly. Are there any specific STR to try?

Just some manual tests from things that I’ve found problematic - mainly browser extensions (Microsoft editor and grammarly), the onscreen keyboard text replacement thing on macOS, system emoji inserter on Mac and windows and system spellcheck

[birtles]

Just some manual tests from things that I’ve found problematic - mainly browser extensions (Microsoft editor and grammarly), the onscreen keyboard text replacement thing on macOS, system emoji inserter on Mac and windows and system spellcheck

Got it. Thanks! Unfortunately it's the end of my work day here so I'll have to pick it up tomorrow.

[trueadm]

Seems to have an issue with graphemes now. I'm not sure making $shouldPreventDefaultAndInsertText more complex is the right angle to tackle this, as it affects so many different things and is extremely fragile because of that.

If you can create an e2e test that emulates the behavior of the latest Playwright then I can also dig into this with you – maybe by manually emulating the event sequence (beforeinput, DOM mutations, input) manually in the test. I can take another stab at trying to update Playwright but that's proving to be really difficult too.

[trueadm]

Another plot twist – updating to the latest Playwright still doesn't cause that test to fail. It must be something else?

[birtles]

Seems to have an issue with graphemes now. I'm not sure making $shouldPreventDefaultAndInsertText more complex is the right angle to tackle this, as it affects so many different things and is extremely fragile because of that.

Yes, I agree.

I think this PR & issue got a bit derailed when I discovered the odd-looking condition introduced in #3429.

That is, we had the following:

    (((!isBeforeInput &&
      (!CAN_USE_BEFORE_INPUT ||
        // We check to see if there has been
        // a recent beforeinput event for "textInput". If there has been one in the last
        // 50ms then we proceed as normal. However, if there is not, then this is likely
        // a dangling `input` event caused by execCommand('insertText').
        lastBeforeInputInsertTextTimeStamp < timeStamp + 50)) ||
      textLength < 2 ||
      doesContainGrapheme(text)) &&
      anchor.offset !== focus.offset &&
      !anchorNode.isComposing()) ||

which I believe was supposed to be:

    (!isBeforeInput &&
      (!CAN_USE_BEFORE_INPUT ||
        // We check to see if there has been
        // a recent beforeinput event for "textInput". If there has been one in the last
        // 50ms then we proceed as normal. However, if there is not, then this is likely
        // a dangling `input` event caused by execCommand('insertText').
        lastBeforeInputInsertTextTimeStamp < timeStamp + 50) &&
      (textLength < 2 || doesContainGrapheme(text)) &&
      anchor.offset !== focus.offset &&
      !anchorNode.isComposing()) ||

(Since otherwise $shouldPreventDefaultAndInsertText will return true for any input event preceded by a beforeinput event with a non-collapsed selection and where we're not composing.)

But when I fix that, the test added in #3470 (the "Front updated" one) fails.

So, if my understanding is correct and the code introduced in #3429 is buggy, then I think the first step would be getting the newly added E2E test to pass with the above-mentioned code fixed--which might mean revisiting how to fix #3428 / #3429.

If you can create an e2e test that emulates the behavior of the latest Playwright then I can also dig into this with you – maybe by manually emulating the event sequence (beforeinput, DOM mutations, input) manually in the test. I can take another stab at trying to update Playwright but that's proving to be really difficult too.

Sure! I can have a go at that but it's probably more important to revisit the fix for #3428 / #3429 first.

[birtles]

Another plot twist – updating to the latest Playwright still doesn't cause that test to fail. It must be something else?

That's interesting. I've simplified my app down to the following and it still reproduces for me:

<LexicalComposer initialConfig={initialConfig}>
  <RichTextPlugin
    contentEditable={<ContentEditable />}
    ErrorBoundary={ErrorBoundary}
    placeholder={<div>Yer</div>}
  />
</LexicalComposer>

So my guess is either:

• There's something in the lexical playground setup that is triggering a different behavior
• There's another event handler somewhere in my app that is triggering the different behavior

Webkit tends to take different code paths based on the presence of even a single no-op event handler so it might be hard to track down what is tickling the different behavior here.

[trueadm]

Another plot twist – updating to the latest Playwright still doesn't cause that test to fail. It must be something else?

That's interesting. I've simplified my app down to the following and it still reproduces for me:

<LexicalComposer initialConfig={initialConfig}>
  <RichTextPlugin
    contentEditable={<ContentEditable />}
    ErrorBoundary={ErrorBoundary}
    placeholder={<div>Yer</div>}
  />
</LexicalComposer>

So my guess is either:

• There's something in the lexical playground setup that is triggering a different behavior
• There's another event handler somewhere in my app that is triggering the different behavior

Webkit tends to take different code paths based on the presence of even a single no-op event handler so it might be hard to track down what is tickling the different behavior here.

Could it be about focus/selection? The playground auto-focuses the content editable via the <AutoFocusPlugin />. I wonder if that's the big difference?

[trueadm]

I think this PR & issue got a bit derailed when I discovered the odd-looking condition introduced in https://github.com/facebook/lexical/pull/3429.

I agree, that does look wrong. Let me try and fix that as per your suggestion and make things work. I think this might be a separate problem from what you're seeing though, which might be related to the fact your environment might not be behaving the same as the playground – specifically around focus or selection.

[birtles]

Could it be about focus/selection? The playground auto-focuses the content editable via the <AutoFocusPlugin />. I wonder if that's the big difference?

I just tried throwing the <AutoFocusPlugin /> in but it didn't seem to make a difference.

I also tried ripping things out of playground on main to see if I can get that test to fail there but no luck.

[trueadm]

@birtles Are you using React 18?

[birtles]

@birtles Are you using React 18?

Oh I'm really sorry! That's it! I'm using Preact. I didn't even think about that being a factor. Sorry!

[trueadm]

Oh I'm really sorry! That's it! I'm using Preact. I didn't even think about that being a factor. Sorry!

Should we close this PR and issue then?

[birtles]

Should we close this PR and issue then?

I think we can close this PR but probably leave the issue open for now.

I'd like to look at the issue after we have a proper fix in place for #3428 / #3429.

I think it's still worth fixing because:

• We don't know for sure if Preact is to blame
• Even if it is, Lexical is framework agnostic and its conceivable that another framework or even a future version of React or Playwright could generate the same situation
• This used to work before 0.6.3 so it should be possible to get it to work again

With a proper fix for #3428 / #3429 maybe this case will just work, or maybe it will be a one-liner to fix. In any case most of this PR won't be needed.

[trueadm]

I'd like to look at the issue after we have a proper fix in place for https://github.com/facebook/lexical/issues/3428 / https://github.com/facebook/lexical/pull/3429.

Go for it.

Even if it is, Lexical is framework agnostic and its conceivable that another framework or even a future version of React or Playwright could generate the same situation

Whilst Lexical is framework agnostic, you’re also using React based plugins. Preact isn’t a 1:1 swap in replacement for React 18, as much as they may market it as such. This has been a common pattern even before React 18 too (I used to work on the React core team).

At the same time, it would be good to maybe setup a code sandbox with Preact so we can further dig into this :) unless this might be related to running in sync mode. We run React 18 in concurrent mode, which is how you're meant to use React 18, but maybe that could be fixing the issue too.

[birtles]

At the same time, it would be good to maybe setup a code sandbox with Preact so we can further dig into this :) unless this might be related to running in sync mode. We run React 18 in concurrent mode, which is how you're meant to use React 18, but maybe that could be fixing the issue too.

Yes, that would be good. Unfortunately I need to spend a bit of time first debugging why certain cases of deletion are broken for us after updating to 0.7.0 so it might be a while before I can get back to this. (I suspect either 6b7d82842adac1832209b58cac476faeb4c48752 or d30c88202e8d68e7ab79c9d2bf45366de0cea794 but they're big changesets so it's going to take a while.)