Created attachment 338092 [details] testcase (uses enhanced privileges) See testcase, I get this error when trying to use selectAll() method from nsISelectionController: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISelectionController.selectAll]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///C:/Documents%20and%20Settings/mw/Bureaublad/caret_draw_selectioncontroller.htm :: doe :: line 20" data: no] I don't think that should happen
Created attachment 338093 [details] [diff] [review] patch Removing this code: - //HACKHACKHACK - if (!aNewFocus->GetParent()) - return NS_ERROR_FAILURE; - //END HACKHACKHACK /checking for root frames/content - in nsFrameSelection::TakeFocus seems to make this work. I have no idea if it's safe to remove that, though.
Attachment #338093 - Flags: review?(uriber)
I tracked this code back to where it was submitted, which was on January 1999, with the comment "selection should work now more or less": http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base/src/Attic&command=DIFF&root=/cvsroot&file=nsRangeList.cpp&rev1=1.14&rev2=1.15#7 So not too much help from there :-) I'll have a closer look at this later.
Comment on attachment 338093 [details] [diff] [review] patch I'm generally fine with this, assuming that we really want SelectAll() to select the entire document (including the <head> element!). Note that invoking "Select All" through the UI doesn't trigger this code, but rather just selects all children of the <body> tag. Also, I found at least one piece of code that doesn't handle an entire-document selection well: notice that trying to copy the selection created by SelectAll() (with your fix) doesn't work. The failure is in the following stack trace: #0 nsHTMLCopyEncoder::GetNodeLocation (this=0x1d4ade90, inChild=0x171aad3c, outParent=0xbfffce70, outOffset=0xbfffce6c) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1663 #1 0x11691963 in nsHTMLCopyEncoder::GetPromotedPoint (this=0x1d4ade90, aWhere=nsHTMLCopyEncoder::kStart, aNode=0x171aad3c, aOffset=0, outNode=0xbfffcf30, outOffset=0xbfffcf24, common=0xcdca8c) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1504 #2 0x11692249 in nsHTMLCopyEncoder::PromoteAncestorChain (this=0x1d4ade90, ioNode=0xbfffcfb0, ioStartOffset=0xbfffcfa8, ioEndOffset=0xbfffcfa4) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1412 #3 0x11692aaa in nsHTMLCopyEncoder::PromoteRange (this=0x1d4ade90, inRange=0x1717324c) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1373 #4 0x11695d0c in nsHTMLCopyEncoder::SetSelection (this=0x1d4ade90, aSelection=0x1712aa00) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1228 #5 0x1165cedb in nsCopySupport::HTMLCopy (aSel=0x1712aa00, aDoc=0xcdca00, aClipboardID=1) at /Users/uri/mozilla/central/mozilla/content/base/src/nsCopySupport.cpp:117 #6 0x11403aa4 in PresShell::DoCopy (this=0xb09c00) at /Users/uri/mozilla/central/mozilla/layout/base/nsPresShell.cpp:4242 (More stack frames follow...) but the root cause is probably that nsHTMLCopyEncoder::IsRoot() looks for a "body" element, but not an "html" element (or a document). Please submit a follow-up bug on this if you check in this patch.
(In reply to comment #3) > (From update of attachment 338093 [details] [diff] [review]) > I'm generally fine with this, assuming that we really want SelectAll() to > select the entire document (including the <head> element!). Note that invoking > "Select All" through the UI doesn't trigger this code, but rather just selects > all children of the <body> tag. Yeah, that's what I was thinking too. Maybe the SelectAll() function needs to do that instead? Nobody noticed that it was broken anyway.
It seems to me, the error you described in comment 3, could also happen with some specially crafted script that tinkers with selection. I tried to come up with some code that would trigger the same error as you mentioned in comment 3, but I get a different error, which I filed as bug 454904. Or is it the same error you described in comment 3?
(In reply to comment #4) > Yeah, that's what I was thinking too. Maybe the SelectAll() function needs to > do that instead? Nobody noticed that it was broken anyway. Sounds like a good idea, although seems non-trivial to implement... currently "select all" goes through to different paths depending on whether you're doing it on the entire do or inside a text control (both of these end up in SelectAllChildren), so I think we'll have to replicate the logic for both of them here... Sorry, looking at this mess of code really depresses me. I might take another shot at it when I'm in a better mood.
(In reply to comment #5) > It seems to me, the error you described in comment 3, could also happen with > some specially crafted script that tinkers with selection. > I tried to come up with some code that would trigger the same error as you > mentioned in comment 3, but I get a different error, which I filed as bug > 454904. Or is it the same error you described in comment 3? I'm pretty sure it's not the same error. I'll have a look at that one too.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.