Closed
Bug 495141
Opened 16 years ago
Closed 5 years ago
Highlight all of the Findbar does not work when iframe document has body removed
Categories
(Toolkit :: Find Toolbar, defect, P5)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: martijn.martijn, NeedInfo)
References
Details
(Keywords: regression, testcase)
Attachments
(2 files)
See testcase, instead of highlighting all text in the document, you get this error instead:
Error: body is null
Source File: chrome://global/content/bindings/findbar.xml
Line: 943
|
Assignee | |
Comment 1•16 years ago
|
||
Assignee: nobody → martijn.martijn
Attachment #379989 -
Flags: review?(vladimir)
|
|
Assignee | |
Updated•16 years ago
|
Attachment #379989 -
Flags: review?(vladimir)
Attachment #379989 -
Flags: review?(mano)
Attachment #379989 -
Flags: review?(graememcc_firefox)
|
||
Comment 2•16 years ago
|
||
Comment on attachment 379989 [details] [diff] [review]
patch
FWIW, wouldn't say this is a regression from 451286 - this didn't work in 3.0 either.
Personally, I wouldn't have done the snapshot comparison thing for this and 493658 - I probably would have just performed the highlighting and then checked for the correct number of ranges in the main document's find selection. (The test for 451286 had to be written this way as in that case ranges being present did not imply that the highlighting was visible, which is not the problem here). That said, clearly the correct code gets exercised, so I'm fine with it.
And now that I think about it, I need to fix the disabled reftest for bug 263683, to prove that the range existing in the selection implies the highlight is visible!
r=graememcc, but this will still need toolkit peer review before it can land.
Attachment #379989 -
Flags: review?(graememcc_firefox) → review+
|
||
Comment 3•16 years ago
|
||
Comment on attachment 379989 [details] [diff] [review]
patch
>diff -NparwU20 src.48ce4e714782/toolkit/content/tests/chrome/bug451286_window.xul src/toolkit/content/tests/chrome/bug451286_window.xul
>+ gBrowser.loadURI("data:text/html; <long url>")
Might be worth breaking this into its own file at this point, just for the sake of readability :)
>diff -NparwU20 src.48ce4e714782/toolkit/content/widgets/findbar.xml src/toolkit/content/widgets/findbar.xml
> var textFound = false;
>
> for (var i = 0; win.frames && i < win.frames.length; i++) {
> if (this._highlightDoc(aHighlight, aWord, win.frames[i]))
> textFound = true;
> }
>
> var controller = this._getSelectionController(win);
> if (!controller) {
> // Without the selection controller,
> // we are unable to (un)highlight any matches
> return false;
> }
>
> var doc = win.document;
>- if (!doc || !(doc instanceof HTMLDocument))
>+ if (!doc || !(doc instanceof HTMLDocument) || !doc.body)
> return textFound;
Not related to this patch, but why does this function return |false| in the no-selection-controller case, but |textFound| in the no-document case? Seems like it should return |textFound| in both cases. Worth a new bug?
Attachment #379989 -
Flags: review?(mano) → review+
|
|
||
Comment 4•16 years ago
|
||
> Worth a new bug?
Yeah, I've got a couple of other misc nits to fix, I'll catch that and those in another bug.
|
||
Updated•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
|
||
Updated•10 years ago
|
Priority: -- → P5
|
||
Comment 5•5 years ago
|
||
I see that this is a very old bug, but it never got resolved.
I believe that the test case may be invalid now. Should the iframe be empty?
Is that iframe document that has the body removed?
<html>
<head>
<title>Highlight all of the Findbar does not work when iframe document has body removed</title>
</head>
<body>
text
<iframe onload="this.contentDocument.removeChild(this.contentDocument.documentElement)"></iframe>
text
</body>
</html>
Mike, can you give me a quick help or redirect me to someone else? Thanks!
Flags: needinfo?(mdeboer)
|
|
||
Comment 6•5 years ago
|
||
Neil Deakin: "Looks like it works fine now. Just close the bug."
Closing as verified based on emailed information.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•