Opened 17 years ago

Closed 16 years ago

#676 closed Bug (fixed)

Form field loses name if moved right after placement

Reported by: jonnes@… Owned by: Alfonso Martínez de Lizarrondo
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 (3)

676.patch (10.1 KB) - added by Alfonso Martínez de Lizarrondo 16 years ago.
Proposed SVN patch
676_2.patch (10.7 KB) - added by Alfonso Martínez de Lizarrondo 16 years ago.
New patch
676_3.patch (842 bytes) - added by Alfonso Martínez de Lizarrondo 16 years ago.
Reuse the existing FCKDomTools.MoveChildren

Download all attachments as: .zip

Change History (16)

comment:1 Changed 17 years ago by Martin Kou

Reporter: changed from Martin Kou to jonnes@…

comment:2 Changed 16 years ago by Martin Kou

Keywords: Confirmed IE added
Version: SVN

The bug seems to occur in IE only.

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

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 16 years ago by Alfonso Martínez de Lizarrondo

Keywords: HasPatch added
Milestone: FCKeditor 2.6
Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

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 16 years ago by Alfonso Martínez de Lizarrondo

Attachment: 676.patch added

Proposed SVN patch

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

Keywords: Review? added; SF HasPatch removed
Version: SVNFCKeditor 2.4

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

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

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 16 years ago by Alfonso Martínez de Lizarrondo

Attachment: 676_2.patch added

New patch

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

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 16 years ago by Martin Kou

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 16 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: assignedclosed

Fixed with [1504]

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

Keywords: Review+ removed
Resolution: fixed
Status: closedreopened

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 16 years ago by Alfonso Martínez de Lizarrondo

Attachment: 676_3.patch added

Reuse the existing FCKDomTools.MoveChildren

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

Keywords: Review? added

comment:12 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

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

Resolution: fixed
Status: reopenedclosed

Fixed with [1601]

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