-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug: Text sometimes not updated in Safari since 0.6.4 (regression from #3429) #3460
Comments
Thank you for the very detailed bug report. What if add a
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. |
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 |
That's a good idea. If you could help with a PR, that would be epic :) |
Sure, I'll have a look tomorrow. |
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 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 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. |
Here's the change to the Webkit version (#3476) However, going back to the original issue how did you plan on detecting when the anchor node's backing element is orphaned? |
Is there any way to recreate this issue without using Playwright? |
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.
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 That said, I went to work on a speculative fix and I realized my original analysis was wrong. I said that on the It turns out it's this beast that is returning lexical/packages/lexical/src/LexicalEvents.ts Lines 198 to 209 in a49e682
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 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 But the saga continues. With that change, we will then call How did this work on 0.6.3? In 0.6.3 we:
So I guess we want something like:
|
Oh, and make sure we perform the controlled text insertion in 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) !== |
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: lexical/packages/lexical/src/LexicalEvents.ts Lines 700 to 703 in 365c947
That is, the DOM still has the "Front" string in it while the incoming 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. |
Fixes facebook#3460. Consists of three changes: 1. In facebook#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: facebook#3460 (comment) 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.
I had another dig into this today and here's what I worked out:
Regarding how to proceed, the two options that come to mind are:
The latter is simpler but I'll have a try at the former and see if that works. |
Ok, I wrote a patch that fixes this particular issue but causes other tests to fail. In particular when we fail the following condition: lexical/packages/lexical/src/LexicalEvents.ts Lines 692 to 706 in 2689776
we often fail to update the lexical state. In
@trueadm Do you recall what that condition is needed for? Should we be doing |
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? |
Ok, I just pushed another fix to the PR so I'll see what the e2e tests think of that.
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. |
Lexical version: 0.6.4
Given a playwright test such as the following:
I get the following test failure after updating from 0.6.3 to 0.6.4 in webkit only.
Having debugged the difference between the two versions I see the following:
Firstly, we call
$shouldPreventDefaultAndInsertText
fromonBeforeInput.updateEditor
with inputType'insertText'
.In Chromium in 0.6.3 and 0.6.4
$shouldPreventDefaultAndInsertText
will returntrue
due to the following condition evaluating totrue
:lexical/packages/lexical/src/LexicalEvents.ts
Lines 216 to 220 in a2f3b2f
In Chromium, the
domAnchorNode
points to the wrapping<div>
element and hence will not match the text node for thebackingAnchorElement
(which is the child<span>
element). As a result we will callpreventDefault()
and dispatch aCONTROLLED_TEXT_INSERTION_COMMAND
.In Webkit, however, in both 0.6.3 and 0.6.4, the above condition will return
false
sincedomAnchorNode
points to text node child ofbackingAnchorElement
. Hence none of the conditions in$shouldPreventDefaultAndInsertText
evaluate totrue
and we don't dispatch aCONTROLLED_TEXT_INSERTION_COMMAND
.Since we don't call
preventDefault()
we get a call toonInput
.In
onInput
we call$shouldPreventDefaultAndInsertText
again but this time, when we have an'input'
event,domAnchorNode
points to the theText
node on the<span>
(containing "Front updated") back thebackingAnchorElement
points to an orphaned<span>
whoseText
node contains just "Front". Since these are differentText
nodes, the backing anchor element check evaluates totrue
and$shouldPreventDefaultAndInsertText
returnstrue
.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
:lexical/packages/lexical/src/LexicalEvents.ts
Line 616 in 1367743
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 eventdata
with the anchor node's text:lexical/packages/lexical/src/LexicalEvents.ts
Lines 692 to 706 in a2f3b2f
In the case of this particular test, the combination of
anchorNode.getTextContent()
anddata
matches the text content ofdomSelection.anchorNode
so we don't dispatch aCONTROLLED_TEXT_INSERTION_COMMAND
.(Specifically,
anchorNode.getTextContent()
is"Front"
, but the selection encompasses the whole string from 0 to 5 so we end up just consideringevent.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 inanchorNode
.If I force the condition to evaluate to true and hence dispatch a
CONTROLLED_TEXT_INSERTION_COMMAND
the test passes again.In Firefox,
beforeinput
usesinputType
of'insertCompositionText'
which is ignored byonBeforeInput
so it also proceeds toonInput
. However, it will fail the branch$shouldPreventDefaultAndInsertText
inonInput
(unlike Safari,backingAnchorElement
appears to point to the live<span>
element in Firefox) and proceed to update the selected text from the DOM:lexical/packages/lexical/src/LexicalEvents.ts
Lines 726 to 727 in a2f3b2f
The text was updated successfully, but these errors were encountered: