Fix for: `editor.insertHtml` and `editor.insertElement` pollutes editable with empty spans #2987

engineering-this · 2019-03-27T10:11:35Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

Inside prepareRangeToDataInsertion I've added check that will remove created nodes for which isEmptyInlineRemoveable is true.

Closes #2813

jacekbogdanski

I'm still able to reproduce the issue using editor.insertElement function. IMO it doesn't make sense to fix only part of the issue if different insertion functions are still bugged.

I'm wondering if we can safely remove spans from content in such manner. They may be a part of some custom feature like bookmarks, although this issue: #2484 says otherwise and even our selection.createBookmars function creates spans with   character 🤔

These changes are pretty wide in the context of the whole editor. Although, I ran all unit tests on Chrome with no fails.

I'm also wondering if this PR shouldn't be a part of major release, because it may cause some breaking changes if someone gets around spans removal and using them as a part of some feature. WDYT?

jacekbogdanski · 2019-03-27T15:14:45Z

core/editable.js

@@ -1717,14 +1718,24 @@
 			// Split inline elements so HTML will be inserted with its own styles.
 			path = range.startPath();
 			if ( ( node = path.contains( isInline, false, 1 ) ) ) {
-				range.splitElement( node );
+				clone = range.splitElement( node );


A bit misleading thus this element is not a clone but the late element after the split. Maybe rename it into endNode?

jacekbogdanski · 2019-03-27T15:15:50Z

core/editable.js

 				that.inlineStylesRoot = node;
 				that.inlineStylesPeak = path.lastElement;
 			}

 			// Record inline merging candidates for later cleanup in place.
 			bm = range.createBookmark();

+			// When called by insertHtml remove empty element created after splitting (#2813).
+			if ( isHtml && clone && clone.isEmptyInlineRemoveable() ) {


Would be nice to extract both span node removals into a separate function to avoid duplication.

jacekbogdanski · 2019-03-27T15:17:41Z

tests/core/editable/inserthtmlintospan.js

@@ -0,0 +1,33 @@
+/* bender-tags: editor,insertion */


Do we really need a separate file for these tests? It looks like they could be placed inside tests/core/editable/insertion.js. WDYT?

jacekbogdanski · 2019-03-27T15:27:19Z

tests/core/editable/inserthtmlintospan.js

+
+			bender.tools.selection.setWithHtml( editor, initial );
+
+			editor.insertHtml( '<div>div</div>' );


I'm missing tests for editor.insertHtmlIntoRange which also uses prepareRangeToDataInsertion private function.

I've added such tests. Small note, it's a method of editable 😉

engineering-this · 2019-04-03T08:20:41Z

I'm also wondering if this PR shouldn't be a part of major release, because it may cause some breaking changes if someone gets around spans removal and using them as a part of some feature. WDYT?

It is possible that someone is somehow preserving empty spans (#2484). However this fix affects elements that were created or altered by removing selected content from selection containers thus it shouldn't affect empty spans that were in editor before insertion.

Also after looking at element.isEmptyInlineRemovable it looks like changing CKEDITOR.dtd.$removeEmpty[ elementName ] to false should be sufficient to make desired element safe from being removed when empty.

jacekbogdanski

It looks like editor.insertHtml doesn't work correctly with block elements and your tests exploit this issue:

Open tests/core/editable/manual/insert manual test.
Follow test steps for insert html button.

Expected

Div element is inserted under paragraph like it's done for insert element button.

Actual

Div element is inlined inside paragraph.

I see that there is no other easy way around this issue, so it would be nice to create separate ticket for it and reference the issue inside manual test. Also, don't forget to add information about workaround for manual test inside newly opened issue ticket, so we can get back to it when the issue is resolved.

I'm finding unit tests inside tests/core/editable/insert.js hightly unreadable. I see that you tried to save some LOC but it shouldn't be done in expense of readibility. As an example, the first test checking editor.insertHtml could be as simple as:

var editor = this.editor;

bender.tools.selection.setWithHtml( editor, '<span>{foo}bar</span>' );
editor.insertHtml( '<div>div</div>' );

assert.areEqual( '<div>div</div><span>bar</span>', editor.editable().getHtml() );

It also would be great to add tests verifying that empty edge spans are preserved for both inserted html and editor content. E.g. you could check if div html into <span>foo{baz} </span> preserves the space. We will be sure that your changes don't touch user content.

I'm also missing a manual test for insertHtmlIntoRange and insertElementIntoRange.

jacekbogdanski · 2019-04-08T08:25:18Z

core/editable.js

@@ -2286,6 +2301,10 @@
 		return insert;
 	} )();

