Opened 17 years ago

Closed 11 years ago

#503 closed Bug (wontfix)

Unordered list paste: doesn't work correctly in IE

Reported by: tba21cn Owned by: Martin Kou
Priority: Normal Milestone:
Component: General Version: FCKeditor 2.4.1
Keywords: Cc:

Description (last modified by Martin Kou)

I checked Ticket #428 IE problems created unordered list from pasted list, it is not exactly the same as this one.

I have the following code in FCKeditor.

<ul>
    <li>unordered list item 1</li>
    <li>unordered list item 2</li>
    <li>unordered list item 3</li>
    <li>unordered list item 4</li>
</ul>
  1. In edit view, high light 2nd and 3rd list items, and do copy.
  2. go to some place without any format, and do paste, I've got:
    <p>
    <li>unordered list item 2</li>
    <li>unordered list item 3</li>
    </p>
    

The <li> lost <ul>.

It doesn't happen in FireFox, it is an IE only issue.

Attachments (3)

503.patch (1.1 KB) - added by Martin Kou 16 years ago.
Proposed patch for fixing #503.
503_2.patch (3.3 KB) - added by Frederico Caldeira Knabben 16 years ago.
503_3.patch (622 bytes) - added by Alfonso Martínez de Lizarrondo 16 years ago.
little adjustment

Download all attachments as: .zip

Change History (26)

comment:1 Changed 17 years ago by Frederico Caldeira Knabben

Milestone: FCKeditor 2.6

Confirmed with IE6. Ok with FF2.

This one seams to be hard to fix, as it is a clipboard issue, usually handled by the browser itself. A deeper analysis is needed.

comment:2 Changed 17 years ago by Frederico Caldeira Knabben

Keywords: Confirmed IE added

comment:3 in reply to:  1 Changed 17 years ago by tba21cn

I made a work around about this, but I don't think it is the "right way" to make it work, but maybe help. It is: If command is "Paste", move cursor back 1 character, check parent name(or ancestor's name) is LI or not, if find 'LI', check this 'LI' has 'UL'/'OL' as parent or not, if not, create a new 'UL' element, insert in front of this 'LI', and append this 'LI' to the 'UL', and check 'UL' 's preceding-sibling 'LI', and append them to the 'UL'(loop).

Replying to fredck:

Confirmed with IE6. Ok with FF2.

This one seams to be hard to fix, as it is a clipboard issue, usually handled by the browser itself. A deeper analysis is needed.

comment:4 Changed 17 years ago by Jon Håvard Gundersen

Any progress on this one? This bug really messes with our documents. Maybe the best thing is to add a check to the processing of the document, because we could get bad html from sources outside of the editor also.

We really don't want listelements to appear outside of a list.

comment:5 Changed 17 years ago by Jon Håvard Gundersen

I've created a suggestion on how to use document processor to move any parentless list elements into a bullet list to prevent messy html.

Any feedback? This solution is maybe a bit heavy on large documents with long lists, but the speed seems ok with my manual testing at least.

List = FCKDocumentProcessor.AppendNew() ;
List.ProcessDocument = function( document )
{
	var LIs = document.getElementsByTagName("LI");
	var listParent = /^(?:UL|OL)$/i;
	
	for(var i = 0; i < LIs.length; i++){
		var el = LIs[i]; 
		var parent = el.parentNode;
		var siblings = new Array();
		
		//Collect all siblings
		do {
			siblings.push(el);
		} while( LIs[i + 1] && el.nextSibling == LIs[i + 1] && (el = LIs[++i]))
		
		var parentName = parent.tagName ? parent.tagName.toLowerCase() : "";
		
		//Locate elements without correct parents
		if(!listParent.test(parentName)){
	
			
			if(parentName == "body") parent = siblings[0].previousSibling;
			
			while(parent.parentNode && parent.parentNode.tagName && parent.parentNode.tagName.toLowerCase() != "body"){
				parent = parent.parentNode; 		
			}
			
			//Create new list
			var list = document.createElement("UL");
			for (var j = 0; j < siblings.length; j++){
				list.appendChild(siblings[j]);
			}
			
			if( parent ) document.body.insertBefore(list, parent);
			else FCK.InsertElement(list);
			
			
		}
	}
}

