Ticket #6173 (confirmed Bug)

Opened 4 years ago

Last modified 3 years ago

Removing Items from unordered list (WinXP/IE8)

Reported by: tom Owned by:
Priority: Normal Milestone:
Component: General Version: 3.3.2
Keywords: IE Discussion Cc: pomu@…

Description (last modified by tobiasz.cudnik) (diff)

I've got a problem with unordered / numbered lists with IE8 on WindowsXP.

  • Create an unordered list with some items.
  • Now try to get one item to the upper line with the "delete"-key.
  • This will not work.
  • If you try the "backspace"-key on the line that should be added to the item above you got an inconsistent list, because the </li>-Tag of the upper line will not disappear.

This error is not in Firefox3 (WinXP), but IE8 (Windows7)

This is also the case in http://ckeditor.com/demo/

Attachments

TestList.doc (22.5 KB) - added by tom 4 years ago.
Testlist in Word-Format
6173.patch (1.9 KB) - added by paho 3 years ago.
Patch
6173_2.patch (5.7 KB) - added by paho 3 years ago.
Patch
6173_3_rev.patch (6.4 KB) - added by paho 3 years ago.
Patch
6173_4_rev.patch (6.3 KB) - added by paho 3 years ago.
Patch
6173_5.patch (5.3 KB) - added by garry.yao 3 years ago.

Change History

Changed 4 years ago by tom

Testlist in Word-Format

comment:1 Changed 4 years ago by tobiasz.cudnik

  • Status changed from new to confirmed
  • Keywords IE added
  • Description modified (diff)
  • Milestone set to CKEditor 3.5

comment:2 Changed 4 years ago by tom

This Bug is only with IE8 on WindowsXP!

IE8 on Windows7 works fine.

comment:3 Changed 3 years ago by paho

  • Status changed from confirmed to assigned
  • Owner set to paho

comment:4 Changed 3 years ago by fredck

  • Milestone changed from CKEditor 3.5 to CKeditor 3.4.3

Changed 3 years ago by paho

Patch

comment:5 Changed 3 years ago by paho

  • Status changed from assigned to review

comment:6 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

This patch is our first "delete" key handling logic, and looks really good. I'm not trying to make things complicated but while we should consider the following thing:

  1. Apply it for other browsers as well? e.g. Firefox generally works well while stuck will the following case on "del":
    <ol>
    	<li>
    		<p>
    			item1^</p>
    	</li>
    	<li>
    		item2</li>
    </ol>
    
  2. The merging result after "del" in the above list should be the following instead:
    <ol>
    	<li>
    		<p>
    			item1item2</p>
    	</li>
    </ol>
    
  3. Nested list should be considered as well, generally "del" and "backspace" merge the first list item of sub list, then remove the list if empty;

Changed 3 years ago by paho

Patch

comment:7 Changed 3 years ago by paho

  • Status changed from review_failed to review

comment:8 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

Looks good, fails for the following cases on "Backspace":

<ul>
	<li><p>paragraph1</p>
		<p>paragraph2</p>
		<ul>
			<li>^inner item1</li>
		</ul>
	</li>
</ul>

comment:9 Changed 3 years ago by garry.yao

And the following points:

  1. Cache intermediate element when possible (there's no releaser optimization there);
  2. Undo/Redo should work as well;
  3. innerPrevious/innerNext must be well checked to be valid for reception new children.

comment:10 Changed 3 years ago by tobiasz.cudnik

I would also add that those long lines could be splitted into multiline statements for a better readability.

Changed 3 years ago by paho

Patch

comment:11 Changed 3 years ago by paho

  • Status changed from review_failed to review

comment:12 Changed 3 years ago by pomu0325

  • Cc pomu@… added

comment:13 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

Have the following content and selection press "Backspace":

<ol>
	<li>
		<p>
			item1</p>
		<ol>
			<li>
				<h1>
					^item21</h1>
				<h1>
					item22</h1>
			</li>
		</ol>
	</li>
</ol>

Actual Result:

<ol>
	<li>
		<p>
			item1</p>
		<h1>
			item22</h1>
	</li>
</ol>

Expected Result:

<ol>
	<li>
		<p>
			item1item21</p>
		<ol>
			<li>
				<h1>
					item22</h1>
			</li>
		</ol>
	</li>
</ol>

Changed 3 years ago by paho

Patch

comment:14 Changed 3 years ago by paho

  • Status changed from review_failed to review

comment:15 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

There's still lots of problems with the latest patch, I'm now taking over this ticket.

Changed 3 years ago by garry.yao

comment:16 Changed 3 years ago by garry.yao

  • Owner changed from paho to garry.yao
  • Status changed from review_failed to review

comment:17 Changed 3 years ago by tobiasz.cudnik

  • Status changed from review to review_failed

I have two points for the latest patch:

  1. Following TC, already mentioned in comment 13 - IE8, use the content below, hit Backspace, nothing happens:
    <ol>
    	<li>
    		<p>
    			item1</p>
    		<ol>
    			<li>
    				<h1>
    					^item21</h1>
    				<h1>
    					item22</h1>
    			</li>
    		</ol>
    	</li>
    </ol>
    
  2. I think all logic common to every instance of the editor shouldn't be placed inside an init function, as it's duplicated for every instance. This includes:
  • mergeListItems
  • doDelete
  • keydown listener

comment:18 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review
  1. The TC WFM by having the nested list outdented, we could discuss though whether it's a good design.
  2. Those are closures scoped to the editor by design.

comment:19 Changed 3 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

It seems i've been having some cache issues, now i'm getting proper results with attached TC. I agree with the outdent approach for nested lists.

As it goes about closures, we don't need them there, the editor variable can be simply passed as a parameter, which would result in a reduced memory consumption in case of many editor instances.

comment:20 Changed 3 years ago by fredck

  • Status changed from review_passed to new
  • Owner garry.yao deleted
  • Milestone CKEditor 3.4.3 deleted

Ok... I got a complete overview of the patch proposal. It's all interesting and I'm sure some good time has been spent on it. I'm even sorry to say that, but I think the approach to handle this situation was wrong. Let me expose some reasons:

  1. The ticket is related to an issue limited to IE8+XP. Considering that it works well with Win7, we could even expect it to work better on XP in the future (is MS still supporting XP?... anyway). In any case, it's a specific tc on a specific browser. Considering the number of other issues we have to get fixed, this would have less priority (it was a mistake to target it initially) and in any case the amount of code would not justify the fix.
  1. Garry came with a series of "edge cases". Of course, I would love to fix every single aspect of the editor behavior, but we must keep the editor size small. It's hard to justify the patch size because of the cases.
  1. Then again... the cases didn't come from the "real world". I mean, people out there opening tickets for them. It makes it even harder to justify the fix and its size.
  1. The solution proposed goes outside the ticket TC, being much broader. In fact it adds an important new feature into the editor, which should be handled in a dedicated ticket and even have an enlarged discussion to understand if we should introduce it now, and how to do that (avoiding many on the points in this list).

Other things to consider:

  1. The editor is already being considered huge on size. We must be aware to not bloat its code.
  1. We'll always have situations where to add a lot of code in the editor. We must be smart to decide when it's worth, and when it's not.
  1. We didn't leave the option to the end developer to decide whether to use or not this fix.

To conclude, let's have this patch remembered, so it could be part of a the potential keyboard handler we could work on the future. But considering all the above, we should not proceed with it right now.

comment:21 Changed 3 years ago by garry.yao

  • Status changed from new to confirmed
  • Keywords Discussion added
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy