Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[lexical] Bug Fix: lines were being deleted with deleteLine #6719

Merged
merged 9 commits into from
Oct 16, 2024
Merged

[lexical] Bug Fix: lines were being deleted with deleteLine #6719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
12bec35
fix: deleteLine
taro-shono Oct 10, 2024
6d64208
fix: perttier
taro-shono Oct 15, 2024
a5cee0b
feat: test
taro-shono Oct 15, 2024
c73a172
Merge branch 'main' into fix-deleteline
taro-shono Oct 15, 2024
8e7ffb1
Merge branch 'main' into fix-deleteline
ivailop7 Oct 15, 2024
787b327
Update packages/lexical/src/LexicalSelection.ts
taro-shono Oct 15, 2024
c26567c
feat: test
taro-shono Oct 15, 2024
c27f281
fix: test
taro-shono Oct 15, 2024
2f33838
Merge branch 'main' into fix-deleteline
taro-shono Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 39 additions & 9 deletions packages/lexical-playground/__tests__/e2e/Selection.spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,20 +208,20 @@ test.describe.parallel('Selection', () => {
</p>
`,
];
const empty = html`
<p class="PlaygroundEditorTheme__paragraph"><br /></p>
`;

await deleteLine();
await assertHTML(page, lines.slice(0, 3).join(''));
await assertHTML(page, [lines[0], lines[1], lines[2], empty].join(''));
await page.keyboard.press('Backspace');
await deleteLine();
await assertHTML(page, lines.slice(0, 2).join(''));
await assertHTML(page, [lines[0], lines[1]].join(''));
await deleteLine();
await assertHTML(page, lines.slice(0, 1).join(''));
await assertHTML(page, [lines[0], empty].join(''));
await page.keyboard.press('Backspace');
await deleteLine();
await assertHTML(
page,
html`
<p class="PlaygroundEditorTheme__paragraph"><br /></p>
`,
);
await assertHTML(page, empty);
});

test('can delete line which ends with element with CMD+delete', async ({
Expand Down Expand Up @@ -251,6 +251,7 @@ test.describe.parallel('Selection', () => {
};

await deleteLine();
await page.keyboard.press('Backspace');
await assertHTML(
page,
html`
Expand All @@ -261,6 +262,7 @@ test.describe.parallel('Selection', () => {
</p>
`,
);
await page.keyboard.press('Backspace');
await deleteLine();
await assertHTML(
page,
Expand All @@ -270,6 +272,34 @@ test.describe.parallel('Selection', () => {
);
});

test('can delete line by collapse', async ({page, isPlainText}) => {
test.skip(isPlainText || !IS_MAC);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to skip this test on mac?

Suggested change
test.skip(isPlainText || !IS_MAC);
test.skip(isPlainText);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so too. But I don't know why this line:

lexical/packages/lexical-playground/__tests__/e2e/Selection.spec.mjs

Line 170 in c27f281

test.skip(isPlainText || !IS_MAC);

lexical/packages/lexical-playground/__tests__/e2e/Selection.spec.mjs

Line 231 in c27f281

test.skip(isPlainText || !IS_MAC);

The above test is the same as the deleteLine test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not useful there either! All of them pass without it for me locally on my Mac. We can address that separately though

await focusEditor(page);
await insertCollapsible(page);
await page.keyboard.type('text');
await page.keyboard.press('Enter');
await page.keyboard.press('ArrowUp');

const deleteLine = async () => {
await keyDownCtrlOrMeta(page);
await page.keyboard.press('Backspace');
await keyUpCtrlOrMeta(page);
};
await deleteLine();
await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">text</span>
</p>
<p class="PlaygroundEditorTheme__paragraph"><br /></p>
<p class="PlaygroundEditorTheme__paragraph"><br /></p>
`,
);
});

test('Can insert inline element within text and put selection after it', async ({
page,
isPlainText,
Expand Down
10 changes: 5 additions & 5 deletions packages/lexical/src/LexicalSelection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1856,11 +1856,11 @@ export class RangeSelection implements BaseSelection {

this.modify('extend', isBackward, 'lineboundary');

// If selection is extended to cover text edge then extend it one character more
// to delete its parent element. Otherwise text content will be deleted but empty
// parent node will remain
const endPoint = isBackward ? this.focus : this.anchor;
if (endPoint.offset === 0) {
// If the selection starts at the beginning of a text node (offset 0),
// 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.isCollapsed() && this.anchor.offset === 0) {
this.modify('extend', isBackward, 'character');
}

Expand Down
Loading