Form field loses name if moved right after placement

[jonnes@…]

If I add an element with a filled name property and I move it right after submitting, the entered name property is lost. The bug doesn't occur if I save, or show the code first. The bug occurs for form fields e.g. a textfield. This bug also occurs on the demo site.

Maybe a related bug is the name of a link that isn't saved in the link.

---

Moved from SF:
http://sourceforge.net/tracker/index.php?func=detail&aid=1277342&group_id=75348&atid=543653

[martinkou]

The bug seems to occur in IE only.

[alfonsoml]

This can be fixed by creating the input element with the name attribute included:

fck_textfield.html

@@ -48 +48 @@
                 GetE('txtSize').value   = GetAttribute( oActiveEl, 'size' ) ;
                 GetE('txtMax').value    = GetAttribute( oActiveEl, 'maxLength' ) ;
                 GetE('txtType').value   = oActiveEl.type ;
-
-                GetE('txtType').disabled = true ;
         }
         else
                 oActiveEl = null ;
@@ -72 +70 @@
                 return false ;
         }
 
+        // IE doesn't allow easily to change properties of an existing object, so remove the old and create a new one.
+        if ( oActiveEl && oEditor.FCKBrowserInfo.IsIE )
+        {
+                oActiveEl.parentNode.removeChild( oActiveEl ) ;
+                oActiveEl = null ;
+        }
+
         if ( !oActiveEl )
         {
-                oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ;
-                oActiveEl.type = GetE('txtType').value ;
+                // #676, IE doesn't play nice with the name attribute.
+                if ( oEditor.FCKBrowserInfo.IsIE )
+                {
+                        oActiveEl = oEditor.FCK.EditorDocument.createElement( '<INPUT type="' + GetE('txtType').value + '" name="' + GetE('txtName').value + '">' ) ;
+                }
+                else
+                {
+                        oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ;
+                }
                 oEditor.FCKUndo.SaveUndoStep() ;
                 oActiveEl = oEditor.FCK.InsertElement( oActiveEl ) ;
         }
 
         oActiveEl.name = GetE('txtName').value ;
+        oActiveEl.type = GetE('txtType').value ;
+
         SetAttribute( oActiveEl, 'value'        , GetE('txtValue').value ) ;
         SetAttribute( oActiveEl, 'size'         , GetE('txtSize').value ) ;
         SetAttribute( oActiveEl, 'maxlength', GetE('txtMax').value ) ;

If we care only about the moment when it's created then it enough to use just the

		// #676, IE doesn't play nice with the name attribute.
		if ( oEditor.FCKBrowserInfo.IsIE )
		{
			oActiveEl = oEditor.FCK.EditorDocument.createElement( '<INPUT type="' + GetE('txtType').value + '" name="' + GetE('txtName').value + '">' ) ;
		}
		else
		{
			oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ;
		}

part, but the fact is that after the element has been created, its name is edited and then moved, the name will revert back to its previous value, so the solution seems to be to recreate the element for IE everytime that it's edited.

The drawback is that any other properly not handled in this dialog will be lost, but on the other side it allows to change the type property after the element has been created (#738)

But I'm thinking now that instead of just destroying the old element we can hold it just a little, and then copy every property except the ones that the dialog allows editing, so unknown properties (for the dialog, like Class, Id or Style) will remain and we keep the bugfix.

[alfonsoml]

That late idea seems to work, and this code fixes the problems for text inputs and textareas:

common/fck_dialog_common.js

@@ -57 +57 @@
         return ( oValue == null ? valueIfNull : oValue ) ;
 }
 
