Range is expanded when inserting a link on styled text

[Teresa Monahan]

To Reproduce:

• Enter bold text followed by a space in the editor e.g. 'test '
• Select the text you entered but not the space character
• Open the Link dialog and insert a URL. Press OK

Problem: The anchor tag has been added as a parent of the strong tag. You can see this from the elements path bar - a is listed before strong. Also if you click after the link and type some text, you can clearly see that the space character has been included as part of the link.

The HTML after the link is inserted is:

<p><a href="http://wwwwwwww"><strong>test </strong></a></p>

It should be:

<p><strong><a href="http://wwwwwwww">test</a> </strong></p>

[Frederico Caldeira Knabben]

​http://ckeditor4.t/tt/10778/1.html

[Piotr Jasiun]

If there is a space at the end of paragraph <p>[foo] </p> it is not displayed in WYSIWYG mode and it should be included into range by enlarge function ([<p>foo </p>]). This was the reason of this bug.

Now I use walker to check if there are only whitespaces till the end of block and enlarge range only if so.

Changes in ​t/10778 and corresponding test branch.

[Olek Nowodziński]

I rebased both branches and pushed minor commits here and there.

Tests

1. Why is that test gone in 820eb4a9?

   @@ -682,26 +925,9 @@
                                   assert.areSame( document.getElementById( '_EnlargeB1' ).childNodes[ 2 ], range.endContainer.$, 'range.endContainer' );
                                   assert.areSame( 5, range.endOffset, 'range.endOffset' );
                                   assert.isFalse( range.collapsed, 'range.collapsed' );
   -                       } :
   -                       function()
   -                       {
   -                               // <p> Test<b> <i>  [Enlarge</i> this]</b>   </p>
   -                               // <p> Test<b> [<i>  Enlarge</i> this]</b>   </p>
   -
   -                               var range = new CKEDITOR.dom.range( doc );
   -                               range.setStart( doc.getById( '_EnlargeI1' ).getFirst(), 2 );
   -                               range.setEnd( doc.getById( '_EnlargeB1' ).getChild( 2 ), 5 );
   -
   -                               range.enlarge( CKEDITOR.ENLARGE_ELEMENT );
   -
   -                               assert.areSame( document.getElementById( '_EnlargeB1' ), range.startContainer.$, 'range.startContainer' );
   -                               assert.areSame( 1, range.startOffset, 'range.startOffset' );
   -                               assert.areSame( document.getElementById( '_EnlargeB1' ).childNodes[ 2 ], range.endContainer.$, 'range.endContainer' );
   -                               assert.areSame( 5, range.endOffset, 'range.endOffset' );
   -                               assert.isFalse( range.collapsed, 'range.collapsed' );
                           },
   

2. document.documentMode > 8 is not CKEDITOR.env.version > 8. document.documentMode is undefined in non-IE browsers. It means that while the first condition would fail for all non-IE browsers, the second one passes in almost every browser regardless of vendor. It totally changes the flow of tests (was that intentional?).

document.documentMode > 8 is more like ( CKEDITOR.env.ie && CKEDITOR.env.version > 8 )

3. Tests: What's the meaning of pipe in test space 11&12? i.e.

   // <div>x<p>[foo] |bar</p>x</div>`
   

   It's not obvious and, IIRC, this is none of our standard notations.

4. Is that behavior correct, really (test space 1)?

   // <div>x<p><b>[foo] </b>bar</p>x</div> ->
   // <div>x[<p><b>foo] </b>bar</p>x</div>
   

   I mean, in general, it's assumed that range.enlarge( CKEDITOR.ENLARGE_ELEMENT ); will expand the range symmetrically. It worries me a lot that range#enlarge is no longer the same thing described in the doc-string, namely:

   Expands the range so that partial units are completely contained.

   Even if such behavior is a remedy for a particular case, I'm not quite sure whether it's safe to introduce it globally because there's a chance that existing bits of code strongly rely on the symmetry. Perhaps we need a separate method?

DEV

1. The ​condition inside of the guard should be simplified.
2. Missing colon in 1. :)
3. I'm wondering if element#isEditable would be better in this ​condition. What would happen if enlarge approached a display:none element? Or one of CKEDITOR.dtd.$empty? Is that case covered by tests?

I commented that one out and all tests passed, so what is that for anyway?

4. Better pre-compile regex like /[^\s\ufeff]/.test( siblingText ) instead of re-defining it with every iteration of the walker.
5. I commented ​that condition out and all tests passed. What is it for then? I don't get the comment anyway.
6. onlyWhiteSpaces needs a few words of explanation. What does it return? And why (arguments)?
7. If we're fine with Tests: 4., range#enlarge needs extended doc string including a few examples because the logic is definitely different.

---

Anyway, regardless of aforementioned (minor) concerns, I must frankly admit that I'm hardly able to penetrate the logic introduced here and there so I trust in tests. So if we get through all the obstacles in the list and eventually find Tests: 4. not relevant I'll be for R+ since the issue is fixed and nothing else seems to be broken.

[Piotr Jasiun]

Replying to a.nowodzinski:

I rebased both branches and pushed minor commits here and there.

Tests

