Opened 10 years ago

Closed 10 years ago

#3228 closed Bug (fixed)

List plugin not right on pseudo block

Reported by: Garry Yao Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Cc:

Description

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>
      

Attachments (2)

3228.patch (470 bytes) - added by Garry Yao 10 years ago.
3228_2.patch (9.8 KB) - added by Martin Kou 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 in reply to:  description Changed 10 years ago by 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>
      

Changed 10 years ago by Garry Yao

Attachment: 3228.patch added

comment:2 Changed 10 years ago by Garry Yao

Keywords: Confirmed Review? added
Owner: set to Garry Yao
Status: newassigned

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

comment:3 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

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.

comment:4 in reply to:  3 Changed 10 years ago by Garry Yao

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

comment:5 Changed 10 years ago by Martin Kou

Owner: changed from Garry Yao to Martin Kou
Status: assignednew

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.

comment:6 Changed 10 years ago by 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.

comment:7 Changed 10 years ago by Martin Kou

:|...

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

Changed 10 years ago by Martin Kou

Attachment: 3228_2.patch added

comment:8 in reply to:  6 ; Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed
Status: newassigned

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

comment:9 Changed 10 years ago by Garry Yao

Keywords: Review? removed

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

comment:10 in reply to:  8 Changed 10 years ago by 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.

  1. CKEDITOR.dom.range::checkEndOfBlock bug described as follows: Input:
    <p>abc^<br />def<p>
    
    Process: range.checkEndOfBlock(); Expected Value: false Actual Value: true
  2. range.js - The BR forgiving logic in getCheckStartEndBlockEvalFunction() forgives every BR instead of only the first encountered BR as specified.

Nice catch.

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

  1. range.js - Possible "walker is not defined" JavaScript error at line 1131.
  2. 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.

  1. range.js - Wrong syntax at L1521.

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

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

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

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

comment:11 Changed 10 years ago by 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.

comment:12 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Get fixed at #3304.

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