Opened 17 years ago
Closed 17 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)
Change History (9)
Changed 17 years ago by
| Attachment: | sample01.html added |
|---|
comment:1 Changed 17 years ago by
| 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 17 years ago by
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 17 years ago by
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 17 years ago by
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
57 57 58 58 if ( selection && selection.anchorNode && selection.anchorNode.nodeType == 1 ) 59 59 { 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 ] ; 62 64 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 ; 72 77 } 73 78 74 79 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 17 years ago by
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 17 years ago by
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 17 years ago by
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.
this file will show the state of the selection while it changes.