[lexical] Bug Fix(keydown): trigger copy command for non range selection #6439
Conversation
mrdivyansh
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi @mrdivyansh! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
size-limit report 📦
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
similar to the test, im unable to manually reproduce the issue described in #6438 Screen.Recording.2024-07-24.at.6.02.02.PM.movfollowed the same steps listed in the issue are u using any special setup locally? |
|
@potatowagon, you missed the 3rd step. I can see a paragraph node (409) before the table node (410) in your demo. You need to ensure there is no other nodes except a table node. |
|
ok i can repro the issue after including the third step |
| @@ -1080,7 +1079,7 @@ function onKeyDown(event: KeyboardEvent, editor: LexicalEditor): void { | |||
| dispatchCommand(editor, REDO_COMMAND, undefined); | |||
| } else { | |||
| const prevSelection = editor._editorState._selection; | |||
| if ($isNodeSelection(prevSelection)) { | |||
| if (!$isRangeSelection(prevSelection)) { | |||
There was a problem hiding this comment.
have checked that this includes a solo table into the copy cmd, tho whats missing is that the table doesnt get deleted on cut (which is also happening in prod so not an issue introed in this PR)
Screen.Recording.2024-07-26.at.1.15.43.PM.mov
There was a problem hiding this comment.
hi @zurfyx! checking with u is this one line change safe?
| // Delete the last empty paragraph | ||
| await moveToEditorEnd(page); | ||
| await page.keyboard.press('Backspace'); | ||
|
|
There was a problem hiding this comment.
I am not sure why I can't reproduce the bug by automated interaction but manual interaction.
try adding an assert here to assert that the table isnt wrapped by 2 para nodes
There was a problem hiding this comment.
if the assert fails it could be need to add keypress delays before hitting backspace
There was a problem hiding this comment.
I observed a weird behaviour with the Chrome browser. The copy function works when I intermittently switch to another tab and then return to Lexical to trigger the copy. Perhaps this is the reason the test doesn't fail for Chromium browser.
Lexical.Playground.-.27.July.2024.mp4
So, I decided to switch to Firefox with Playwright. Bang! The test failed without the PR changes and passed with the changes 🎉.
Lexical.Playground.-.27.July.2024.mp4
There was a problem hiding this comment.
this test is failing alot on the collab e2e suite, could u please help to fix it? if not landing this change will cause the CI to be unstable
possible fixes include adding asserts and keypress delays at backspace
There was a problem hiding this comment.
I tried with assertion. They were passing. I will check the failing tests.
potatowagon
added
extended-tests
|
None of the tests fail on my local machine. Added delays and assertions. It might help. |
Description
The copy and cut command isn't triggered when the selection is
TableSelectiontype. Currently, the code checks forNodeSelection; however, it should have been supporting custom selection as well.Closes #6438
Test plan
I have added an e2e test but the test passes even without my changes. I am not sure why I can't reproduce the bug by automated interaction but manual interaction. I am looking for help on this.The added test works for Firefox.
Before
Lexical.Playground.-.22.July.2024.mp4
After
Lexical.Playground.-.22.July.2024.1.mp4