Opened 10 years ago

Closed 10 years ago

#3608 closed Bug (fixed)

Create list on empty document error

Reported by: Garry Yao Owned by: Garry Yao
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: Core : Styles Version:
Keywords: Confirmed Review+ Cc:

Description

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.

Attachments (4)

3608.patch (723 bytes) - added by Garry Yao 10 years ago.
3608_2.patch (10.0 KB) - added by Garry Yao 10 years ago.
3608_3.patch (10.0 KB) - added by Garry Yao 10 years ago.
3608_4.patch (9.6 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by Garry Yao

Attachment: 3608.patch added

comment:1 Changed 10 years ago by Garry Yao

Keywords: Review? added
Status: newassigned

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.

comment:2 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Priority: HighNormal
  • 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.

comment:3 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed
Priority: NormalHigh

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.

Changed 10 years ago by Garry Yao

Attachment: 3608_2.patch added

Changed 10 years ago by Garry Yao

Attachment: 3608_3.patch added

comment:4 Changed 10 years ago by Garry Yao

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

comment:5 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

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

Changed 10 years ago by Garry Yao

Attachment: 3608_4.patch added

comment:6 in reply to:  5 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

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.

comment:7 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:8 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3607].

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