Unordered list paste: doesn't work correctly in IE

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.

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.

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.

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);
			
			
		}
	}
}

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.

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.

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

Tidied up the description a bit.

Proposed patch for fixing #503.

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.

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.

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.

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.

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.

Fixed with [1388].

Click here for more info about our SVN system.

little adjustment

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?

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.

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