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.

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

Bug: DELETE_CHARACTER_COMMAND pushes meaningless history #5350

Open
2wheeh opened this issue Dec 7, 2023 · 1 comment · May be fixed by #7222
Open

Bug: DELETE_CHARACTER_COMMAND pushes meaningless history #5350

2wheeh opened this issue Dec 7, 2023 · 1 comment · May be fixed by #7222

Comments

@2wheeh
Copy link
Contributor

2wheeh commented Dec 7, 2023

Lexical version: latest

Steps To Reproduce

  1. press DELETE or BACKSPACE on the very first position (offset === 0) with collapsed selection, without any content and sibling (i.e. empty editor).
  2. then it pushes additional history per each DELETE_CHARACTER_COMMAND that has done nothing.

Link to code example: can reproduce it on playground.

The current behavior

before.mov

The expected behavior

It should work as it has some content:
DELETE or BACKSPACE command which does nothing doesn't push history

expected.mov
@2wheeh 2wheeh changed the title Bug: DELETE_CHARACTER_COMMAND pushes meaningless histories Bug: DELETE_CHARACTER_COMMAND pushes meaningless history Dec 7, 2023
@2wheeh
Copy link
Contributor Author

2wheeh commented Dec 8, 2023

And also collapseAtStart of HeadingNode acts a bit weird for me to see it:
On collapseAtStart, it is implemented to replace with a new one when it has child. So it ends up with pushing history that does nothing as well.

before-heading.mov

I wonder why we have to make a new HeadingNode in this case 🤔. What is the intention of doing this ?
If it is not needed, below change can fix this for HeadingNode and we can focus on collapseAtStart of ParagraphNode

// lexical-rich-text/src/index.ts L371
// now
collapseAtStart(): true {
    const newElement = !this.isEmpty()
      ? $createHeadingNode(this.getTag())
      : $createParagraphNode();
    const children = this.getChildren();
    children.forEach((child) => newElement.append(child));
    this.replace(newElement);
    return true;
  }

// what about this ?
collapseAtStart(): true {
    if (!this.isEmpty()) {
      return true;
    }

    const newElement = $createParagraphNode();
    const children = this.getChildren();
    children.forEach((child) => newElement.append(child));
    this.replace(newElement);
    return true;
  }

kirandash added a commit to bgwebagency/lexical that referenced this issue Feb 20, 2025
- Simplify HeadingNode's collapseAtStart to prevent unnecessary node recreation
- Return early when no actual changes are needed
- Fixes facebook#5350 - DELETE_CHARACTER_COMMAND pushing meaningless history entries
kirandash added a commit to bgwebagency/lexical that referenced this issue Feb 20, 2025
- Fix test assertion for backspace at start of heading with no content
- Fix test assertion for backspace at start of heading with content
- Add multiple backspace operations to verify no history pollution
- Properly verify undo behavior reverts to previous meaningful state

Part of fix for facebook#5350 - DELETE_CHARACTER_COMMAND pushing meaningless history
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant