Ticket #676 (closed Bug: fixed)

Opened 7 years ago

Last modified 6 years ago

Form field loses name if moved right after placement

Reported by: jonnes@… Owned by: alfonsoml
Priority: Normal Milestone: FCKeditor 2.6
Component: General Version: FCKeditor 2.4
Keywords: Confirmed IE Review+ Cc:

Description

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

Attachments

676.patch (10.1 KB) - added by alfonsoml 6 years ago.
Proposed SVN patch
676_2.patch (10.7 KB) - added by alfonsoml 6 years ago.
New patch
676_3.patch (842 bytes) - added by alfonsoml 6 years ago.
Reuse the existing FCKDomTools.MoveChildren

Change History

comment:1 Changed 7 years ago by martinkou

  • Reporter changed from martinkou to jonnes@…

comment:2 Changed 7 years ago by martinkou

  • Keywords Confirmed IE added
  • Version set to SVN

The bug seems to occur in IE only.

comment:3 Changed 7 years ago by alfonsoml

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

  • fck_textfield.html

     
    4848                GetE('txtSize').value   = GetAttribute( oActiveEl, 'size' ) ; 
    4949                GetE('txtMax').value    = GetAttribute( oActiveEl, 'maxLength' ) ; 
    5050                GetE('txtType').value   = oActiveEl.type ; 
    51  
    52                 GetE('txtType').disabled = true ; 
    5351        } 
    5452        else 
    5553                oActiveEl = null ; 
     
    7270                return false ; 
    7371        } 
    7472 
     73        // IE doesn't allow easily to change properties of an existing object, so remove the old and create a new one. 
     74        if ( oActiveEl && oEditor.FCKBrowserInfo.IsIE ) 
     75        { 
     76                oActiveEl.parentNode.removeChild( oActiveEl ) ; 
     77                oActiveEl = null ; 
     78        } 
     79 
    7580        if ( !oActiveEl ) 
    7681        { 
    77                 oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ; 
    78                 oActiveEl.type = GetE('txtType').value ; 
     82                // #676, IE doesn't play nice with the name attribute. 
     83                if ( oEditor.FCKBrowserInfo.IsIE ) 
     84                { 
     85                        oActiveEl = oEditor.FCK.EditorDocument.createElement( '<INPUT type="' + GetE('txtType').value + '" name="' + GetE('txtName').value + '">' ) ; 
     86                } 
     87                else 
     88                { 
     89                        oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ; 
     90                } 
    7991                oEditor.FCKUndo.SaveUndoStep() ; 
    8092                oActiveEl = oEditor.FCK.InsertElement( oActiveEl ) ; 
    8193        } 
    8294 
    8395        oActiveEl.name = GetE('txtName').value ; 
     96        oActiveEl.type = GetE('txtType').value ; 
     97 
    8498        SetAttribute( oActiveEl, 'value'        , GetE('txtValue').value ) ; 
    8599        SetAttribute( oActiveEl, 'size'         , GetE('txtSize').value ) ; 
    86100        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.

