Bug: Text sometimes not updated in Safari since 0.6.4 (regression from #3429)

[birtles]

Lexical version: 0.6.4

Given a playwright test such as the following:

test.describe('Suite', () => {
  test.only('Edits stuff', async ({
    page,
  }) => {
    await page.goto('/page/with/editor');
    await page.
      .locator('[data-rich-text-input-front] [data-lexical-editor]')
      .fill('Front');

    await pageOne
      .locator('[data-rich-text-input-front] [data-lexical-editor]')
      .fill('Front updated');

    await expect(
      pageOne.locator('[data-rich-text-input-front] [data-lexical-editor]')
    ).toHaveText('Front updated');
  });
});

I get the following test failure after updating from 0.6.3 to 0.6.4 in webkit only.

    Error: expect(received).toHaveText(expected)

    Expected string: "Front updated"
    Received string: "Front"

Having debugged the difference between the two versions I see the following:

Firstly, we call $shouldPreventDefaultAndInsertText from onBeforeInput.updateEditor with inputType 'insertText'.

In Chromium in 0.6.3 and 0.6.4 $shouldPreventDefaultAndInsertText will return true due to the following condition evaluating to true:

https://github.com/facebook/lexical/blob/a2f3b2fe157bcd20d69c1e85b9203d6c52d244ab/packages/lexical/src/LexicalEvents.ts#L216-L220

In Chromium, the domAnchorNode points to the wrapping <div> element and hence will not match the text node for the backingAnchorElement (which is the child <span> element). As a result we will call preventDefault() and dispatch a CONTROLLED_TEXT_INSERTION_COMMAND.

In Webkit, however, in both 0.6.3 and 0.6.4, the above condition will return false since domAnchorNode points to text node child of backingAnchorElement. Hence none of the conditions in $shouldPreventDefaultAndInsertText evaluate to true and we don't dispatch a CONTROLLED_TEXT_INSERTION_COMMAND.

Since we don't call preventDefault() we get a call to onInput.

In onInput we call $shouldPreventDefaultAndInsertText again but this time, when we have an 'input' event, domAnchorNode points to the the Text node on the <span> (containing "Front updated") back the backingAnchorElement points to an orphaned <span> whose Text node contains just "Front". Since these are different Text nodes, the backing anchor element check evaluates to true and $shouldPreventDefaultAndInsertText returns true.

From this point on the behavior differs between 0.6.3 and 0.6.4.

In 0.6.3, we unconditionally dispatch CONTROLLED_TEXT_INSERTION_COMMAND:

https://github.com/facebook/lexical/blob/1367743625e8ea79c08d881d4cecbe9195da2fcc/packages/lexical/src/LexicalEvents.ts#L616

However, in 0.6.4, as of #3429, we only dispatch a CONTROLLED_TEXT_INSERTION_COMMAND if the text in the DOM appears to differ from the result of splicing the event data with the anchor node's text:

https://github.com/facebook/lexical/blob/a2f3b2fe157bcd20d69c1e85b9203d6c52d244ab/packages/lexical/src/LexicalEvents.ts#L692-L706

In the case of this particular test, the combination of anchorNode.getTextContent() and data matches the text content of domSelection.anchorNode so we don't dispatch a CONTROLLED_TEXT_INSERTION_COMMAND.

(Specifically, anchorNode.getTextContent() is "Front", but the selection encompasses the whole string from 0 to 5 so we end up just considering event.data which is "Front updated".)

As a result, anchorNode is never updated to match the DOM. And presumably at some later point we clobber the DOM with the value in anchorNode.

If I force the condition to evaluate to true and hence dispatch a CONTROLLED_TEXT_INSERTION_COMMAND the test passes again.

In Firefox, beforeinput uses inputType of 'insertCompositionText' which is ignored by onBeforeInput so it also proceeds to onInput. However, it will fail the branch $shouldPreventDefaultAndInsertText in onInput (unlike Safari, backingAnchorElement appears to point to the live <span> element in Firefox) and proceed to update the selected text from the DOM:

https://github.com/facebook/lexical/blob/a2f3b2fe157bcd20d69c1e85b9203d6c52d244ab/packages/lexical/src/LexicalEvents.ts#L726-L727

[trueadm]

Thank you for the very detailed bug report.

What if add a IS_SAFARI || IS_IOS to

((isBeforeInput || !CAN_USE_BEFORE_INPUT) && 
   backingAnchorElement !== null && 
   !anchorNode.isComposing() && 
   domAnchorNode !== getDOMTextNode(backingAnchorElement))

Would that maybe resolve this? This logic is all quite tricky. It would be good to have a better heuristic here but I'm unsure what works well.

[birtles]

Yes, the following seems to fix my failing test at least:

 ((isBeforeInput || !CAN_USE_BEFORE_INPUT) && 
    backingAnchorElement !== null && 
    !anchorNode.isComposing() && 
-   domAnchorNode !== getDOMTextNode(backingAnchorElement))
+  (IS_SAFARI || IS_IOS || domAnchorNode !== getDOMTextNode(backingAnchorElement)))