+	function removeEmptyInline( element ) {
+		element && element.isEmptyInlineRemoveable() && element.remove();


Please, use if statement if a return value of the expression is irrelevant.

engineering-this · 2019-04-09T16:15:07Z

I'm finding unit tests inside tests/core/editable/insert.js hightly unreadable. I see that you tried to save some LOC but it shouldn't be done in expense of readibility. As an example, the first test checking editor.insertHtml could be as simple as:

I can rework test, so each test case is explicitly declared, however I'd prefer to use one general test callback which will test each case based on options param like we do in many test files. WDYT @jacekbogdanski

engineering-this · 2019-04-10T08:22:25Z

It looks like editor.insertHtml doesn't work correctly with block elements and your tests exploit this issue:

Extracted to #3042

engineering-this · 2019-04-15T10:52:43Z

It also would be great to add tests verifying that empty edge spans are preserved for both inserted html and editor content. E.g. you could check if div html into foo{baz} preserves the space. We will be sure that your changes don't touch user content.

I've added such test case, but I need to mention that when you press space in editor, when selection is at the end of any element, than   will be inserted, because otherwise space won't be rendered by browser, so   entity is used in tc.

jacekbogdanski

One test is failing on IE10, IE9:

jacekbogdanski · 2019-04-15T14:34:08Z

tests/core/editable/insertion.js

+		var methods = [ 'insertHtml', 'insertHtmlIntoRange', 'insertElement', 'insertElementIntoRange', 'insertText' ];
+
+		CKEDITOR.tools.array.forEach( methods, function( methodName ) {
+			tests[ options.name.replace( 'insert', methodName ) ] = function() {


Small suggestion to compose a test method name rather than replace insert string:

tests[ 'test ' + methodName + ' ' + options.name ] = ...

seems less hacky for me and more resistant for incorrect usage e.g. if we decide to add new test without insert prefix.

jacekbogdanski · 2019-04-15T14:36:03Z

tests/core/editable/insertion.js

+	}
+
+	function assertInsertionMethod( editor, expected, methodName, html ) {
+		var useRange = methodName.indexOf( 'Range' ) >= 0,


Maybe you could make it even more readable by using a switch statement instead of string sniffing directly from addInsertionTests?

jacekbogdanski · 2019-04-15T14:42:45Z

tests/core/editable/insertion.js

+	bender.test( tests );
+
+	function addInsertionTests( tests ) {
+		CKEDITOR.tools.array.forEach( tests, addInsertionTest );


tests variable already exists in this file. I know that function is very small, nevertheless, maybe you could rename it to avoid using the same name? Hoisting makes name covering a bit unreadable.

tests/core/editable/insertion.js

+	};
+
+	// Add tests for (#2813).
+	addInsertionTests( [


CHANGES.md

engineering-this · 2019-04-16T10:23:34Z

Apparently space character ' ' should be used instead of   on IE. This change result in multiple tests failing in IEs. I've extracted it to separate ticket #3061, as it seems to be unrelated with the issue we are fixing here. Failing test are not ignored on IE.

jacekbogdanski

Looks good, just some minor issues.

Failing test are not ignored on IE.

But the tests are ignored on IE 🤔

It would be nice to add a test case when you are inserting an empty element.

jacekbogdanski · 2019-04-23T10:24:48Z

tests/core/editable/insertion.js

@@ -4,7 +4,8 @@
 	'use strict';

 	var doc = CKEDITOR.document,
-		tools = bender.tools;
+		tools = bender.tools,
+		space = CKEDITOR.env.ie ? ' ' : '&nbsp;';


You are declaring space here for IE issue, but at the same time IE tests are ignored and the test is working on Edge without this modification.

I think this value should be here, as this is used for the IE, however because of other bug test is ignored. Once #3061 is resolved test can be just unignored.

jacekbogdanski · 2019-04-23T10:28:13Z

tests/core/editable/insertion.js

+			name: 'when selection is in the middle of span',
+			initial: '<span>foo{bar}' + space + '</span>',
+			data: '<div>div</div>',
+			expected: '<span>foo</span><div>div</div><span>' + space + '</span>',


It would be nice to add test covering the case when selection is preceded with space (<span> {foo}</span>) because you are removing it at https://github.com/ckeditor/ckeditor-dev/blob/2669504728055ff89038f8bf87828cb6925868ae/core/editable.js#L468-L469

This reverts commit 9214d3a.

f1ames · 2019-06-17T14:34:28Z

Rebased on the latest major.

f1ames

The recent tests fix looks good 👍

However, I have encountered a group of cases which I'm not sure how to treat them. It happens when selection spans over multiple block elements:

It still leaves empty elements, but it seems the cause is different. Not sure if we should treat it as separate issue 🤔 Also the empty spans are not focusable so caret cannot be placed inside and are removed on getData(). It kind of looks like behaviour related to this issue, WDYT? Alternatively we can treat it as a follow-up and fix if necessary.

engineering-this · 2019-06-18T07:25:43Z

Although it looks similar to the original bug it's caused by something else, so I'm for extracting it as a follow-up issue.

f1ames · 2019-06-18T08:48:33Z

Although it looks similar to the original bug it's caused by something else, so I'm for extracting it as a follow-up issue.

I have extracted it to #3177. Since it is present from 4.5 (or even older), there is no need to fix it unless really necessary and we should be careful to make sure nothing breaks since it touches multiple block elements.

f1ames

Ok, looks good 👍

engineering-this force-pushed the t/2813 branch from 728e5ff to e4f5440 Compare March 27, 2019 11:09

jacekbogdanski self-assigned this Mar 27, 2019

jacekbogdanski requested changes Mar 27, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski Mar 27, 2019

engineering-this self-assigned this Apr 2, 2019

engineering-this force-pushed the t/2813 branch from e4f5440 to 34ade1d Compare April 3, 2019 08:22

engineering-this changed the title ~~Fix for: editor.insertHtml pollutes editable with empty spans~~ Fix for: editor.insertHtml and editor.insertElement pollutes editable with empty spans Apr 3, 2019

engineering-this changed the base branch from master to major April 3, 2019 08:24

engineering-this requested a review from jacekbogdanski April 3, 2019 09:49

CKEditorBot unassigned engineering-this Apr 3, 2019

jacekbogdanski self-assigned this Apr 5, 2019

jacekbogdanski requested changes Apr 8, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski Apr 8, 2019

engineering-this self-assigned this Apr 9, 2019

engineering-this mentioned this pull request Apr 9, 2019

insertHtml inserts only text from given element #3042

Open

engineering-this requested a review from jacekbogdanski April 15, 2019 10:55

CKEditorBot unassigned engineering-this Apr 15, 2019

jacekbogdanski self-assigned this Apr 15, 2019

jacekbogdanski requested changes Apr 15, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski Apr 15, 2019

engineering-this self-assigned this Apr 15, 2019

engineering-this requested a review from jacekbogdanski April 16, 2019 10:27

CKEditorBot unassigned engineering-this Apr 16, 2019

jacekbogdanski self-assigned this Apr 23, 2019

jacekbogdanski requested changes Apr 23, 2019

View reviewed changes

engineering-this and others added 18 commits June 17, 2019 16:32

Tests: update for IE and ignore.

f4f2c5a

Changes: update entry.

782950b

Tests: add TC.

8d3cfcd

Correct conditional for Edge.

8c8f342

Remove IE variable space as test is ignored for IE.

b77b2b6

Tests: add case.

fdb2859

Simplify while condition.

ab6c760

Tests: reword test description.

4017e77

Styling.

4400615

Added extensive comment.

6f40410

Tests: Two new unit test cases added.

f90f93f

Tests: fix normalizeHtml method removing non empty paragraphs.

d1f5543

Tests: added various elements to be tested.

efee373

Revert "Changes: update entry."

940725d

This reverts commit 9214d3a.

Tests: fix for IE.

2c773b0

Updated changelog entry.

45733e3

Refactoring.

c3aa928

Tests: remove empty paragraph from the end of normalized html string.

7f08cc6

f1ames force-pushed the t/2813 branch from 50dd8c1 to 7f08cc6 Compare June 17, 2019 14:33

f1ames self-assigned this Jun 17, 2019

f1ames suggested changes Jun 17, 2019

View reviewed changes

f1ames mentioned this pull request Jun 18, 2019

Editor insertion methods may leave empty spans for selection over multiple block elements #3177

Open

f1ames self-requested a review June 18, 2019 08:54

f1ames approved these changes Jun 18, 2019

View reviewed changes

CKEditorBot unassigned f1ames Jun 18, 2019

f1ames merged commit 639a4b0 into major Jun 18, 2019

f1ames deleted the t/2813 branch June 18, 2019 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for: `editor.insertHtml` and `editor.insertElement` pollutes editable with empty spans #2987

Fix for: `editor.insertHtml` and `editor.insertElement` pollutes editable with empty spans #2987

engineering-this commented Mar 27, 2019 •

edited

jacekbogdanski left a comment

jacekbogdanski Mar 27, 2019

jacekbogdanski Mar 27, 2019

jacekbogdanski Mar 27, 2019

jacekbogdanski Mar 27, 2019

engineering-this Apr 3, 2019

engineering-this commented Apr 3, 2019

jacekbogdanski left a comment

jacekbogdanski Apr 8, 2019

engineering-this commented Apr 9, 2019 •

edited

engineering-this commented Apr 10, 2019

engineering-this commented Apr 15, 2019

jacekbogdanski left a comment

jacekbogdanski Apr 15, 2019

jacekbogdanski Apr 15, 2019

jacekbogdanski Apr 15, 2019

This comment was marked as resolved.

engineering-this commented Apr 16, 2019

jacekbogdanski left a comment

jacekbogdanski Apr 23, 2019

engineering-this Apr 26, 2019

jacekbogdanski Apr 23, 2019

f1ames commented Jun 17, 2019

f1ames left a comment

engineering-this commented Jun 18, 2019

f1ames commented Jun 18, 2019

f1ames left a comment


		bender.tools.selection.setWithHtml( editor, initial );

		editor.insertHtml( '<div>div</div>' );

Fix for: editor.insertHtml and editor.insertElement pollutes editable with empty spans #2987

Fix for: editor.insertHtml and editor.insertElement pollutes editable with empty spans #2987

Conversation

engineering-this commented Mar 27, 2019 • edited

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

jacekbogdanski 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

engineering-this commented Apr 3, 2019

jacekbogdanski left a comment

Choose a reason for hiding this comment

Expected

Actual

Choose a reason for hiding this comment

engineering-this commented Apr 9, 2019 • edited

engineering-this commented Apr 10, 2019

engineering-this commented Apr 15, 2019

jacekbogdanski 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

This comment was marked as resolved.

engineering-this commented Apr 16, 2019

jacekbogdanski 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

f1ames commented Jun 17, 2019

f1ames left a comment

Choose a reason for hiding this comment

engineering-this commented Jun 18, 2019

f1ames commented Jun 18, 2019

f1ames left a comment

Choose a reason for hiding this comment

Fix for: `editor.insertHtml` and `editor.insertElement` pollutes editable with empty spans #2987

Fix for: `editor.insertHtml` and `editor.insertElement` pollutes editable with empty spans #2987

engineering-this commented Mar 27, 2019 •

edited

engineering-this commented Apr 9, 2019 •

edited