Opened 6 years ago

Closed 6 years ago

#10778 closed Bug (fixed)

Range is expanded when inserting a link on styled text

Reported by: Teresa Monahan Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.3.2
Component: Core : Styles Version: 4.2.1
Keywords: IBM Cc: Satya Minnekanti, Irina

Description

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>

Change History (9)

comment:1 Changed 6 years ago by Frederico Caldeira Knabben

Component: GeneralCore : Styles
Status: newconfirmed

comment:2 Changed 6 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:3 Changed 6 years ago by Piotr Jasiun

Status: assignedreview

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.

comment:4 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_failed

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' );
                            },
    
  1. 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 )

  1. 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.
  1. 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?

  1. Better pre-compile regex like /[^\s\ufeff]/.test( siblingText ) instead of re-defining it with every iteration of the walker.
  2. I commented that condition out and all tests passed. What is it for then? I don't get the comment anyway.
  3. onlyWhiteSpaces needs a few words of explanation. What does it return? And why (arguments)?
  4. 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.

comment:5 in reply to:  4 Changed 6 years ago by Piotr Jasiun

Status: review_failedreview

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()
			{
				// ...

			}
		// ...
	}
  1. 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 )

  1. 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.

  1. 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.

  1. Missing colon in 1. :)

Fixed.

  1. 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.

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

Done.

  1. 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.

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

Added.

  1. 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.

comment:6 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

I pushed 2 additional commits to dev.

comment:7 Changed 6 years ago by Olek Nowodziński

Milestone: CKEditor 4.3.2

comment:8 Changed 6 years ago by Piotrek Koszuliński

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

To be removed from log:

comment:9 Changed 6 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed

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

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

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy