Closed Bug 451286 Opened 11 years ago Closed 11 years ago

'Highlight all' does not seem to work at all across frames

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: martijn.martijn, Assigned: graememcc)

References

(Depends on 1 open bug)

Details

(Keywords: testcase)

Attachments

(3 files, 2 obsolete files)

Attached file testcase
See testcase, the "Highlight all" button of the Find Toolbar does not seem to work at all across frames.
This never seemed to work.
bz, roc:

Adding some dump statements in the findbar js tells me that all the matches are being correctly found, and reported back to the findbar, which then tries to add the appropriate ranges, which should trigger the painting of the highlight.

Am I perhaps falling foul of the iframe has a separate presshell?
Add the appropriate ranges to where?  To the selection for the iframe's presshell?  Or to the selection for the parent document presshell?
It should be the parent document's presshell. 

As selection controllers aren't available from script (bug 138068), this is the code I'm using:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#437
> It should be the parent document's presshell. 

Yeah, that can't possibly work.  When you step into a subframe you need to switch to the new docshell + selection controller.
Attached patch Patch v1 (obsolete) — Splinter Review
Boris, thanks for your help which set me off on the right path.

This patch ensures the highlighting code uses the right selection controller for the DOM window the match was found in - previously it would always grab the controller for the parent document.

Can't think of a way to test this at the moment: the matching ranges would always be successfully added whether we were using the right controller or not, so that doesn't work as a test.
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Attachment #337921 - Flags: review?(mano)
Blocks: 263683
Test by doing the Find thing and doing manual add-to-selection stuff and comparing the resulting images?
> Test by doing the Find thing and doing manual add-to-selection stuff and
> comparing the resulting images?

A reftest makes sense, but as far as I can see, you can't access browser chrome in reftest files, and thus can't invoke find bar functionality. Or am I completely wrong, or misunderstanding you?

> Am I perhaps falling foul of the iframe has a separate presshell?
For the record, s/has/having. otherwise I'll be cringing every time I see that comment.
I added a utility file to mochitest to do reftest-like things, because I despaired of reftest ever having a way to run it with privileges.  See http://mxr-stage.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/WindowSnapshot.js and http://mxr-stage.mozilla.org/mozilla-central/search?string=WindowSnapshot.js for a couple of tests that use it.
> I added a utility file to mochitest to do reftest-like things, because I
> despaired of reftest ever having a way to run it with privileges.

This is just what I needed! Thanks a lot, Boris.

New patch attached for review.
Attachment #337921 - Attachment is obsolete: true
Attachment #338180 - Flags: review?(mano)
Attachment #337921 - Flags: review?(mano)
Comment on attachment 338180 [details] [diff] [review]
Patch v2 - includes reftest masquerading as a mochitest

>+    function ok(condition, message) {
>+      window.opener.wrappedJSObject.SimpleTest.ok(condition, message);
>+    }
>+
>+    function snapshotWindow(targetWindow) {
>+      return window.opener.wrappedJSObject.snapshotWindow(targetWindow);
>+    }
>+
>+    function compareSnapshots(snapshot1, snapshot2) {
>+      return window.opener.wrappedJSObject.compareSnapshots(snapshot1, snapshot2);
>+    }
>+

I would use lambdas here :)


r=mano.
For checkin.

Carrying forward r=mano.
Attachment #338180 - Attachment is obsolete: true
Attachment #340337 - Flags: review+
Keywords: checkin-needed
Comment on attachment 340337 [details] [diff] [review]
Patch v3 - for checkin
[Checkin: Comment 12]

http://hg.mozilla.org/mozilla-central/rev/c4c680e90de4
Attachment #340337 - Attachment description: Patch v3 - for checkin → Patch v3 - for checkin [Checkin: Comment 12]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Depends on: 493658
Depends on: 495141
Depends on: 495153
Attached patch Improve the testSplinter Review
In bug 451540 comment 15, after a query in the previous comment, I promised to fix up this test to show that unhighlighting works too.

Until now, I failed to deliver. I've tidied up the file a bit too, to make it clearer what's going on.
Attachment #386292 - Flags: review?(mano)
Comment on attachment 386292 [details] [diff] [review]
Improve the test

>diff --git a/toolkit/content/tests/chrome/bug451286_window.xul b/toolkit/content/tests/chrome/bug451286_window.xul
>--- a/toolkit/content/tests/chrome/bug451286_window.xul
>+++ b/toolkit/content/tests/chrome/bug451286_window.xul
>@@ -45,16 +45,18 @@
>         height="600"
>         onload="onLoad();"
>         title="451286 test (also tests bug 493658)">
> 
>   <script type="application/javascript"><![CDATA[
>     const Ci = Components.interfaces;
>     const Cc = Components.classes;
>     const Cr = Components.results;
>+    const SEARCH_TEXT = "text";
>+    const DATAURI = "data:text/html,"+SEARCH_TEXT;
> 

nit: missing spaces around the operator.

>-      //Take snapshots
>+      //Take snapshot of manual highlighting

nit: space missing after //

r=mano
Attachment #386292 - Flags: review?(mano) → review+
Depends on: 645955
You need to log in before you can comment on or make changes to this bug.