Mention post-fixer now correctly handles undo. #17

jodator · 2019-03-28T13:16:40Z

Fix: Mention post-fixer now correctly handles partial mentions on insert, remove and paste. Closes ckeditor/ckeditor5#4629. Closes ckeditor/ckeditor5#4622.

scofalik · 2019-03-29T09:39:47Z

Three things:

In insertions, you need to check deeply for inserted text nodes. When you inserted a paragraph, a partial mention might be in that paragraph. Differ returns only the top-level changes (it is optimized for downcasting). You may write it as a util (or a method on Differ) because it is not the first time we are having trouble with this. Or we could have some kind of flag like .getChanges( { deep: true } );.
Shift+enter and also pasting does not work correctly.
If we agree that mentions are always on exactly one text node (we don't allow partial styling) I am not sure if we need the "additional check for deleting last character". When is it needed? If you delete the last character in mention then, well, first of all, it would have to be a one-character-mention :D second of all, you don't need to remove the attribute from it if the whole mention is gone. So I think it is not needed?

jodator · 2019-03-29T10:01:52Z

If we agree that mentions are always on exactly one text node (we don't allow partial styling) I am not sure if we need the "additional check for deleting last character". When is it needed? If you delete the last character in mention then, well, first of all, it would have to be a one-character-mention :D second of all, you don't need to remove the attribute from it if the whole mention is gone. So I think it is not needed?

AFAIR it's an edge-case when selection is after mention and you remove last character (ie d in @Ted) then the previous check wouldn't work as selection is not inside text node.

scofalik · 2019-03-29T10:04:27Z

Oh, ok, you meant "last" as "right-most" not "the only" :D. Yeah, I checked it and you are right.

scofalik · 2019-03-29T10:10:19Z

So, in this case, we also need a check for the "left-most" character and I'd like to see a more clear comment. Use "left-most" and "right-most" or "at the beginning" and "at the end" instead of "first" and "last" to make it non-ambiguous.

Also, considering the other changes, I think that maybe structuring the post-fixer this way will be better:

for ( ... ) {
    if ( change.type == 'insert' ) {
        // Logic for insertions of text and non-text inside the mention.
        // And also looking for partial mentions in inserted content.
    } else if ( change.type == 'remove' ) {
        // Logic for removing of text inside, at the beginning and at the end of a mention.
    }
}

…tion post-fixer.

…ents and splitting parent.

…inside a mention.

…redundant condition check.

jodator · 2019-03-31T20:59:05Z

@scofalik: Ready to re-review. ~~Two follow ups~~ issues has been fixed:

~~MentionUI doesn't have 100% CC #20~~
~~You can type 1 char with mention (after changing selection attributes - ie enabling typing with bold). #21~~

scofalik

Some changes required :P

Other than that I miss two things in tests:

In attribute post-fixer, I'd like to see if unbolding also works correctly (you can add it to the existing unit tests).
I am missing clipboard test (pasting something like <blockquote>Foo bar@jod</blockquote>.

src/mentionediting.js

scofalik · 2019-04-02T09:21:07Z

src/mentionediting.js

+		}
+
+		// Check text nodes on inserted elements (might occur when splitting paragraph on enter key).
+		if ( change.name != '$text' && change.type == 'insert' && schema.checkChild( change.name, '$text' ) ) {


schema.checkChild() looks like a premature optimization. Is it needed? Does not having it produce any bugs?

Also, I think this might be incorrect, for example what if following is pasted:

<blockquote>@jodFoo</blockquote>

blockQuote cannot contain text but there is still text node somewhere inside.

But this is fixed by upcast converter. This check is for splitting parent element with mention when split occurred inside text with mention. As such there is no need to check other cases then the elements that allow text inside.

src/mentionediting.js

scofalik · 2019-04-02T09:28:42Z

src/mentionediting.js

-		if ( change.type == 'remove' ) {
-			const nodeBefore = change.position.nodeBefore;
+		// Inserted inline elements might break mention.
+		if ( change.type == 'insert' && schema.isInline( change.name ) ) {


After resolving the above comment this might be the last usage of schema in this postfixer. If this is for optimization purposes, I'd consider removing this and removing the need of passing schema here. Keep it simple.

checkAttributeMentionOnNode already checks if the node is text so it will be safe.

no it explicitly checks for inserted in-line elements like <softBreak> which may break text (with mention.

src/mentionediting.js

scofalik · 2019-04-02T09:46:54Z

src/mentionediting.js

-			if ( change.name == '$text' && textNode && textNode.hasAttribute( 'mention' ) ) {
-				writer.removeAttribute( 'mention', textNode );
-				wasChanged = true;
+			for ( const child of insertedNode.getChildren() ) {


Same as above, if a deep structure was pasted, you need to go deeper.

for ( const item of writer.createRangeIn( insertedNode ) ) { ... }

scofalik · 2019-04-02T10:16:20Z

src/mentionediting.js

+	// Yield text nodes with broken mention from the range.
+	for ( const textProxy of range.getItems() ) {
+		if ( !checkMentionAttributeOnNode( textProxy ) ) {
+			yield textProxy.textNode;


I understand that this is covered by unit tests but I wonder out of curiosity when this happens?

I mean, if that happens for mentions that are not on the boundary of the range. Some something like [foo<mention />bar].

So this is used in attribute post-fixer:

fo[o <$text mention="{...}">men]tion</$text>

So when the range on which bold is applied is ends inside a mention. Then I want to get that dangling mention node on the right(left) side of range.end(start).

So isn't it that we are always interested in nodes next to range boundaries?

When applying attribute on this range yes.

tests/mentionediting.js

Co-Authored-By: jodator <jodator@gmail.com>

jodator · 2019-04-02T12:03:41Z

@scofalik I've resolved all the issues from your comments.

I would leave those optimizations are they checks cases that might break mention during editing.

The pasting part of nested nodes is done by upcast converter - it does not allow partial mentions.

I've added tests for undo integration with attribute post-fixer.

scofalik · 2019-04-02T15:20:11Z

The pasting part of nested nodes is done by upcast converter - it does not allow partial mentions.

Typically, I prefer having general solutions that cover the whole problem instead of directly solving the predicted cases. This is safer (and, in my opinion, correct) way to go. For example, I just had to fix a bug in view writer because some behavior was predicted only for one case while it could simply be inside a for loop following the special cases and applying wrapping/unwrapping to all children recursively.

This is why I have mixed feelings about it. I don't want to introduce redundant code that might never be used. But I also don't want to introduce potentially bad code. For now, if I didn't miss anything, the code is fine. However, if there is ever a feature (widget) that breaks blockquote, there will be a problem:

<blockQuote><paragraph><$text mention="jodator">@jod[]ator</$text></paragraph></blockQuote>

Paste the widget that can be only in the root at []:

<blockQuote><paragraph><$text mention="jodator">@jod</$text></paragraph></blockQuote>
<hypotethicalWidget />
<blockQuote><paragraph><$text mention="jodator">ator</$text></paragraph></blockQuote>

Or when a 3rd party plugin will simply split both paragraph and blockQuote. Or if we will support more nested structure and for some reason, we will break multiple blocks at once.

Anyway, it is hypothetical for now. I tried to think about cases in OT and block lists (the thing we plan to introduce, where you will be able to, for example, add multiple paragraphs, headers, maybe tables to list items) but I don't know if anything there will break this behavior.

So as I said, I have mixed feelings. I summon @Reinmar .

…range boundaries.

jodator · 2019-04-02T15:58:20Z

Paste the widget that can be only in the root at []:

OK valid TC. I've manged to do it with

blockquote > paragraph > mention

and split:

editor.model.change( writer => {
 writer.split( editor.model.document.selection.focus, editor.model.document.getRoot() );
} );

I'll update the PR for deep check.

Mention post-fixer now correctly handles undo.

660a2cb

jodator requested a review from Reinmar March 28, 2019 13:16

Add missing docs.

618367b

scofalik requested review from scofalik and removed request for Reinmar March 28, 2019 17:28

scofalik added the status:review- label Mar 29, 2019

jodator added 8 commits March 29, 2019 14:28

Merge branch 'master' into t/12

089d6a5

Add extend attribute on mention post-fixer and fix remove partial men…

837b23d

…tion post-fixer.

Reorganize mention editing tests.

2656bf3

Removing partial mention post-fixer now detects inserting inline elem…

81a2daa

…ents and splitting parent.

Removing partial mention post-fixer now detects inserting text nodes …

36d12b6

…inside a mention.

Remove redundant checks from remove partial mention post-fixer.

5a2148d

Add missing test case for attribute on mention post-fixer and remove …

a2c4220

…redundant condition check.

Refactor extracting broken mentions from range and its boundaries.

99e3b6b

jodator removed the status:review- label Mar 31, 2019

jodator added 2 commits April 1, 2019 08:02

Fix mention ui tests.

402cd78

Add test for typing with selection attributes set.

dc1d06f

scofalik suggested changes Apr 2, 2019

View reviewed changes

scofalik added the status:review- label Apr 2, 2019

scofalik and others added 6 commits April 2, 2019 12:27

Update src/mentionediting.js

02ff73b

Co-Authored-By: jodator <jodator@gmail.com>

Update src/mentionediting.js

6fbe6d1

Co-Authored-By: jodator <jodator@gmail.com>

Update src/mentionediting.js

5f58453

Co-Authored-By: jodator <jodator@gmail.com>

Rename checkMentionAttributeOnNode() to isBrokenMentionNode().

327dd8f

Improve removePartialMentionPostFixer() code style.

6a287e1

Move checkAndFix() method out of post-fixer.

7bd26d5

jodator added 4 commits April 2, 2019 13:21

Add undo integration tests for attribute post-fixer.

65d76b6

Remove redundant check in mention post-fixer.

6f5f20c

Update comment in partial mention post-fixer.

9892ccc

Add integration test with clipboard.

e0cd1d7

jodator requested a review from scofalik April 2, 2019 12:03

jodator removed the status:review- label Apr 2, 2019

Attribute on mention post-fixer will only check text nodes at change …

d566c6e

…range boundaries.

jodator added 2 commits April 2, 2019 18:09

Add deep check of inserted item in the search of a broken mention.

c1c64b0

Use range.getItem() instead of range.getWalker().

90d12eb

scofalik merged commit 07778e3 into master Apr 2, 2019

scofalik deleted the t/12 branch April 2, 2019 16:17

jodator mentioned this pull request Oct 9, 2019

Mention UI doesn't have 100% CC ckeditor/ckeditor5#4631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mention post-fixer now correctly handles undo. #17

Mention post-fixer now correctly handles undo. #17

jodator commented Mar 28, 2019 •

edited by pomek

Loading

scofalik commented Mar 29, 2019 •

edited

Loading

jodator commented Mar 29, 2019 •

edited

Loading

scofalik commented Mar 29, 2019

scofalik commented Mar 29, 2019

jodator commented Mar 31, 2019 •

edited

Loading

scofalik left a comment

scofalik Apr 2, 2019

jodator Apr 2, 2019

scofalik Apr 2, 2019

scofalik Apr 2, 2019

jodator Apr 2, 2019

scofalik Apr 2, 2019

scofalik Apr 2, 2019

scofalik Apr 2, 2019

jodator Apr 2, 2019

scofalik Apr 2, 2019

jodator Apr 2, 2019

jodator commented Apr 2, 2019

scofalik commented Apr 2, 2019

jodator commented Apr 2, 2019

Mention post-fixer now correctly handles undo. #17

Mention post-fixer now correctly handles undo. #17

Conversation

jodator commented Mar 28, 2019 • edited by pomek Loading

scofalik commented Mar 29, 2019 • edited Loading

jodator commented Mar 29, 2019 • edited Loading

scofalik commented Mar 29, 2019

scofalik commented Mar 29, 2019

jodator commented Mar 31, 2019 • edited Loading

scofalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator commented Apr 2, 2019

scofalik commented Apr 2, 2019

jodator commented Apr 2, 2019

jodator commented Mar 28, 2019 •

edited by pomek

Loading

scofalik commented Mar 29, 2019 •

edited

Loading

jodator commented Mar 29, 2019 •

edited

Loading

jodator commented Mar 31, 2019 •

edited

Loading