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

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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.

Comment 1

[Graeme McCutcheon [:graememcc]]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Add the appropriate ranges to where?  To the selection for the iframe's presshell?  Or to the selection for the parent document presshell?

Comment 3

[Graeme McCutcheon [:graememcc]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Test by doing the Find thing and doing manual add-to-selection stuff and comparing the resulting images?

Comment 7

[Graeme McCutcheon [:graememcc]]

> 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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2 - includes reftest masquerading as a mochitest

> 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.

Comment 10

[Asaf Romano (gone)]

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.

Comment 11

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v3 - for checkin [Checkin: Comment 12]

For checkin.

Carrying forward r=mano.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 340337 [details] [diff] [review]
Patch v3 - for checkin
[Checkin: Comment 12]

http://hg.mozilla.org/mozilla-central/rev/c4c680e90de4

Comment 13

[Graeme McCutcheon [:graememcc]]

Attachment: Improve the test

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.

Comment 14

[Asaf Romano (gone)]

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

Comment 15

[Graeme McCutcheon [:graememcc]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f7a945c81304