+// Utility function to create an element with a name attribute in IE, so it behaves properly when moved around
+// It also allows to change the name or other special attributes in an existing node
+function CreateNamedElement( oEditor, oOriginal, nodeName, oAttributes )
+{
+        var oNewNode ;
+        // IE doesn't allow easily to change properties of an existing object, so remove the old and create a new one.
+        var oldNode = null ;
+        if ( oOriginal && oEditor.FCKBrowserInfo.IsIE )
+        {
+                oldNode = oOriginal ;
+                oOriginal = null ;
+        }
+
+        // If the node existed (and it's not IE), then we don't need to do anything here
+        if ( oOriginal )
+                return oOriginal ;
+
+        // #676, IE doesn't play nice with the name attribute.
+        if ( oEditor.FCKBrowserInfo.IsIE )
+        {
+                var sbHTML = [] ;
+                sbHTML.push( '<' + nodeName ) ;
+                for( var prop in oAttributes )
+                {
+                        sbHTML.push( ' ' + prop + '="' + oAttributes[prop] + '"' ) ;
+                }
+                sbHTML.push( '>' ) ;
+                if ( !oEditor.FCKListsLib.EmptyElements[nodeName.toLowerCase()] )
+                        sbHTML.push( '</' + nodeName + '>' ) ;
+
+                oNewNode = oEditor.FCK.EditorDocument.createElement( sbHTML.join('') ) ;
+                // Check if we are just changing the properties of an existing node: copy its properties
+                if ( oldNode )
+                {
+                        CopyAttributes( oldNode, oNewNode ) ;
+                        MoveContents( oldNode, oNewNode ) ;
+                        oldNode.parentNode.removeChild( oldNode ) ;
+                        oldNode = null ;
+                        // Trick to refresh the selection object and avoid error in FCKDomRange.MoveToSelection due to the removed node
+                        var oSel = oEditor.FCK.EditorDocument.selection ;
+                        oSel.createRange() ; // Now oSel.type will be 'None' reflecting the real situation
+                }
+                oNewNode = oEditor.FCK.InsertElement( oNewNode ) ;
+        }
+        else
+        {
+                oNewNode = oEditor.FCK.InsertElement( nodeName ) ;
+        }
+
+        return oNewNode ;
+}
+
+// Copy all the attributes from one node to the other, kinda like a clone
+function CopyAttributes( oSource, oDest )
+{
+        var aAttributes = oSource.attributes ;
+
+        for ( var n = 0 ; n < aAttributes.length ; n++ )
+        {
+                var oAttribute = aAttributes[n] ;
+
+                if ( oAttribute.specified )
+                {
+                        var sAttName = oAttribute.nodeName ;
+                        // We can set the type only once, so do it with the proper value, not copying it.
+                        if ( sAttName == 'type' )
+                                continue ;
+
+                        var sAttValue = oSource.getAttribute( sAttName, 2 ) ;
+                        if ( sAttValue == null )
+                                sAttValue = oAttribute.nodeValue ;
+
+                        oDest.setAttribute( sAttName, sAttValue, 0 ) ;  // 0 : Case Insensitive
+                }
+        }
+        // The style:
+        oDest.style.cssText = oSource.style.cssText ;
+}
+
+// Move the contents from one node to the other
+function MoveContents( oSource, oDest )
+{
+        while ( oSource.firstChild )
+        {
+                var oNode = oSource.removeChild( oSource.firstChild ) ;
+                oDest.appendChild( oNode ) ;
+        }
+}
+
+
 var KeyIdentifierMap = 
 {
         End             : 35,

fck_textarea.html

@@ -56 +56 @@
 function Ok()
 {
         oEditor.FCKUndo.SaveUndoStep() ;
-        
-        if ( !oActiveEl )
-        {
-                oActiveEl = oEditor.FCK.InsertElement( 'textarea' ) ;
-        }
 
+        oActiveEl = CreateNamedElement( oEditor, oActiveEl, 'TEXTAREA', {name: GetE('txtName').value} ) ;
+
         oActiveEl.name = GetE('txtName').value ;
         SetAttribute( oActiveEl, 'cols', GetE('txtCols').value ) ;
         SetAttribute( oActiveEl, 'rows', GetE('txtRows').value ) ;

fck_textfield.html

@@ -48 +48 @@
                 GetE('txtSize').value   = GetAttribute( oActiveEl, 'size' ) ;
                 GetE('txtMax').value    = GetAttribute( oActiveEl, 'maxLength' ) ;
                 GetE('txtType').value   = oActiveEl.type ;
-
-                GetE('txtType').disabled = true ;
         }
         else
                 oActiveEl = null ;
@@ -72 +70 @@
                 return false ;
         }
 
-        if ( !oActiveEl )
-        {
-                oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ;
-                oActiveEl.type = GetE('txtType').value ;
-                oEditor.FCKUndo.SaveUndoStep() ;
-                oActiveEl = oEditor.FCK.InsertElement( oActiveEl ) ;
-        }
+        oEditor.FCKUndo.SaveUndoStep() ;
 
+        oActiveEl = CreateNamedElement( oEditor, oActiveEl, 'INPUT', {name: GetE('txtName').value, type: GetE('txtType').value } ) ;
+
         oActiveEl.name = GetE('txtName').value ;
+        oActiveEl.type = GetE('txtType').value ;
+
         SetAttribute( oActiveEl, 'value'        , GetE('txtValue').value ) ;
         SetAttribute( oActiveEl, 'size'         , GetE('txtSize').value ) ;
         SetAttribute( oActiveEl, 'maxlength', GetE('txtMax').value ) ;

The code for the rest of the elements can be written just as soon as this can land, it's just a matter of doing the same changes in their dialogs.

[alfonsoml]

Proposed SVN patch

[alfonsoml]

This patch takes care of all the form elements, the code also takes care of #738

[alfonsoml]

I must apply the ideas from #738 to create a new element only when it's really needed and to try to select better the newly created element.

[alfonsoml]

New patch

[alfonsoml]

I've updated the patch so now it recreates the element only if one of the special attributes has changed, and after closing the dialog the element is properly selected, not a text selection like it did happen previously. Both ideas from #738.

[martinkou]

The method used makes sense and it worked great in my tests. So I'm giving it a Review+.

[alfonsoml]

Fixed with [1504]

[alfonsoml]

I've realized now that the MoveContents function added to fck_dialog_common.js is the same idea than FCKDomTools.MoveChildren

I think that it would be better to use the FCKDomTools.MoveChildren in order to avoid duplication (I thought about adding all the functionality to FCKDomTools, but as it isn't used in the core, then it would increase the base file size without any special reason)

I'll prepare a new patch.

[alfonsoml]

Reuse the existing FCKDomTools.MoveChildren

[alfonsoml]

Fixed with [1601]