comment:6 Changed 17 years ago by Frederico Caldeira Knabben

I was wondering if it would not be safer to make those checks in the XHTML processor, as it would guarantee that the output is correct, no matter the (unknown and strange) operation made in the editor. In this way, we would fix any possible problem related to orphan LIs.

Also, maybe the logic for the fix could be a little bit simpler, just checking for the parent node, and moving the node and it siblings when needed. Everything in a single loop.

comment:7 Changed 17 years ago by Jon Håvard Gundersen

You're probably right. It had been fun if you explained a bit how the xhtml processor works. It isn't as easy as the document processor to understand :). I also think that we need to be sure that the xhtml processor is runned immediately after paste and not only when switching view mode or saving.

In general I think that it is a bit confusing for the user when the document could be changed after he saves it. This should happen in real time - thats why I went for the document processor.

comment:8 Changed 17 years ago by Jon Håvard Gundersen

And the point of the nested loop is to prevent checking parent on all siblings :) Note the iterator value is ascending.

comment:9 Changed 16 years ago by Martin Kou

Description: modified (diff)

Tidied up the description a bit.

comment:10 Changed 16 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

Changed 16 years ago by Martin Kou

Attachment: 503.patch added

Proposed patch for fixing #503.

comment:11 Changed 16 years ago by Martin Kou

Keywords: Review? added; Confirmed IE removed

comment:12 in reply to:  7 Changed 16 years ago by Frederico Caldeira Knabben

Replying to jonhg:

You're probably right. It had been fun if you explained a bit how the xhtml processor works. It isn't as easy as the document processor to understand :).

The Document Processor can be used to make changes in the content when loading it. It is also used when calling the InsertHtml function. So your proposed code would work to fix already existing code, or even code pasted through the dialogs.

In the other hand, the pure pasting operations would no be handled with it.

I also think that we need to be sure that the xhtml processor is runned immediately after paste and not only when switching view mode or saving.

In general I think that it is a bit confusing for the user when the document could be changed after he saves it. This should happen in real time - thats why I went for the document processor.

I agree with you. Our problem here is the very limited support browsers give to pasting operations.

About the XHTML processor, it simply navigates through the DOM tree, generating the final output produced by the editor.

For safety, all we can propose is fixing the generated code with the XHTML processor, as proposed by Martin.

comment:13 Changed 16 years ago by Alfonso Martínez de Lizarrondo

Two questions about the code. In

if ( targetNode.lastChild && targetNode.lastChild.nodeName.IEquals( 'ul' ) ) 
	newTarget = targetNode.lastChild ; 
else 
	newTarget = FCKXHtml.XML.createElement( 'ul' ) ; 

Shouldn't the check be done also with 'ol' like it's done at the start of the function?

and then

FCKXHtml._AppendNode( newTarget, htmlNode ) ; 

I've not tried to run it, but I'm not sure that the newTarget element is always right. In one case it takes the last node of an existing element, and in the other it does create a new element.

I should check it myself, but I'm afraid that it might be tricky to get the exact conditions and maybe you already know that this is right.

comment:14 Changed 16 years ago by Martin Kou

The check is only done against <ul> because orphaned <li> items always appear as unordered list items in IE. If we check for <ol> and group those items under an ordered list, we would have converted an unordered list item to an ordered list item, and that would be wrong.

The <li> tag processor is executed in the FCKXHtml._AppendNode() function, which is used to reconstruct an XML tree from an HTML node in a depth-first search manner. The newTarget element is the destination for the <li> node to be appended into in the XML tree, and so it must be either a <ul> at the correct tree depth that we have just inserted into the XML tree, or a new <ul> if a previous <ul> was not found.

