List plugin not right on pseudo block

[Garry Yao]

Reproducing Procedures

1. Open the replace by class example page;
2. Make the following content with selection:

   <p>
   	te^xt<br />text</p>
   

3. Click on list command;
   • Expected Result:

     <ol>
     	<li>
     		text</li>
     </ol>
     <p>
     	<br />
     	text</p>
     

   • Actual Result:

     <ol>
     	<li>
     		text<br />
     		text</li>
     </ol>
     

[Garry Yao]

A more representable TC would be:

Reproducing Procedures

1. Open the replace by class example page;
2. Make the following content with selection:

          <div>
              peseudoBlock
              <p>
                  realBlock
              </p>
          </div>
   

3. Click on list command;
   • Expected Result:

     	<div>
                 <ol>
                     <li>
                         peseudoBlock
                     </li>
                 </ol>
                 <p>
                     realBlock
                 </p>
             </div>
     

   • Actual Result:

      <ol>
     	<li>
     		peseudoBlock
     		<p>
     			realBlock</p>
     	</li>
     </ol>
     

[Garry Yao]

The problem here is list style should also enforce real block.

[Martin Kou]

The real problem is not the list plugin. The real problem is happening at L94 in the domiterator plugin. After the range is told to enlarge to include the future list item contents, it actually enlarged to include the whole paragraph.

[Garry Yao]

Replying to martinkou: Yes, you're right, this one should be fixed with #3051.

[Martin Kou]

Hmm... I've tried #3051's patch but it's causing JavaScript errors instead (maybe because it's outdated vs. the trunk?), and #3051 is currently scheduled for v3.1.

Since list on <br> is a basic feature we shouldn't wait for v3.1 to get this fixed. I'm taking over this ticket.

[Martin Kou]

A simpler test case:

1. Put the following to the editing area in replacebyclass.html.

   <p>
   	te^xt<br />text</p>
   
   

2. Execute the following script.

   e = CKEDITOR.instances.editor1;
   s = e.getSelection();
   r = s.getRanges()[0];
   b = r.createBookmark();      // This is essential to trigger the bug
   r.enlarge(CKEDITOR.ENLARGE_LIST_ITEM_CONTENTS);
   r.select();
   

3. The whole paragraph, instead of just the first line, is selected.

[Martin Kou]

:|...

After fixing CKEDITOR.dom.range::enlarge(), CKEDITOR.dom.range::checkEndOfBlock() is also found to be broken.

[Martin Kou]

Ok, the patch fixes the following things:

1. CKEDITOR.dom.range::enlarge() bug in 6.
2. CKEDITOR.dom.range::checkEndOfBlock bug described as follows: Input:

   <p>abc^<br />def<p>
   

   Process: range.checkEndOfBlock(); Expected Value: false Actual Value: true
3. range.js - The BR forgiving logic in getCheckStartEndBlockEvalFunction() forgives every BR instead of only the first encountered BR as specified.
4. range.js - BR forgiving logic should not be executed in IE since tail BRs are visible in IE.
5. range.js - Possible "walker is not defined" JavaScript error at line 1131.
6. range.js - Possible global namespace pollution at L1132 - L1133.
7. range.js - Wrong syntax at L1521.
8. walker.js - Logical error at L125 - L126 for breakOnFalse == true.
9. Minor coding style fix in domiterator plugin.

The patch also adds 5 test cases for CKEDITOR.dom.range::enlarge( CKEDITOR.ENLARGE_LIST_ITEM_CONTENTS ) and 5 test cases for CKEDITOR.dom.range::checkEndOfBlock().

[Garry Yao]

Since we're introducing changes to the range api at #3304. Please freezing this ticket to make sure there's not any confliction.

[Garry Yao]

Replying to martinkou:

Ok, the patch fixes the following things:

1. CKEDITOR.dom.range::enlarge() bug in 6.

I've had this fix at #3304, adapting to the new range walker API.

2. CKEDITOR.dom.range::checkEndOfBlock bug described as follows: Input:

   <p>abc^<br />def<p>
   

   Process: range.checkEndOfBlock(); Expected Value: false Actual Value: true
3. range.js - The BR forgiving logic in getCheckStartEndBlockEvalFunction() forgives every BR instead of only the first encountered BR as specified.

Nice catch.

4. range.js - BR forgiving logic should not be executed in IE since tail BRs are visible in IE.

I can't tell the same from this ​test.

5. range.js - Possible "walker is not defined" JavaScript error at line 1131.
6. range.js - Possible global namespace pollution at L1132 - L1133.

5.and 6.are same as 1., the bug will vanish as soon as adapting to the new API.

7. range.js - Wrong syntax at L1521.

Actually we're preparing to deprecate the touchedNode function pair.

8. walker.js - Logical error at L125 - L126 for breakOnFalse == true.

It's not valid, 'breakOnFalse' will only return 'false' for the failure of evaluator.

9. Minor coding style fix in domiterator plugin.

The patch also adds 5 test cases for CKEDITOR.dom.range::enlarge( CKEDITOR.ENLARGE_LIST_ITEM_CONTENTS ) and 5 test cases for CKEDITOR.dom.range::checkEndOfBlock().

I agreed with all of your TCs except 'test_checkEndOfBlock11'.

[Martin Kou]

Ok, so I'll need to wait for #3304 for issues 1, 5 and 6. I've just posted some test cases there.

For point 4, tail BRs are indeed visible in IE under editor mode. They're not visible in your sample because your sample doesn't run in editor mode. If you try the following instead you'll see the tail BRs are indeed visible.

1. Open replacebyclass.html in IE6, 7, or 8.
2. If you're using IE6 or 7, make sure you have not enabled Show Blocks mode.
3. Press Shift-Enter at the end of the sample paragraph, you see that a new line is added.
4. Inspect the sample paragraph in IE Developer Toolbar, you see that the new line is due to the tail BR.

Point 8.. I've talked to Fred about this yesterday and he seemed to think otherwise. But from the looks of the walker code (i.e. lastForward() and lastBackward()), it seems you're correct. So it seems to me that checkEndOfBlock() and checkStartOfBlock() should change to lastForward() and lastBackward() after #3304 is committed.

[Garry Yao]

Get fixed at #3304.