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

[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

Merged
merged 5 commits into from
Nov 5, 2024
Merged

[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d062f6
Add test for collaboration undo bug
mike-atticus Sep 26, 2024
4fbe7fe
Fix: clean up dangling text in YJS
james-atticus Oct 29, 2024
a40b5b9
remove unused comments
james-atticus Nov 4, 2024
1d43245
Merge branch 'main' into collaboration-undo-bug-test
james-atticus Nov 5, 2024
8bc45e6
Merge branch 'main' into collaboration-undo-bug-test
etrepum Nov 5, 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
159 changes: 159 additions & 0 deletions packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,163 @@ test.describe('Collaboration', () => {
focusPath: [1, 1, 0],
});
});

test('Remove dangling text from YJS when there is no preceding text node', async ({
isRichText,
page,
isCollab,
browserName,
}) => {
// test.skip(!isCollab || IS_MAC);
test.skip(!isCollab);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto below

Suggested change
// test.skip(!isCollab || IS_MAC);
test.skip(!isCollab);
test.skip(!isCollab || 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.

Is there a reason this should be skipped on Mac? I am not seeing any platform specific code here, I think what this will do is make it harder for contributors to test locally on a Mac because some of the tests just won't run.

Copy link
Contributor

Choose a reason for hiding this comment

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

No particular reason, just pattern matching existing tests. Works fine on Mac so happy to leave as is.


// Left collaborator types two paragraphs of text
await focusEditor(page);
await page.keyboard.type('Line 1');
await page.keyboard.press('Enter');
await sleep(1050); // default merge interval is 1000, add 50ms as overhead due to CI latency.
await page.keyboard.type('This is a test. ');

// Right collaborator types at the end of paragraph 2
await sleep(1050);
await page
.frameLocator('iframe[name="right"]')
.locator('[data-lexical-editor="true"]')
.focus();
await page.keyboard.press('ArrowDown'); // Move caret to end of paragraph 2
await page.keyboard.press('ArrowDown');
await page.keyboard.type('Word');

await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">Line 1</span>
</p>
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">This is a test. Word</span>
</p>
`,
);

// Left collaborator undoes their text in the second paragraph.
await sleep(50);
await page.frameLocator('iframe[name="left"]').getByLabel('Undo').click();

// The undo also removed the text node from YJS.
// Check that the dangling text from right user was also removed.
await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">Line 1</span>
</p>
<p class="PlaygroundEditorTheme__paragraph"><br /></p>
`,
);
Comment on lines +281 to +291
Copy link
Contributor

@james-atticus james-atticus Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fails on main with:

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 5

      <p
        class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
        dir="ltr">
        <span data-lexical-text="true">Line 1</span>
      </p>
    - <p class="PlaygroundEditorTheme__paragraph"><br /></p>
    + <p
    +   class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
    +   dir="ltr">
    +   <span data-lexical-text="true">ord</span>
    + </p>

The text is ord because neither of the two if checks pass, so Lexical ends up in node._text = spliceString(node._text, offset, delCount, '');. delCount is the length of the string from the left user, plus 1 for the deleted YMap, resulting in the W also being removed.


// Left collaborator refreshes their page
await page.evaluate(() => {
document
.querySelector('iframe[name="left"]')
.contentDocument.location.reload();
});

// Page content should be the same as before the refresh
await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">Line 1</span>
</p>
<p class="PlaygroundEditorTheme__paragraph"><br /></p>
`,
);
Comment on lines +301 to +311
Copy link
Contributor

Choose a reason for hiding this comment

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

If you comment out the assert above, this passes on main because of this handling. The left editor's change is synced over to the right editor.

});

test('Merge dangling text into preceding text node', async ({
isRichText,
page,
isCollab,
browserName,
}) => {
// test.skip(!isCollab || IS_MAC);
test.skip(!isCollab);

// Left collaborator types two pieces of text in the same paragraph, but with different styling.
await focusEditor(page);
await page.keyboard.type('normal');
await sleep(1050);
await toggleBold(page);
await page.keyboard.type('bold');

// Right collaborator types at the end of the paragraph.
await sleep(50);
await page
.frameLocator('iframe[name="right"]')
.locator('[data-lexical-editor="true"]')
.focus();
await page.keyboard.press('ArrowDown'); // Move caret to end of paragraph
await page.keyboard.type('BOLD');

await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">normal</span>
<strong
class="PlaygroundEditorTheme__textBold"
data-lexical-text="true">
boldBOLD
</strong>
</p>
`,
);

// Left collaborator undoes their bold text.
await sleep(50);
await page.frameLocator('iframe[name="left"]').getByLabel('Undo').click();

// The undo also removed bold the text node from YJS.
// Check that the dangling text from right user was merged into the preceding text node.
await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">normalBOLD</span>
</p>
`,
);
Comment on lines +360 to +369
Copy link
Contributor

@james-atticus james-atticus Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fails on main with:

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 4

      <p
        class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
        dir="ltr">
    -   <span data-lexical-text="true">normalBOLD</span>
    +   <span data-lexical-text="true">normal</span>
    +   <strong class="PlaygroundEditorTheme__textBold" data-lexical-text="true">
    +     OLD
    +   </strong>

The B character is dropped for the same reason as above.


// Left collaborator refreshes their page
await page.evaluate(() => {
document
.querySelector('iframe[name="left"]')
.contentDocument.location.reload();
});

// Page content should be the same as before the refresh
await assertHTML(
page,
html`
<p
class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
dir="ltr">
<span data-lexical-text="true">normalBOLD</span>
</p>
`,
);
Comment on lines +379 to +388
Copy link
Contributor

Choose a reason for hiding this comment

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

On main, this assert passes for the left editor (because the refresh got it back in sync) but not the right editor (which will only get back in sync when it refreshes).

});
});
32 changes: 18 additions & 14 deletions packages/lexical-yjs/src/CollabElementNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,21 +157,25 @@ export class CollabElementNode {
nodeIndex !== 0 ? children[nodeIndex - 1] : null;
const nodeSize = node.getSize();

if (
offset === 0 &&
delCount === 1 &&
nodeIndex > 0 &&
prevCollabNode instanceof CollabTextNode &&
length === nodeSize &&
// If the node has no keys, it's been deleted
Array.from(node._map.keys()).length === 0
) {
// Merge the text node with previous.
prevCollabNode._text += node._text;
children.splice(nodeIndex, 1);
} else if (offset === 0 && delCount === nodeSize) {
// The entire thing needs removing
if (offset === 0 && length === nodeSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our observation is that length < nodeSize when you're just deleting some text from the node, and length === nodeSize if the YMap has been deleted. This matches up with the old code's first if branch (which also had Array.from(node._map.keys()).length === 0 but we couldn't see why that was necessary). It also aligns with the second branch delCount === nodeSize, where delCount = min(length, deletionSize).

// Text node has been deleted.
children.splice(nodeIndex, 1);
// If this was caused by an undo from YJS, there could be dangling text.
const danglingText = spliceString(
node._text,
offset,
delCount - 1,
'',
);
if (danglingText.length > 0) {
if (prevCollabNode instanceof CollabTextNode) {
// Merge the text node with previous.
prevCollabNode._text += danglingText;
} else {
// No previous text node to merge into, just delete the text.
this._xmlText.delete(offset, danglingText.length);
}
}
} else {
node._text = spliceString(node._text, offset, delCount, '');
}
Expand Down
Loading