1. Why is that test gone in 820eb4a9?

   @@ -682,26 +925,9 @@
                                   assert.areSame( document.getElementById( '_EnlargeB1' ).childNodes[ 2 ], range.endContainer.$, 'range.endContainer' );
                                   assert.areSame( 5, range.endOffset, 'range.endOffset' );
                                   assert.isFalse( range.collapsed, 'range.collapsed' );
   -                       } :
   -                       function()
   -                       {
   -                               // <p> Test<b> <i>  [Enlarge</i> this]</b>   </p>
   -                               // <p> Test<b> [<i>  Enlarge</i> this]</b>   </p>
   -
   -                               var range = new CKEDITOR.dom.range( doc );
   -                               range.setStart( doc.getById( '_EnlargeI1' ).getFirst(), 2 );
   -                               range.setEnd( doc.getById( '_EnlargeB1' ).getChild( 2 ), 5 );
   -
   -                               range.enlarge( CKEDITOR.ENLARGE_ELEMENT );
   -
   -                               assert.areSame( document.getElementById( '_EnlargeB1' ), range.startContainer.$, 'range.startContainer' );
   -                               assert.areSame( 1, range.startOffset, 'range.startOffset' );
   -                               assert.areSame( document.getElementById( '_EnlargeB1' ).childNodes[ 2 ], range.endContainer.$, 'range.endContainer' );
   -                               assert.areSame( 5, range.endOffset, 'range.endOffset' );
   -                               assert.isFalse( range.collapsed, 'range.collapsed' );
                           },
   

This part of test is IE only and this test was for... not IE. It would be never executed:

	// IE only tests.
	CKEDITOR.env.ie && YUITest.Util.mix( tests, {
		// ...

		test_enlarge_element5 : CKEDITOR.env.ie ?
			function()
			{
				// ...
			} 
		:
			function()
			{
				// ...

			}
		// ...
	}

2. document.documentMode > 8 is not CKEDITOR.env.version > 8. document.documentMode is undefined in non-IE browsers. It means that while the first condition would fail for all non-IE browsers, the second one passes in almost every browser regardless of vendor. It totally changes the flow of tests (was that intentional?).

As above, these are IE-only test so CKEDITOR.env.version and document.documentMode means the same but CKEDITOR.env.version is IMHO more readable.

document.documentMode > 8 is more like ( CKEDITOR.env.ie && CKEDITOR.env.version > 8 )

3. Tests: What's the meaning of pipe in test space 11&12? i.e.

   // <div>x<p>[foo] |bar</p>x</div>`
   

   It's not obvious and, IIRC, this is none of our standard notations.

I've added comment in code. These are two separate text nodes.

4. Is that behavior correct, really (test space 1)?

   // <div>x<p><b>[foo] </b>bar</p>x</div> ->
   // <div>x[<p><b>foo] </b>bar</p>x</div>
   

   I mean, in general, it's assumed that range.enlarge( CKEDITOR.ENLARGE_ELEMENT ); will expand the range symmetrically. It worries me a lot that range#enlarge is no longer the same thing described in the doc-string, namely:

   Expands the range so that partial units are completely contained.

   Even if such behavior is a remedy for a particular case, I'm not quite sure whether it's safe to introduce it globally because there's a chance that existing bits of code strongly rely on the symmetry. Perhaps we need a separate method?

I agree that this behavior is incorrect but it you run this test on master you will get the same result. I agree that some day whole enlarge function should be rewritten (and I really like to do such refactoring) but I needs plenty of tests and would takes more than mount. In this ticked I wanted to focus only on one but and left behavior of enlarged as unchanged as it is possible.

DEV

1. The ​condition inside of the guard should be simplified.

Done.

2. Missing colon in 1. :)

Fixed.

3. I'm wondering if element#isEditable would be better in this ​condition. What would happen if enlarge approached a display:none element? Or one of CKEDITOR.dtd.$empty? Is that case covered by tests?

I commented that one out and all tests passed, so what is that for anyway?

Walkers guard only check if it is the end of block. If fact we do not need this condition and I removed it.

4. Better pre-compile regex like /[^\s\ufeff]/.test( siblingText ) instead of re-defining it with every iteration of the walker.

Done.

5. I commented ​that condition out and all tests passed. What is it for then? I don't get the comment anyway.

I've added test and comment for this.

6. onlyWhiteSpaces needs a few words of explanation. What does it return? And why (arguments)?

Added.

7. If we're fine with Tests: 4., range#enlarge needs extended doc string including a few examples because the logic is definitely different.

As I explained in Tests: 4. I do not changed this messy behavior. Indeed it should be fixed and documented but it is different topic.

[Olek Nowodziński]

I pushed 2 additional commits to dev.

[Piotrek Koszuliński]

Befor merging to master please squash commits fixing code style into commits that introduced issues.

To be removed from log:

• ​https://github.com/cksource/ckeditor-dev/commit/ca63a1bcda2a00bd165d94a60b99636a3c36d840
• ​https://github.com/cksource/ckeditor-dev/commit/a4a53b3b8c39e6045774571056818854f44518e0

[Piotr Jasiun]

I've cleaned git history and close ticket with hashes:

• ​git:e3c7913
• tests:bb55f5a

Working on it I realized that enlanrge behavior is sometimes incorrect and created ticket for this #11374.