I guess another option would be, in onInput, to detect when the anchor node's backing element is orphaned and dispatch a CONTROLLED_TEXT_INSERTION_COMMAND in that case?

[trueadm]

I guess another option would be, in onInput, to detect when the anchor node's backing element is orphaned and dispatch a CONTROLLED_TEXT_INSERTION_COMMAND in that case?

That's a good idea. If you could help with a PR, that would be epic :)

[birtles]

Sure, I'll have a look tomorrow.

[trueadm]

I extracted your test case into this PR https://github.com/facebook/lexical/pull/3470. Interestingly, I can't seem to repro the issue in Safari? Is it maybe down to differences in Playwright version?

[birtles]

I extracted your test case into this PR #3470. Interestingly, I can't seem to repro the issue in Safari? Is it maybe down to differences in Playwright version?

Thanks!

Hmm, could be. I see that lexical is on 1.22.1 / 1.23.0-next-alpha-trueadm-fork whereas I've been testing with 1.28.1.

Looks like Playwright 1.22 / 1.23 is using Webkit 15.4 where as 1.28.1 is using Webkit 16.4 which could explain the difference.

I tried updating my fork of lexical to debug the tests there but I'm having trouble with the dependencies for Webkit (chromium and firefox work fine). Various libraries are reported missing and npx playwright install-deps errors out due to other unavailable dependencies. I guess that version of playwright doesn't support Webkit on Ubuntu 22.04.

Any plan to bump the playwright version in the near future? Otherwise I guess I'll need to set up Ubuntu 20.04 to debug why it's not failing.

[trueadm]

Here's the change to the Webkit version (https://github.com/facebook/lexical/pull/3476)

However, going back to the original issue how did you plan on detecting when the anchor node's backing element is orphaned?

[trueadm]

Is there any way to recreate this issue without using Playwright?

[birtles]

Here's the change to the Webkit version (#3476)

Thanks! Unfortunately I still can't get Ubuntu 22.04 to like it so I'm torn between setting up Ubuntu 20.04 or trying to debug on a tiny underpowered Mac I have.

However, going back to the original issue how did you plan on detecting when the anchor node's backing element is orphaned?

isConnected appears to do the trick in this case.

Is there any way to recreate this issue without using Playwright?

I started trying to create a code sandbox for this at https://codesandbox.io/s/lexical-plain-text-example-forked-mls7m6 but ran in to trouble emulating Playwright's fill (source ref 1, 2). It works for a regular contenteditable node, but I guess execCommand is not a suitable substitute (as I believe we have code in lexical specifically to filter out execCommand input events).

That said, I went to work on a speculative fix and I realized my original analysis was wrong. I said that on the 'input' event $shouldPreventDefaultAndInsertText was returning true due to the anchor backing element condition but as of #3438 that condition is now only used for 'beforeinput' events.

It turns out it's this beast that is returning true:

https://github.com/facebook/lexical/blob/a49e682a35e423deb5598522a36e194d45cabe78/packages/lexical/src/LexicalEvents.ts#L198-L209

I'm find that a bit hard to read so I broke it out and annotated the result of each part:

const condition =
  (
    (
      !isBeforeInput && // true
      (
        !CAN_USE_BEFORE_INPUT || // false
        // 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 // true
      )
    ) ||
    textLength < 2 || // false
    doesContainGrapheme(text) // false
  ) &&
  anchor.offset !== focus.offset && // true
  !anchorNode.isComposing() // true

