Create list on empty document error

[Garry Yao]

Some serious regression bug occurred in all browsers with different symptom.

Reproducing Procedures

1. Open the replace by class example page in IE.
2. Click on 'New Page' to clear document;
3. Now click on Ordered List to begin creating list;
   • Actual Result : JavaScript error occured, when beginning to type, the text doesn't fall into a list item.

[Garry Yao]

Regression of [3538], the incorrect part is for bookmark detection, we should keep stepping over the bookmark inner text node while keep the <span> node within count.

[Frederico Caldeira Knabben]

• The function name should be isBookmarkContent at this point, to not make it confusing.

• Following the test case, in IE the list is not created at first click.

• In IE, with enter mode set to <br>, we still have an error being thrown.

• In FF the cursor is in the wrong position when creating lists over empty paragraphs.

[Garry Yao]

Several defects were found during exploring Fred's comments:

In IE, with enter mode set to <br>, we still have an error being thrown

Arround list/plugin.js:L379, we say:

range.selectNodeContents( br ); // Result in a collapsed range(startContainer:br, startOffset:0, endContainer:br, startOffset:0)

This trunk of codes will result in a really a weird range and subsequent calls to 'range.insertNode()' will fail since <br> is accepting any child nodes.

In FF the cursor is in the wrong position when creating lists over empty paragraphs.

This's due to the problem of startContainer/endContainer of the range is accidently become a bookmark text node, which is supposed to be fixed by #3574. In this case, the problem could be identified as: When creating list in an empty document, the document initial as:

^<br _moz_dirty=""/> 

After the unavoidable bookmark is created:

<span _fck_bookmark="1" > </span>^<br />

After invoking 'range.enlarge( BLOCK_UNIT )':

<span _fck_bookmark="1" >[ </span><br />]

After invoking 'range.extractContents()', the bookmark <span> will be duplicated into two.

<span _fck_bookmark="1" ></span><span _fck_bookmark="1" ></span><br />

While changes from [3538] eliminate this unwanted result by moving the range out of the bookmark as:

<span _fck_bookmark="1" > </span>[<br />]

But the cons is that it also exclude the bookmark node from been included within the range, which is wrong, so after creating the list, result in :

<span _fck_bookmark="1" > </span><ul><li></li></ul>

After move to bookmark, the selection is outside the newly created list item:

^<ul><li></li></ul>

The correct fix will be about 'range::enlarge( BLOCK_UNIT )', the correct result after it should looks like:

[<span _fck_bookmark="1" > </span><br />] // Include the bookmark nodes on the boundary.

Deducing to a more generic TC, if we're enlarging such a range:

<p><b><i>^text</i></b></p>

• Expected Result:

  <p>[<b><i>text</i></b>]</p> // Start/End the range right before/after the block boundary.
  

• Actual Result:

  <p><b><i>[text]</i></b></p> // Result in an redundant '<b><li></li></b>' later when invoke 'range.extractContent()'
  

Changes in the patch on range.js,domiterator/plugin.js and walker.js is the fix for this.

The function name should be isBookmarkContent at this point, to not make it confusing.

This function has been refacted into more generic function 'walker.bookmark' instead.

Following the test case, in IE the list is not created at first click

No more occurence after the aboving fixes.

[Garry Yao]

Fix logic errors on CKEDITOR.dom.walker.bookmark when ‘contentOnly != true' .

[Martin Kou]

I've just had a discussion with Garry, the logic in the patch should be correct. Only some code simplifications needed:

1. The contentOnly flag in CKEDITOR.dom.walker.bookmark should not be needed - the function can check whether the current node is a text node instead.
2. debugger line in range.js - L1191.
3. L1168, range.js - it's sufficient to just use blockBoundary.contains().

[Garry Yao]

Fixing Martin's reviewing points except: Replying to martinkou:

1. The contentOnly flag in CKEDITOR.dom.walker.bookmark should not be needed - the function can check whether the current node is a text node instead.

As stated in the comments, the 'contentOnly' flag is about forcing the guard/evalutor created on CKEDITOR.dom.walker.bookmark to only check the bookmark inner text node instead of the bookmark <span> it self, so it's an necessary parameter.

[Garry Yao]

Fixed with [3607].