Opened 12 years ago

Closed 12 years ago

#1357 closed Bug (fixed)

Doing Select All in Firefox, the parent node of the selection is wrong

Reported by: Alfonso Martínez de Lizarrondo Owned by:
Priority: Normal Milestone: FCKeditor 2.5
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Firefox Cc:

Description

If the Select All command is used in Firefox, the selection object will return the <html> node as the parent of the current selection, but GetSelectedElement() returns <p> not the body

Attachments (1)

sample01.html (3.2 KB) - added by Alfonso Martínez de Lizarrondo 12 years ago.
this file will show the state of the selection while it changes.

Download all attachments as: .zip

Change History (9)

Changed 12 years ago by Alfonso Martínez de Lizarrondo

Attachment: sample01.html added

this file will show the state of the selection while it changes.

comment:1 Changed 12 years ago by Alfonso Martínez de Lizarrondo

Milestone: FCKeditor 2.5

After some sleep I think that the parent node is right, the problem is FCKSelection.GetSelectedElement() because it returns <P> even if there are several paragraphs in the editor (the selected element should be the <body> to have a consistent behavior)

comment:2 Changed 12 years ago by Frederico Caldeira Knabben

From the code comments:

// Retrieves the selected element (if any), just in the case that a single
// element (object like and image or a table) is selected.
FCKSelection.GetSelectedElement = function()
}}

It seams that GetSelectedElement has been created to retrieve "Control" only selections. So, in the reported case, it should really return "null".

comment:3 Changed 12 years ago by Frederico Caldeira Knabben

I meant:

It seams that GetSelectedElement has been created to retrieve "Control" only selections. So, in the reported case, it should really return "null".

comment:4 Changed 12 years ago by Alfonso Martínez de Lizarrondo

On one hand I think that it would be interesting to have that function return the current selected node (no matter if it is an object or not), this patch could fix it that way:

  • fckselection_gecko.js

     
    5757
    5858        if ( selection && selection.anchorNode && selection.anchorNode.nodeType == 1 )
    5959        {
    60                 // This one is good for all browsers, expect Safari Mac.
    61                 selectedElement = selection.anchorNode.childNodes[ selection.anchorOffset ] ;
     60                if ( this.GetType() == 'Control' )
     61                {
     62                        // This one is good for all browsers, expect Safari Mac.
     63                        selectedElement = selection.anchorNode.childNodes[ selection.anchorOffset ] ;
    6264
    63                 // For Safari (Mac only), the anchor node for a control selection is
    64                 // the control itself, which seams logic. FF and Opera use the parent
    65                 // as the anchor node, pointing to the control with the offset.
    66                 // As FF created the selection "standard", Safari would do better by
    67                 // following their steps.
    68                 if ( !selectedElement )
    69                         selectedElement = selection.anchorNode ;
    70                 else if ( selectedElement.nodeType != 1 )
    71                         return null ;
     65                        // For Safari (Mac only), the anchor node for a control selection is
     66                        // the control itself, which seams logic. FF and Opera use the parent
     67                        // as the anchor node, pointing to the control with the offset.
     68                        // As FF created the selection "standard", Safari would do better by
     69                        // following their steps.
     70                        if ( !selectedElement )
     71                                selectedElement = selection.anchorNode ;
     72                        else if ( selectedElement.nodeType != 1 )
     73                                return null ;
     74                }
     75                else
     76                        return selection.anchorNode ;
    7277        }
    7378
    7479        return selectedElement ;

On the other hand, that might not be good for all the situations, then taking out the

	else
		return selection.anchorNode ;

will make the function return something only if there's a control selected.

comment:5 Changed 12 years ago by Frederico Caldeira Knabben

Have you faced any issue in the editor relative to this ticket? I'm considering waiting the 2.5 Beta release to commit it.

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

Now I'm testing my nodePath plugin to show the DOM path to the current element and I saw that a <html> appeared that didn't previously, so this isn't currently a big problem, but it's something that has changed from 2.4.3 and I don't know if other parts might be affected.

comment:7 Changed 12 years ago by Frederico Caldeira Knabben

It seams that it is ok for the core code. For the plugin, I would reccomend using GetSelectedElement if the type is "Control", otherwise using the new FCKSelection.GetBoundaryParentElement( true ), reflecting the toolbar buttons behavior, which consider the start of the selection as the anchor.

Let's apply you proposed fix (without the "else" part) right after the release.

comment:8 Changed 12 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: newclosed

Fixed with [1061]

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