Prove of correctness of the newTarget element:

  1. If a <ul> was inserted previously at the level that the <li>'s parent should be, then it must be located at targetNode.lastChild - since we've been doing the tree reconstruction in DFS. So newTarget = targetNode.lastChild is right.
  2. If targetNode.lastChild is not a <ul>, then it is not possible for a suitable <ul> node to exist in the XML tree for inserting the <li>, by considering XML nodes before, at, and after targetNode.lastChild in DFS order:
    1. Suppose there is a <ul> before targetNode.lastChild in DFS order, which is suitable for inserting the <li>, and we inserted it there. Then the <li> would have jumped over at least one node in DFS order to go to the <ul>, which does not make sense. So this cannot happen by contradiction.
    2. targetNode.lastChild itself is not a <ul>, so it's not a suitable newTarget.
    3. There is no <ul> after targetNode.lastChild in DFS order because we've been reconstructing the XML tree in a DFS-like style.
  3. It follows from point 2 that a new <ul> node would be needed if targetNode.lastChild is not a <ul>, so newTarget = FCKXhtml.XML.createElement('ul') is correct.

comment:15 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The thing that always stopped me by passing the proposed patch is the code that hack to look for the last fixed <ul> to append subsequent <li>s. I always thought that it would be possible to process all of them in a row, as soon as the first one is found. It sounded like somehthing that could easily be buggy (like having an orphan <li> after a valid <ul>).

Actually, it seems it is possible to process all of them togheter. I'm attaching a proposal patch for it.

Changed 16 years ago by Frederico Caldeira Knabben

Attachment: 503_2.patch added

comment:16 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review- removed

I've attached another patch for it. It processes all subsequent <li>s in a row. It also fixes the FixBody function, to not fix <li>s in the body, so no extra <p>s are created when pasting the following:

<li>List 1</li>
<li>List 2</li>

It also includes the changelog entry, which is being always missing in the patches.

comment:17 Changed 16 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [1388].

Click here for more info about our SVN system.

Changed 16 years ago by Alfonso Martínez de Lizarrondo

Attachment: 503_3.patch added

little adjustment

comment:18 Changed 16 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review+ removed
Resolution: fixed
Status: closedreopened

Due to #1239 now the compressed project is broken (the IE code seems to work, but it's broken in Firefox). The new patch is a workaround for the problem.

Another question about the original patch, the summary says that this affects IE, but the code has been added in fckxhtml instead of just fckxhtml_ie. Does it matter, or it's better to have it there just in case someone does something weird?

comment:19 Changed 16 years ago by Martin Kou

Keywords: Review+ added; Review? removed

It is possible that someone might input things like this in Firefox:

<p>
    <li>this</li>
    <li>is</li>
    <li>not</li>
    <li>normal</li>
</p>

And the orphaned <li> nodes would appear as an unordered list in WYSIWYG mode.

The patch fixes such possible orphaned list items in Firefox, so there's still some value for the fix to run in Firefox (and other non-IE browsers) as well.

comment:20 Changed 16 years ago by Martin Kou

Resolution: fixed
Status: reopenedclosed

I've committed Alfonso's suggested fix to trunk in [1401].

comment:21 Changed 16 years ago by Jon Håvard Gundersen

Resolution: fixed
Status: closedreopened

I still get this bug on paste,whenever I paste list elements they are inserted as orphans.

It gets fixed on saving or when switching to source mode, but this bug is annoying because the list loses it's formatting and its impossible to end the list with pressing enter twice at the last empty list element.

So maybe xhtml processing should be launched on paste?

comment:22 Changed 16 years ago by Martin Kou

Keywords: Review+ removed
Milestone: FCKeditor 2.6

The problem here is... it's not exactly easy for us to detect what has been pasted in the current 2.6.x code base. Paste detection is scheduled to be done in CKEditor 3.0 (#1427). Perhaps we can get the remaining problems fixed in V3?

comment:23 Changed 11 years ago by Jakub Ś

Resolution: wontfix
Status: reopenedclosed

FCKeditor was retired and is no longer supported. All active development was moved to its successor, CKEditor 3.x, that is a fully mature and far superior product. We recommend you upgrade as soon as possible since this issue is no longer a problem in current code base.

Works in CKEditor.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy