facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

Bug: Unable to backspace delete decorator nodes containing text on android chrome #1965

Closed StormVanDerPol closed 1 year ago

StormVanDerPol commented 2 years ago

When attempting to backspace delete a decorator node that contains DOM text, the keyboard is instead dismissed, and the decorator node stays in the editor. Happens on various Android keyboard apps like GBoard and Microsoft SwiftKey.

This video should demonstrate it: https://files.catbox.moe/3v1gdi.mp4 (Sorry the filesize limit on github wouldn't allow me to upload it directly)

Lexical version: 0.3.8 Android version: 12 Android Chrome version: 100 Microsoft SwiftKey Keyboard version: 8.10.12.4 Gboard version: 11

Steps To Reproduce

  1. go to https://playground.lexical.dev/ using e.g. latest android chrome, while using GBoard or Microsoft SwiftKey
  2. insert a "equation" node (content does not matter)
  3. Move selection to the end of the editor.
  4. Hit backspace

EDIT: Maybe it's best to also check variants of the reproduction steps above like:

  1. insert some random text
  2. insert an "equation" node
  3. Move the selection to the end of the editor
  4. Hit backspace

Since I noticed that 83dac84 from #1968 only solves the issue when using the first set of steps. My bad.

Link to code example: https://playground.lexical.dev/

The current behavior

Hitting backspace does not delete the decorator node, instead, the keyboard is dismissed.

The expected behavior

Hitting backspace should delete the decorator node, and the keyboard should not be dismissed.

EDIT 2: this was previously fixed but still happens, also right now firefox android behaves differently so I edited the title.

As for the behaviour on firefox: android tries to compose in the inner text when holding backspace, deleting some text prior to the decorator node. Placing the caret right in front of the decorator node and hitting backspace works as intended.

StormVanDerPol commented 1 year ago

@trueadm I think this should be re-opened as something caused a regression and this is happening again. (I can't seem to re-open it myself)

trueadm commented 1 year ago

Is this still happening?

StormVanDerPol commented 1 year ago

Yeah, can still repro it using the "katex" nodes in the playground.

trueadm commented 1 year ago

I don't have an Android device on my right now. Can you help me debug this if possible?

If you add a debugger around these lines:

https://github.com/facebook/lexical/blob/main/packages/lexical/src/LexicalEvents.ts#L389

And if you press backspace on Android, can you see what happens after using the web inspector maybe?

No problem if you can't, it would just really help :)

StormVanDerPol commented 1 year ago

Sure! but what do you mean with adding a debugger? I can log stuff if that's what you mean. I'd have to learn to use a debugger since I haven't done that before. Screenshot from 2022-11-24 16-47-14

trueadm commented 1 year ago

If you typer the word debugger into the code it will pause Chrome at that line during execution :)

StormVanDerPol commented 1 year ago

woah. That's awesome.

image

Is there anything specific you want me to look for?

StormVanDerPol commented 1 year ago

is $setSelection(prevSelection.clone()) at line 404 supposed to mutate the selection object defined at 386? I was messing around some more and noticed that the check at line 402 (if ($isRangeSelection(selection))) resolves to false in this case because the selection is still null, even though prevSelection seems like a normal RangeSelection.

If I artificially make that check at 402 resolve to true, we eventually reach 441, where lexical fires:

event.preventDefault();
dispatchCommand(editor, DELETE_CHARACTER_COMMAND, true);

After which it deletes the katex node as expected. Doesn't seem to fix the case where you may have a Katex node then some text, and you hold down backspace. That seems to dismiss the keyboard the moment you reach the decorator node. Seems like event.inputType is insertCompositionText in that case.

trueadm commented 1 year ago

Excellent work digging into that! :) I'll put up a PR.

StormVanDerPol commented 1 year ago

@trueadm I edited my comment a bit, idk if you still caught that. There seem to be two seperate cases where this bug happens.

Anyway I hope it helps in some capacity!

trueadm commented 1 year ago

Does this PR help any? https://github.com/facebook/lexical/pull/3420

This logic is quite delicate though. I'm hesitant to cause https://github.com/facebook/lexical/issues/1973 or https://github.com/facebook/lexical/issues/2937 again.

StormVanDerPol commented 1 year ago

It does help, but you're right about it causing a regression as the problem described in #1973 happens again.

trueadm commented 1 year ago

I updated the PR to adjust for decorators only. Does that still cause the issue in #1973?

StormVanDerPol commented 1 year ago

I checked it out on two different android phones, the problem seems to still be present

trueadm commented 1 year ago

Sigh. Not sure how to avoid that problem. Working with Android and widgets that are marked as contenteditable="false" is such a hassle. Let me know if you discover anything too! I'll dig into this more next week otherwise.

StormVanDerPol commented 1 year ago

Yep! I can imagine android and IMEs are a pain when it comes to developing an RTE for the web. Lexical already works pretty great in that regard imo, I used to use slate but it was causing too many issues for users.

I'll take another look soon-ish too.

thegreatercurve commented 1 year ago

So, this issue only affects EquationNodes, unlike the playground inline ImageNodes which can be deleted. After much experimentation, I found that if I add user-select: none; to the node and change some of the logic for managing focus stealing, it should now be deletable like the ImageNode

alexgleason commented 7 months ago

I'm experiencing this exact issue, but user-select: none doesn't fix it if the node contains text. It does fix it if the node is an image.

benlammers commented 1 month ago

Still seeing this behaviour on Android, seems to be related to the GBoard as reported here https://github.com/facebook/lexical/issues/5042#issuecomment-1868044087, I can reproduce the issue using a Samsung device with Gboard and can confirm it works as expected with the default keyboard.

Is this something that might be addressed? Appreciate all the work and love the library!

https://github.com/facebook/lexical/assets/41494387/2e36fd84-7995-4ef7-8719-7c4ebdab936a

Facundo1609 commented 1 month ago

Still seeing this behaviour on Android, seems to be related to the GBoard as reported here #5042 (comment), I can reproduce the issue using a Samsung device with Gboard and can confirm it works as expected with the default keyboard.

Is this something that might be addressed? Appreciate all the work and love the library!

lexical-playground-google-keyboard.mp4

Hey! I have the same problem but when I trying to delete an image. Do you have the same problem?