comment:4 Changed 7 years ago by alfonsoml

  • Status changed from new to assigned
  • Keywords HasPatch added
  • Owner set to alfonsoml
  • Milestone set to FCKeditor 2.6

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

  • common/fck_dialog_common.js

     
    5757        return ( oValue == null ? valueIfNull : oValue ) ; 
    5858} 
    5959 
     60// Utility function to create an element with a name attribute in IE, so it behaves properly when moved around 
     61// It also allows to change the name or other special attributes in an existing node 
     62function CreateNamedElement( oEditor, oOriginal, nodeName, oAttributes ) 
     63{ 
     64        var oNewNode ; 
     65        // IE doesn't allow easily to change properties of an existing object, so remove the old and create a new one. 
     66        var oldNode = null ; 
     67        if ( oOriginal && oEditor.FCKBrowserInfo.IsIE ) 
     68        { 
     69                oldNode = oOriginal ; 
     70                oOriginal = null ; 
     71        } 
     72 
     73        // If the node existed (and it's not IE), then we don't need to do anything here 
     74        if ( oOriginal ) 
     75                return oOriginal ; 
     76 
     77        // #676, IE doesn't play nice with the name attribute. 
     78        if ( oEditor.FCKBrowserInfo.IsIE ) 
     79        { 
     80                var sbHTML = [] ; 
     81                sbHTML.push( '<' + nodeName ) ; 
     82                for( var prop in oAttributes ) 
     83                { 
     84                        sbHTML.push( ' ' + prop + '="' + oAttributes[prop] + '"' ) ; 
     85                } 
     86                sbHTML.push( '>' ) ; 
     87                if ( !oEditor.FCKListsLib.EmptyElements[nodeName.toLowerCase()] ) 
     88                        sbHTML.push( '</' + nodeName + '>' ) ; 
     89 
     90                oNewNode = oEditor.FCK.EditorDocument.createElement( sbHTML.join('') ) ; 
     91                // Check if we are just changing the properties of an existing node: copy its properties 
     92                if ( oldNode ) 
     93                { 
     94                        CopyAttributes( oldNode, oNewNode ) ; 
     95                        MoveContents( oldNode, oNewNode ) ; 
     96                        oldNode.parentNode.removeChild( oldNode ) ; 
     97                        oldNode = null ; 
     98                        // Trick to refresh the selection object and avoid error in FCKDomRange.MoveToSelection due to the removed node 
     99                        var oSel = oEditor.FCK.EditorDocument.selection ; 
     100                        oSel.createRange() ; // Now oSel.type will be 'None' reflecting the real situation 
     101                } 
     102                oNewNode = oEditor.FCK.InsertElement( oNewNode ) ; 
     103        } 
     104        else 
     105        { 
     106                oNewNode = oEditor.FCK.InsertElement( nodeName ) ; 
     107        } 
     108 
     109        return oNewNode ; 
     110} 
     111 
     112// Copy all the attributes from one node to the other, kinda like a clone 
     113function CopyAttributes( oSource, oDest ) 
     114{ 
     115        var aAttributes = oSource.attributes ; 
     116 
     117        for ( var n = 0 ; n < aAttributes.length ; n++ ) 
     118        { 
     119                var oAttribute = aAttributes[n] ; 
     120 
     121                if ( oAttribute.specified ) 
     122                { 
     123                        var sAttName = oAttribute.nodeName ; 
     124                        // We can set the type only once, so do it with the proper value, not copying it. 
     125                        if ( sAttName == 'type' ) 
     126                                continue ; 
     127 
     128                        var sAttValue = oSource.getAttribute( sAttName, 2 ) ; 
     129                        if ( sAttValue == null ) 
     130                                sAttValue = oAttribute.nodeValue ; 
     131 
     132                        oDest.setAttribute( sAttName, sAttValue, 0 ) ;  // 0 : Case Insensitive 
     133                } 
     134        } 
     135        // The style: 
     136        oDest.style.cssText = oSource.style.cssText ; 
     137} 
     138 
     139// Move the contents from one node to the other 
     140function MoveContents( oSource, oDest ) 
     141{ 
     142        while ( oSource.firstChild ) 
     143        { 
     144                var oNode = oSource.removeChild( oSource.firstChild ) ; 
     145                oDest.appendChild( oNode ) ; 
     146        } 
     147} 
     148 
     149 
    60150var KeyIdentifierMap =  
    61151{ 
    62152        End             : 35, 
  • fck_textarea.html

     
    5656function Ok() 
    5757{ 
    5858        oEditor.FCKUndo.SaveUndoStep() ; 
    59          
    60         if ( !oActiveEl ) 
    61         { 
    62                 oActiveEl = oEditor.FCK.InsertElement( 'textarea' ) ; 
    63         } 
    6459 
     60        oActiveEl = CreateNamedElement( oEditor, oActiveEl, 'TEXTAREA', {name: GetE('txtName').value} ) ; 
     61 
    6562        oActiveEl.name = GetE('txtName').value ; 
    6663        SetAttribute( oActiveEl, 'cols', GetE('txtCols').value ) ; 
    6764        SetAttribute( oActiveEl, 'rows', GetE('txtRows').value ) ; 
  • fck_textfield.html

     
    4848                GetE('txtSize').value   = GetAttribute( oActiveEl, 'size' ) ; 
    4949                GetE('txtMax').value    = GetAttribute( oActiveEl, 'maxLength' ) ; 
    5050                GetE('txtType').value   = oActiveEl.type ; 
    51  
    52                 GetE('txtType').disabled = true ; 
    5351        } 
    5452        else 
    5553                oActiveEl = null ; 
     
    7270                return false ; 
    7371        } 
    7472 
    75         if ( !oActiveEl ) 
    76         { 
    77                 oActiveEl = oEditor.FCK.EditorDocument.createElement( 'INPUT' ) ; 
    78                 oActiveEl.type = GetE('txtType').value ; 
    79                 oEditor.FCKUndo.SaveUndoStep() ; 
    80                 oActiveEl = oEditor.FCK.InsertElement( oActiveEl ) ; 
    81         } 
     73        oEditor.FCKUndo.SaveUndoStep() ; 
    8274 
     75        oActiveEl = CreateNamedElement( oEditor, oActiveEl, 'INPUT', {name: GetE('txtName').value, type: GetE('txtType').value } ) ; 
     76 
    8377        oActiveEl.name = GetE('txtName').value ; 
     78        oActiveEl.type = GetE('txtType').value ; 
     79 
    8480        SetAttribute( oActiveEl, 'value'        , GetE('txtValue').value ) ; 
    8581        SetAttribute( oActiveEl, 'size'         , GetE('txtSize').value ) ; 
    8682        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.

Changed 6 years ago by alfonsoml

Proposed SVN patch

comment:5 Changed 6 years ago by alfonsoml

  • Keywords Review? added; SF HasPatch removed
  • Version changed from SVN to FCKeditor 2.4

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

comment:6 Changed 6 years ago by alfonsoml

  • Keywords Review? removed

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.

Changed 6 years ago by alfonsoml

New patch

comment:7 Changed 6 years ago by alfonsoml

  • Keywords Review? added

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.

comment:8 Changed 6 years ago by martinkou

  • Keywords Review+ added; Review? removed

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

comment:9 Changed 6 years ago by alfonsoml

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [1504]

comment:10 Changed 6 years ago by alfonsoml

  • Status changed from closed to reopened
  • Keywords Review+ removed
  • Resolution fixed deleted

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.

Changed 6 years ago by alfonsoml

Reuse the existing FCKDomTools.MoveChildren

comment:11 Changed 6 years ago by alfonsoml

  • Keywords Review? added

comment:12 Changed 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:13 Changed 6 years ago by alfonsoml

  • Status changed from reopened to closed
  • Resolution set to fixed

Fixed with [1601]

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