-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
insertParagraph command creates additional empty paragraph when used at the end of block element #14510
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the problem is that currently with the following structure: <h2>[]</h2>
, writer.split()
does exactly what we tell it - splits it into <h2>[]</h2><h2></h2>
, and then we insert a paragraph in the middle (same applies for the <p>
elements, but I used <h2>
to make it clearer), ending up with two new blocks instead of one: <h2></h2><p>[]</p><h2></h2>
. So I agree that we problem lies within the command, not writer.split()
method.
The proposed solution makes sense. Nice that you've noticed that the issue occurs also at the beginning of the blocks, not just at the end. Let's clean it up a little and we'll be ready to pass the PR to the Core team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, waiting for CI.
// <paragraph>[]</paragraph> ---> <paragraph></paragraph><paragraph>[]</paragraph> | ||
const isInEmptyBlock = position.isAtStart && position.isAtEnd; | ||
|
||
// E.g. | ||
// <paragraph>foo[]</paragraph> ---> <paragraph>foo</paragraph><paragraph>[]</paragraph> | ||
const isAtEndOfTextBlock = position.isAtEnd && position.nodeBefore?.is( '$text' ); | ||
|
||
// E.g. | ||
// <paragraph>[]foo</paragraph> ---> <paragraph>[]</paragraph><paragraph>foo</paragraph> | ||
const isAtStartOfTextBlock = position.isAtStart && position.nodeAfter?.is( '$text' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about inline elements? For example inline image? Maybe you could check position.parent.isEmpty
instead of checking nodeBefore
/nodeAfter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to keep previous behaviour for non-text elements, look at this test: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-widget/tests/widgettypearound/widgettypearound.js#L1056-L1089
here this kind behaviour is expected for widgets then we fix issue only for text nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about inline widgets, not block widgets. They are allowed everywhere text is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to reflect my comment about inline widgets.
Suggested merge commit message (convention)
Fix (paragraph): "insertParagraph" command doesn't insert two paragraphs when position is at the edge of the line. Closes #13866.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.