Skip to content

Conversation

aryan7081
Copy link
Contributor

@aryan7081 aryan7081 commented Sep 20, 2025

Ref #7263

Inside the joinWithPreviousBlock function of packages/volto-slate/src/blocks/Text/keyboard/joinBlocks.js, pressing Backspace (Delete on mac) at the start of a block incorrectly merged the previous block's content into the current block, instead of merging the current block into previous one.

This happened because the implementation relied on mergeSlateWithBlockBackward(editor, otherBlock) which altered the current block's editor slate causing the wrong merge direction.

This fix removes the dependency and explicitly appends the current block's children (editor.children) into the previous block's value, then deletes the current block.

Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

This adds a new line to the previous paragraph. The expectation here is that the second paragraph will be added to the end of the previous paragraph, without adding a new line.

@aryan7081
Copy link
Contributor Author

@wesleybl Thanks for pointing this out. It is fixed in the latest commit. Now merging happens Inline without adding a new line.

@aryan7081 aryan7081 requested a review from wesleybl September 23, 2025 08:41
Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

After the merge, the cursor must be before the first character of the second paragraph.

@aryan7081
Copy link
Contributor Author

aryan7081 commented Sep 23, 2025

Fixed, the Cursor is now placed before the first character of the second paragraph.

@aryan7081 aryan7081 requested a review from wesleybl September 23, 2025 13:03
Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

When I press Backspace and then Enter, the content that was originally in the second paragraph remains in the first paragraph.

@aryan7081
Copy link
Contributor Author

@wesleybl PR updated:

  • Pressing Enter at the merge point now correctly splits into a new block.
  • Fixed linting issues.
  • Added the required changelog entry, but Git/Towncrier is not detecting the file. Could you advise on what might be missing?

@aryan7081 aryan7081 requested a review from wesleybl September 23, 2025 18:11
@davisagli
Copy link
Member

@aryan7081 You also need a changelog file in packages/volto/news since you added the cypress test there. Use the extension .internal for that one since the change there is not a user-facing change.

@aryan7081
Copy link
Contributor Author

@davisagli Thanks for the guidance! I’ve added the internal changelog for the Cypress test.

Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

I positioned the cursor before the first character of a paragraph. Then I pressed Enter several times. When I press backspace once, it removed the previous block. But after that, I had to press backspace twice to remove the remaining blocks.

@@ -0,0 +1 @@
Fix Backspace at start of a text block: merge current block into previous inline (no extra newline), delete the current block, and place the caret before the first character of the merged content. Also handle Enter immediately after such inline merge by splitting back into two blocks. (@aryan7081)
Copy link
Member

Choose a reason for hiding this comment

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

We generally do not put the author's name in parentheses.

@aryan7081
Copy link
Contributor Author

@wesleybl I have removed the author's name from the changelog entry and fixed the issue of pressing backspace twice to remove the remaining blocks.

@aryan7081 aryan7081 requested a review from wesleybl September 24, 2025 04:37
@@ -0,0 +1 @@
Fix Backspace at start of a text block: merge current block into previous inline (no extra newline), delete the current block, and place the caret before the first character of the merged content. Also handle Enter immediately after such inline merge by splitting back into two blocks.
Copy link
Member

Choose a reason for hiding this comment

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

Please add your username in the note.


// Else the editor contains characters, so we merge the current block's
// `editor` with the block before, `otherBlock`.
const cursor = mergeSlateWithBlockBackward(editor, otherBlock);
Copy link
Member

Choose a reason for hiding this comment

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

Why weren't the changes made to mergeSlateWithBlockBackward? Have you considered whether the code there is needed? Even if it isn't needed, the implementation can be done there to make the code more modular and separate. mergeSlateWithBlockBackward is only used here.

@@ -0,0 +1 @@
Added Cypress test for backspace behavior in slate blocks.
Copy link
Member

Choose a reason for hiding this comment

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

Please add your username in the note.

@aryan7081
Copy link
Contributor Author

@wesleybl Thanks, I investigated and found that mergeSlateWithBlockBackward wasn't being used anywhere, so I fixed the bug in that function. Now, joinWithPreviousBlock uses it instead of duplicating custom logic. Also, I have added the username to the log files.

@aryan7081 aryan7081 requested a review from wesleybl September 25, 2025 05:36
export * from './unwrapEmptyString';
export * from './slashMenu';
export * from './cancelEsc';
export * from './splitAtSeam';
Copy link
Member

Choose a reason for hiding this comment

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

Don't use barrel import. Remove this line and import the full path.

@aryan7081
Copy link
Contributor Author

@wesleybl I have replaced barrel import with direct module import.

@aryan7081 aryan7081 requested a review from wesleybl September 25, 2025 18:15
Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

LGTM

@aryan7081
Copy link
Contributor Author

aryan7081 commented Oct 3, 2025

@wesleybl May I know the reason why you have edited the pr description from Fixes #7263 to Ref #7623? See https://6.docs.plone.org/contributing/first-time.html#create-a-pull-request-from-your-fork item #2. Thank you!

@wesleybl
Copy link
Member

wesleybl commented Oct 3, 2025

@aryan7081 It's because this PR doesn't fix #7263. They are similar but not the same situations.

@aryan7081
Copy link
Contributor Author

@wesleybl Okay, got it. Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants