[lexical] Bug Fix: lines were being deleted with deleteLine
#6719
Conversation
taro-shono
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 @taro-shono! 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 cla@meta.com. Thanks! |
size-limit report 📦
|
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
It would be great if this change had a test, and it is necessary for all PRs to pass the formatting and type checks. This particular formatting issue (extra indent on all changed lines) can be fixed with npm run prettier:fix or by allowing husky to get set up properly which normally happens during npm install.
I think the logic might also not be correct, it seems to me that this extend behavior only happens in other editors (I tested VSCode and Chrome's textarea on github) when the selection is collapsed at offset 0.
@etrepum What does that mean? Please explain in more detail. |
|
A collapsed selection is where the focus and anchor are the same node and offset. It displays as a cursor, no text is highlighted. |
| // extend the selection by one character in the specified direction. | ||
| // This ensures that the parent element is deleted along with its content. | ||
| // Otherwise, only the text content will be deleted, leaving an empty parent node. | ||
| if (this.anchor.offset === 0) { |
There was a problem hiding this comment.
By collapsed I mean that this check should be more like this:
| if (this.anchor.offset === 0) { | |
| if (this.isCollapsed() && this.anchor.offset === 0) { |
There was a problem hiding this comment.
@etrepum isCollapsed check had already been made.
lexical/packages/lexical/src/LexicalSelection.ts
Line 1848 in 787b327
There was a problem hiding this comment.
You're right that a check was already made but the selection is mutated after the check is made with this.modify('extend', isBackward, 'lineboundary'); and an extend modification (if it does anything) will only change either the anchor or focus thus changing the selection so it's no longer collapsed
There was a problem hiding this comment.
I see. I've also added a test for the logic of collapsed: c26567c
Co-authored-by: Bob Ippolito <bob@redivi.com>
| @@ -270,6 +272,34 @@ test.describe.parallel('Selection', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| test('can delete line by collapse', async ({page, isPlainText}) => { | |||
| test.skip(isPlainText || !IS_MAC); | |||
There was a problem hiding this comment.
I don't think we need to skip this test on mac?
| test.skip(isPlainText || !IS_MAC); | |
| test.skip(isPlainText); |
There was a problem hiding this comment.
I think so too. But I don't know why this line:
The above test is the same as the deleteLine test.
There was a problem hiding this comment.
They're not useful there either! All of them pass without it for me locally on my Mac. We can address that separately though
Description
When I pressed ⌘ + Delete, the line was deleted. This is different from the behavior of other editors (such as textarea), so some users may find it to be a bug. Please see the video for more information about the behavior.
Before
Screen.Shot.2024-10-10.at.16.05.54.mp4
After
Screen.Shot.2024-10-10.at.16.05.25.mp4