Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#6946 closed Bug (fixed)

[IE] copy and paste lists from own document leads into invalid lists

Reported by: Syslogic Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.5.3
Component: Core : Parser Version: 3.5
Keywords: IE Cc:

Description

When copying and pasting lists in the CKEditor lists end up malformed in Internet Explorer (7 and 8).


Test setup:

  1. Add code below
    <p>
    	Paragraph before</p>
    <ul>
    	<li>
    		First item</li>
    	<li>
    		Second item</li>
    	<li>
    		Third item</li>
    	<li>
    		Fourth item</li>
    </ul>
    <p>
    	Paragraph after</p>
    
  1. copy the second and the third item and paste it below the last paragraph.

Now the bullets are gone. Visually it looks like one paragraph with between the two items a <br />

  1. Press the Code button. The code definitely doesn't look like what was expected: The items are not in an <ul> tag.
  1. Press the Code button again. The document is changed compared to the contents which was in the CKEditor before toggling the Code view.
  1. Press the Code button again. The code now represents the end result of the paste action.

When pasting a subset of a list, the new pasted data should be wrapped in a <ul> tag in order to create valid code.

Attachments (10)

01 start-situation.png (48.2 KB) - added by Syslogic 7 years ago.
Start situation
02 copy-paste-list-items.png (49.1 KB) - added by Syslogic 7 years ago.
Copy and paste list items
03 generated-code.png (52.8 KB) - added by Syslogic 7 years ago.
The generated code after pasting
04 corrected-after-code-view.png (48.8 KB) - added by Syslogic 7 years ago.
The corrected view after toggling the Code button
05 code-corrected.png (52.0 KB) - added by Syslogic 7 years ago.
The code representing the corrected view
6946.patch (1.3 KB) - added by Garry Yao 7 years ago.
6946_2.patch (3.0 KB) - added by Garry Yao 7 years ago.
6946_3.patch (9.4 KB) - added by Garry Yao 6 years ago.
6946_4.patch (9.5 KB) - added by Garry Yao 6 years ago.
6946_5.patch (9.6 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (24)

Changed 7 years ago by Syslogic

Attachment: 01 start-situation.png added

Start situation

Changed 7 years ago by Syslogic

Copy and paste list items

Changed 7 years ago by Syslogic

Attachment: 03 generated-code.png added

The generated code after pasting

Changed 7 years ago by Syslogic

The corrected view after toggling the Code button

Changed 7 years ago by Syslogic

Attachment: 05 code-corrected.png added

The code representing the corrected view

comment:1 Changed 7 years ago by Garry Yao

Keywords: IE added; IE7 IE8 lists removed
Status: newconfirmed

This's actually an unfixed case of #5626, ticket tc updated with [6299].

Changed 7 years ago by Garry Yao

Attachment: 6946.patch added

comment:2 Changed 7 years ago by Garry Yao

Component: Core : PastingCore : Parser
Owner: set to Garry Yao
Status: confirmedreview

Changed 7 years ago by Garry Yao

Attachment: 6946_2.patch added

comment:3 Changed 7 years ago by Garry Yao

Another patch addressing TCs from #6972.

comment:4 Changed 7 years ago by Alfonso Martínez de Lizarrondo

We should check if this also fixes correctly #6912

comment:5 Changed 6 years ago by Wiktor Walc

Milestone: CKEditor 3.5.2

comment:6 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Put in source:

<dl><dd>a</dd><dt>b</dt></dl>

Toggle to wysiwyg and back to source.
Expected:

<dl>
	<dd>
		a</dd>
	<dt>
		b</dt>
</dl>

Result:

<dl>
	<dd>
		a
		<dl>
			<dt>
				b</dt>
		</dl>
	</dd>
</dl>

Changed 6 years ago by Garry Yao

Attachment: 6946_3.patch added

comment:7 Changed 6 years ago by Garry Yao

Status: review_failedreview

Beyond addressing this particular issue, the patch is a simplification rewrite of the parse's DOM constructing logic, all current TCs regard parsing have to be verified.

comment:8 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Insert:

<ul><ol></ul>

Expected result:

<ul>
	<li>
		<ol>
		</ol>
	</li>
</ul>

Actual result:

<ul>
	<li>
		<ol>
		</ol>
	</li>
	<li>
		<ol>
		</ol>
	</li>
</ul>

Changed 6 years ago by Garry Yao

Attachment: 6946_4.patch added

comment:9 Changed 6 years ago by Garry Yao

Status: review_failedreview

Nice review, not sure why #3828 TC are not enrolled into the parser dt, I'm moving it now with [6546].

comment:10 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Insert:

<li>1</li>
<dt>2</dt>
<dd>3</dd>

Expected:

<ul>
	<li>
		1</li>
</ul>
<dl>
	<dt>
		2</dt>
	<dd>
		3</dd>
</dl>

Actual:

<dl>
	<dt>
		2</dt>
	<dd>
		3</dd>
</dl>

(List item is left out)

Changed 6 years ago by Garry Yao

Attachment: 6946_5.patch added

comment:11 Changed 6 years ago by Garry Yao

Status: review_failedreview

It's quite a tricky case, but we should be able to handle it : two times of invalid fixing.

While the correct expected result is a bit counterintuitive perhaps, but it should be instead, as the dl is invalid inside ul thus are lifted above it.

<dl><dt>2</dt></dl><ul><li>1</li></ul>

comment:12 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:13 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6561], thanks Saar for hq review.

comment:14 Changed 6 years ago by Frederico Caldeira Knabben

I've added a few TCs for the cases found here with [6641].

One of the tests is not passing because the expectation explained on comment:11 is wrong. The one in comment:10 is definitely what we must have.

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