`cloneContents` properly return document fragment #642

msamsel · 2017-07-12T15:00:52Z

What is the purpose of this pull request?

bug fix

Does your PR contain necessary tests?

yep!

This PR contains

Unit tests
Manual tests

What changes did you make?

Modify if statement to accept situation when selection start and ends in this same element, but startNode has type of element where endNode has type of text

close #426

f1ames

There are 4 test failing on Safari:

f1ames · 2017-07-14T08:36:39Z

core/dom/range.js

-		if ( isClone && endNode.type == CKEDITOR.NODE_TEXT && startNode.equals( endNode ) ) {
-			startNode = range.document.createText( startNode.substring( startOffset, endOffset ) );
-			docFrag.append( startNode );
+		// #426 We need to handle situation when selection is located at the beginning of node,


Ticket number should be on the end. You can add one empty line so this description does not mixed up with the previous one.

f1ames · 2017-07-14T08:39:40Z

core/dom/range.js

-			docFrag.append( startNode );
+		// #426 We need to handle situation when selection is located at the beginning of node,
+		// so in selection range start node get type == NODE_ELEMENT instead of NODE_TEXT
+		// We gain selecion like <b>[foo} bar</b>, instead of <b>{foo} bar</b>


so range start node is of type NODE_ELEMENT (instead of NODE_TEXT). It happens for selection like [foo} bar instead of {foo} bar.

?

f1ames · 2017-07-14T08:43:59Z

core/dom/range.js

+
+			// endNode is always text
+			endNode = range.document.createText( endNode.substring( startOffset, endOffset ) );
+			docFrag.append( endNode );


Is endNode used somewhere else? Maybe you could use

docFrag.append( range.document.createText( endNode.substring( startOffset, endOffset ) ) );

I know this is the way the former code was formatted, but assigning new result to endNode - especially if it is not used any further may look somehow confusing.

f1ames · 2017-07-14T09:01:09Z

tests/core/dom/range/clonecontents.js

+
+		// Variety of edge test cases with selection range in text and elements nodes inside one element and multiple ones (#426).
+		'test cloneContents - inner selection1': function() {
+			this.assertHtmlFragment( this.editors.classic, '<p>foo <strong>[bar} baz</strong> foo</p>', 'bar' );


Since we check cases with selection on element boundaries, you may consider extending tests with cases like:

 (no text between p and strong - same with text after strong),

foo[bar</> ba}z (some nested elements),

<tr><td>... (many levels of nested tags).

tests was added at the end.

f1ames · 2017-07-14T09:04:04Z

tests/core/dom/range/manual/clonecontentsstartnodeproblem.html

+	var editor = CKEDITOR.replace( 'editor1' );
+
+	document.getElementById( 'getFragment' ).onclick = function() {
+		// debugger;


Please do not leave commented code like this.

f1ames · 2017-07-14T09:53:29Z

tests/core/dom/range/manual/clonecontentsstartnodeproblem.md

+@bender-ui: collapsed
+@bender-ckeditor-plugins: divarea, toolbar, basicstyles
+
+1. Make selection of **bar** word


Select bar.

?

f1ames · 2017-07-14T09:54:00Z

tests/core/dom/range/manual/clonecontentsstartnodeproblem.md

+@bender-ckeditor-plugins: divarea, toolbar, basicstyles
+
+1. Make selection of **bar** word
+1. Press button below editor


Missing . on the sentence end.

f1ames · 2017-07-14T10:12:44Z

@msamsel There are some differences how selection works in Safari which might be the issue here. When you have selection like [Text]/[Text], Safari always reports it as {Text} - so as attached in text nodes instead of element nodes AFAIR (basically, such selection is always attached in text nodes not element nodes).

f1ames · 2017-07-14T10:46:05Z

I think I run unit tests with some local changes so I have to recheck it.

f1ames · 2017-07-14T12:35:08Z

I rechecked test, Edge/IE11 is fine, however Safari still fails (updated the comment).

f1ames · 2017-07-14T12:48:04Z

I also run those added unit test without the fix in a code and there is only on test failing on Chrome, FF, Edge, IE9 - IE11 which is tests/core/dom/range/clonecontents test cloneContents - inner selection1. Maybe we could reduce the amount of tests or find more failing cases (if there is more than one).

None test is failing on IE8 - you should probably check if the issue is reproducible there, if not we may skip this browser.

On Safari 4 mentioned earlier tests still fails without the fix.

msamsel · 2017-07-18T10:00:51Z

Because IE8 and Safari handling a little bit differently with selection than other browser, so I have to ignore part of the tests on those browsers. Generally they kept selection inside specific node, what would be repetition of earlier tests.

f1ames

Just 2 small things and we are ready to go.

f1ames · 2017-07-27T12:49:38Z

tests/core/dom/range/clonecontents.js

@@ -557,32 +557,60 @@
 		'test cloneContents - inner selection6': function() {
 			this.assertHtmlFragment( this.editors.classic, '<p>foo <strong>bar {baz]</strong> foo</p>', 'baz' );
 		},
+		// Safari always keeps selection insdie node.


Such comments should be right before ignore (so it has a proper context):

'test cloneContents - outer selection1': function() { // Safari always keeps selection insdie node. if ( CKEDITOR.env.safari ) { assert.ignore(); } this.assertHtmlFragment( this.editors.classic, 'foo {bar] baz foo', 'bar' ); },

Same for the one below about IE8 ignore.

Also there is a typo in inside selection insdie node.

f1ames · 2017-07-27T12:54:21Z

core/dom/range.js

+			endNode.type == CKEDITOR.NODE_TEXT &&
+			( startNode.equals( endNode ) || ( startNode.type === CKEDITOR.NODE_ELEMENT && startNode.getFirst().equals( endNode ) ) ) ) {
+
+			// this should be always text


Remember about using full sentences like This should be always text.

…ferently select text.

f1ames

LGTM 👍

f1ames self-requested a review July 14, 2017 07:03

f1ames suggested changes Jul 14, 2017

View reviewed changes

f1ames self-requested a review July 14, 2017 10:45

msamsel force-pushed the t/426 branch from 3cd6e6c to 4eb306a Compare July 17, 2017 14:44

f1ames suggested changes Jul 27, 2017

View reviewed changes

Mateusz Samsel added 9 commits July 27, 2017 16:46

Unit test for incorrect cloning.

f8f0ab1

Modify unit test to cover more edge cases

7abbc28

Improving fix

479a203

Adding manual tests and comment in unit test.

2b0838e

Add end character to fix test on IE8

917d682

Fix of solution after review

0f55c02

Fix test issues after review

005ea42

Fix for tests. Implement conditions for different browsers, which dif…

0ddf43c

…ferently select text.

Small comment improvements after review.

8cfe9c4

msamsel force-pushed the t/426 branch from 3fd74cd to 8cfe9c4 Compare July 27, 2017 14:46

f1ames approved these changes Jul 28, 2017

View reviewed changes

Changelog entry.

96064b2

f1ames merged commit 5655252 into master Jul 28, 2017

f1ames deleted the t/426 branch July 28, 2017 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`cloneContents` properly return document fragment #642

`cloneContents` properly return document fragment #642

msamsel commented Jul 12, 2017 •

edited

Loading

f1ames left a comment •

edited

Loading

f1ames Jul 14, 2017

f1ames Jul 14, 2017

f1ames Jul 14, 2017

f1ames Jul 14, 2017

msamsel Jul 17, 2017

f1ames Jul 14, 2017

f1ames Jul 14, 2017

f1ames Jul 14, 2017

f1ames commented Jul 14, 2017

f1ames commented Jul 14, 2017 •

edited

Loading

f1ames commented Jul 14, 2017

f1ames commented Jul 14, 2017 •

edited

Loading

msamsel commented Jul 18, 2017

f1ames left a comment

f1ames Jul 27, 2017

f1ames Jul 27, 2017

f1ames left a comment

cloneContents properly return document fragment #642

cloneContents properly return document fragment #642

Conversation

msamsel commented Jul 12, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames left a comment • edited Loading

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

f1ames commented Jul 14, 2017

f1ames commented Jul 14, 2017 • edited Loading

f1ames commented Jul 14, 2017

f1ames commented Jul 14, 2017 • edited Loading

msamsel commented Jul 18, 2017

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

`cloneContents` properly return document fragment #642

`cloneContents` properly return document fragment #642

msamsel commented Jul 12, 2017 •

edited

Loading

f1ames left a comment •

edited

Loading

f1ames commented Jul 14, 2017 •

edited

Loading

f1ames commented Jul 14, 2017 •

edited

Loading