The first bit looks a bit suspicious to me. In #3429 we effectively changed it from:

    ((textLength < 2 || doesContainGrapheme(text)) &&
      anchor.offset !== focus.offset &&
      !anchorNode.isComposing()) ||

to

    (((!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()) ||

I think the intent of this change was that we would only apply this check to input events and we should ignore dangling input events. Does that sound right?

However, because we're ||-ing this together with the textLength / doesContainGrapheme check, any time those conditions are true (i.e. any time we have an input event shortly after a beforeinput event), this whole block will return true provided we don't have a collapsed selection and are not composing--which doesn't seem like the intent?

If I change that condition 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()

i.e.:

  !isBeforeInput &&
  (!CAN_USE_BEFORE_INPUT || lastBeforeInputInsertTextTimeStamp < timeStamp + 50) &&
  (textLength < 2 || doesContainGrapheme(text)) &&
  anchor.offset !== focus.offset &&
  !anchorNode.isComposing()

then we consistently return false for both calls to $shouldPreventDefaultAndInsertText (i.e. for both the beforeinput and input event) which is probably the intended result?

But the saga continues.

With that change, we will then call $updateSelectedTextFromDOM but it will fail to do anything (i.e. fail to call $updateTextNodeFromDOMContent) because the DOM selection is pointing at the text content of an orphaned span so $getNearestNodeFromDOMNode returns null.

How did this work on 0.6.3? In 0.6.3 we:

• didn't limit the check for backingAnchorElement to just beforeinput events (changed in #3438) so $shouldPreventDefaultAndInsertText returned true for our input event, and
• we unconditionally dispatched a CONTROLLED_TEXT_INSERTION_COMMAND when that happened (changed in #3429)

So I guess we want something like:

• Fix the check in $shouldPreventDefaultAndInsertText to correctly check for beforeinput events as described above?
• Make $shouldPreventDefaultAndInsertText also check for orphaned backing elements, even in input events?

[birtles]

Oh, and make sure we perform the controlled text insertion in onInput when we have an orphaned anchor backing element:

i.e. something like

diff --git a/packages/lexical/src/LexicalEvents.ts b/packages/lexical/src/LexicalEvents.ts
index 6fef043e..531fe1da 100644
--- a/packages/lexical/src/LexicalEvents.ts
+++ b/packages/lexical/src/LexicalEvents.ts
@@ -196,15 +196,14 @@ function $shouldPreventDefaultAndInsertText(
     // If we're working with a non-text node.
     !$isTextNode(anchorNode) ||
     // If we are replacing a range with a single character or grapheme, and not composing.
-    (((!isBeforeInput &&
+    (!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)) &&
+        lastBeforeInputInsertTextTimeStamp < timeStamp + 50) &&
+      (textLength < 2 || doesContainGrapheme(text)) &&
       anchor.offset !== focus.offset &&
       !anchorNode.isComposing()) ||
     // Any non standard text node.
@@ -218,6 +217,8 @@ function $shouldPreventDefaultAndInsertText(
       backingAnchorElement !== null &&
       !anchorNode.isComposing() &&
       domAnchorNode !== getDOMTextNode(backingAnchorElement)) ||
+    // If the DOM selection element is orphaned
+    (backingAnchorElement !== null && !backingAnchorElement.isConnected) ||
     // Check if we're changing from bold to italics, or some other format.
     anchorNode.getFormat() !== selection.format ||
     // One last set of heuristics to check against.
@@ -688,6 +689,9 @@ function onInput(event: InputEvent, editor: LexicalEditor): void {
       if (domSelection === null) {
         return;
       }
+      const backingAnchorElement = getActiveEditor().getElementByKey(
+        selection.anchor.key,
+      );
       const offset = anchor.offset;
       // If the content is the same as inserted, then don't dispatch an insertion.
       // Given onInput doesn't take the current selection (it uses the previous)
@@ -697,6 +701,7 @@ function onInput(event: InputEvent, editor: LexicalEditor): void {
         selection.isCollapsed() ||
         !$isTextNode(anchorNode) ||
         domSelection.anchorNode === null ||
+        (backingAnchorElement !== null && !backingAnchorElement.isConnected) ||
         anchorNode.getTextContent().slice(0, offset) +
           data +
           anchorNode.getTextContent().slice(offset + selection.focus.offset) !==

[birtles]

I tried debugging the test added in #3470 using Mac to see why it doesn't fail and it looks like the difference is that when I run the test using the lexical repo and its version of Playwright the following check fails:

https://github.com/facebook/lexical/blob/365c9472a80fc8dad6b655238facc357ccccfc1a/packages/lexical/src/LexicalEvents.ts#L700-L703

That is, the DOM still has the "Front" string in it while the incoming event.data has "Front updated". Since they're different, we dispatch the CONTROLLED_TEXT_INSERTION_COMMAND.

For me, using a later version of Playwright and Safari and in the context of a different app, the DOM is already updated to "Front updated" at the point when we process the input event.

I'm not sure if the difference comes about due to (a) Playwright version, (b) Safari version, (c) something in the app I am debugging. I think we can rule out (b) because even after applying #3476 the test still passes.

So it's either Playwright or my app. I've ripped out every plugin and custom style from my app so that the lexical setup is about as vanilla as it gets and the problem still reproduces so I start to suspect a change in Playwright.

In any case, I think the changes suggested above are probably reasonable so I might go ahead and turn them into a PR.

[birtles]

I had another dig into this today and here's what I worked out:

1. The test added in #3470 fails in Lexical 0.6.3.

2. That same test passes in Lexical 0.6.4 because of the (probably erroneous) change introduced in #3429 that makes us return true from $shouldPreventDefaultAndInsertText much more often than we probably intended.

3. The initial patch I put up in PR #3498 effectively "fixes" the change in (2) taking us back to where we were in 0.6.3.

4. The core issue is that when we call Playwright's fill() twice in a row we get one of two behaviors in webkit:

   • The DOM simply doesn't get updated, or at least not by the time the 'input' event arrives (as observed in Lexical's playwright tests)

   • The DOM gets updated, but by introducing a new <span> such that the lexical selection anchor points to a now orphaned <span> (as observed in the playwright tests in my app)

   We don't know what causes the difference between the two behaviors. It might be a difference in Playwright version (version 1.22/1.23 vs 1.28.1) or something else in my app (although went through and ripped out even more possibly related styles etc. and I still get the latter behavior).

5. In either of the cases in (4), we want to trigger a controlled text insertion since otherwise the change won't work.

Regarding how to proceed, the two options that come to mind are:

• Try to actually detect either situation by extending $shouldPreventDefaultAndInsertText by using similar logic to isInput so that, for an input event, we detect if the DOM has been updated or not.

• Simply make $shouldPreventDefaultAndInsertText return true for any input event where IS_SAFARI | IS_IOS is true.

The latter is simpler but I'll have a try at the former and see if that works.

[birtles]

Ok, I wrote a patch that fixes this particular issue but causes other tests to fail.

In particular when we fail the following condition:

https://github.com/facebook/lexical/blob/268977692c050a693c7db77837cfc69ff32d49b2/packages/lexical/src/LexicalEvents.ts#L692-L706

we often fail to update the lexical state.

In onInput,

• If $shouldPreventDefaultAndInsertText returns true,
  • If this check for a non-redundant change returns true
  • We dispatch CONTROLLED_TEXT_INSERTION_COMMAND and everything is ok
  • Else
  • Lexical state doesn't get updated
• Else
  • We run $updateSelectedTextFromDOM and everything is ok

@trueadm Do you recall what that condition is needed for? Should we be doing $updateSelectedTextFromDOM when it returns false? Doing so seems to fix the test failure I was seeing.

[trueadm]

I believe we have that in mainly for Firefox if memory serves me right. You also might have to consider legacy events, or not having native beforeinput, which we capture in our e2e tests. Does grammarly still work with that change too? Specifically fixing a grammarly change with selection being in a completely different block in the editor?

[birtles]

I believe we have that in mainly for Firefox if memory serves me right. You also might have to consider legacy events, or not having native beforeinput, which we capture in our e2e tests.

Ok, I just pushed another fix to the PR so I'll see what the e2e tests think of that.

Does grammarly still work with that change too? Specifically fixing a grammarly change with selection being in a completely different block in the editor?

It appears to work for me in Firefox. Should I be testing in Safari instead though?

[trueadm]

It appears to work for me in Firefox. Should I be testing in Safari instead though?

Grammarly needs to be tested in all browsers that support the extension.