Opened 7 years ago

Closed 7 years ago

#9152 closed Bug (fixed)

[IE8] Delete key before list item

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.5
Component: Core : Lists Version: 3.0
Keywords: IE IBM Cc: monahant@…, satya_minnekanti@…

Description

  1. Load the following into editor
    <p>&nbsp;</p>
    <ol>
    	<li>
    		B</li>
    	<li>
    		C</li>
    </ol>
    
  2. Place the cursor at the end of first empty paragraph;
  3. Press DELETE.
  • Actual: The paragraph is deleted but the list items no longer appear as a list (even though li is still displayed on the elements path bar).

It affects IE8 on WinXP only.

Attachments (4)

9152.patch (3.3 KB) - added by Garry Yao 7 years ago.
9152_2.patch (3.4 KB) - added by Garry Yao 7 years ago.
9152_3.patch (4.1 KB) - added by Garry Yao 7 years ago.
9152_4.patch (2.6 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by Garry Yao

Status: newconfirmed
Version: 3.0

comment:2 Changed 7 years ago by Teresa Monahan

Cc: monahant@… added

comment:3 Changed 7 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

Adding myself to cc

comment:4 Changed 7 years ago by Garry Yao

Component: GeneralCore : Lists
Milestone: CKEditor 3.6.5

Let's have the fix dedicated to list only.

Changed 7 years ago by Garry Yao

Attachment: 9152.patch added

comment:5 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

The patch provides handling for DEL key used before a list, to merge with the first list item.

comment:6 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I had a huge surprise when testing the reported tc. That's definitely not expected.

The current behavior we have with Firefox is the right one, partially. There is still an additional case to fix:

  • In an empty paragraph, the paragraph gets deleted and the caret moves to the start of the list (reported tc).
  • In a non-empty paragraph AND the first list item doesn't have a sublist, that list item gets merged into the paragraph.
  • Any other case does nothing.

We need tests for all the above points to be able to proceed accordingly.

comment:7 Changed 7 years ago by Garry Yao

Status: review_failedreview

The review has to be conducted at #9129 instead of here.

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewassigned

comment:9 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed by #9129.

comment:10 Changed 7 years ago by Satya Minnekanti

In IE7, IE9/IE8 Compat View, when we press Delete button empty paragraph gets deleted but first list item becomes a paragraph Pls re-open the ticket and fix this issue

comment:11 Changed 7 years ago by Jakub Ś

Resolution: fixed
Status: closedreopened

@satya comment is correct. TC from comment 10 is still reproducible.

Changed 7 years ago by Garry Yao

Attachment: 9152_2.patch added

comment:12 Changed 7 years ago by Garry Yao

Status: reopenedreview

In short, we need to make range::checkStart(End)OfBlock return TRUE for the following case, ignoring the filler node, in IE:

<p>&nbsp;^</p>
AND
<p>^&nbsp;</p>
Version 0, edited 7 years ago by Garry Yao (next)

comment:13 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Correct me if I'm wrong, but the current tests are totally valid. If anything else needs to be tested, additional tests must be added, instead of changing the current ones to fit the patch needs.

Changed 7 years ago by Garry Yao

Attachment: 9152_3.patch added

comment:14 Changed 7 years ago by Garry Yao

Status: review_failedreview

You're right, previous patch breaks one original TC in gecko

<p><br />^</p> // => start & end of block

And didn't consider one important TC in IE:

<p>foo|&nbsp;</p> // single text node => end of block

The tests are squashed for easier review.

comment:15 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:16 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7616].

comment:17 Changed 7 years ago by Satya Minnekanti

This is not properly fixed. 2nd scenario mentioned by fred in https://dev.ckeditor.com/ticket/9152#comment:6 still not working. Please re-open this ticket

  1. Load the following in to Editor
<p>
	First Non-Empty Paragrapgh</p>
<ol>
	<li>
		first list item with no sub-lists</li>
	<li>
		second list item with no sublists</li>
</ol>

  1. Place cursor at end of non-empty Paragraph & press Delete

Expected Result: First list item gets merged in to Paragraph and Numbered list remains with one list item.

Actual Result: First list item not merged in to the Paragraph, Numbered list removed , but we still see li in elements path bar, cursor stays at start of first list item. If you switch to HTML Source and then back to Rich Text you will see that Numbered list gets changed to Bulleted list.

Changed 7 years ago by Garry Yao

Attachment: 9152_4.patch added

comment:18 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

It's a pity that it's just IE8@XP doesn't behave well for merging the list item, but the patch would expanded the fix to other browsers as well, considering the current level of supported TC.

Updated keystrokes tests at branch t/9252 accordingly.

comment:19 Changed 7 years ago by Garry Yao

Status: reopenedreview

comment:20 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:21 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7620].

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