Last Comment Bug 451286 - 'Highlight all' does not seem to work at all across frames
: 'Highlight all' does not seem to work at all across frames
Status: RESOLVED FIXED
: testcase
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.1b1
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on: 495141 493658 495153 645955
Blocks: 263683
  Show dependency treegraph
 
Reported: 2008-08-19 16:37 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-03-28 23:16 PDT (History)
6 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (330 bytes, text/html)
2008-08-19 16:37 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch v1 (2.19 KB, patch)
2008-09-10 12:04 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Patch v2 - includes reftest masquerading as a mochitest (10.02 KB, patch)
2008-09-11 13:49 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review
Patch v3 - for checkin [Checkin: Comment 12] (9.82 KB, patch)
2008-09-25 08:52 PDT, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
Details | Diff | Review
Improve the test (6.35 KB, patch)
2009-07-01 08:52 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-19 16:37:00 PDT
Created attachment 334590 [details]
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] 2008-09-10 06:59:32 PDT
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 [:bz] (Out June 25-July 6) 2008-09-10 07:05:44 PDT
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] 2008-09-10 07:11:53 PDT
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 [:bz] (Out June 25-July 6) 2008-09-10 07:30:54 PDT
> 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] 2008-09-10 12:04:15 PDT
Created attachment 337921 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2008-09-10 12:07:59 PDT
Test by doing the Find thing and doing manual add-to-selection stuff and comparing the resulting images?
Comment 7 Graeme McCutcheon [:graememcc] 2008-09-10 13:03:35 PDT
> 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 [:bz] (Out June 25-July 6) 2008-09-10 13:08:32 PDT
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] 2008-09-11 13:49:24 PDT
Created attachment 338180 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-24 09:10:34 PDT
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] 2008-09-25 08:52:04 PDT
Created attachment 340337 [details] [diff] [review]
Patch v3 - for checkin
[Checkin: Comment 12]

For checkin.

Carrying forward r=mano.
Comment 12 Serge Gautherie (:sgautherie) 2008-09-25 09:38:36 PDT
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] 2009-07-01 08:52:46 PDT
Created attachment 386292 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-07-22 01:18:02 PDT
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] 2009-08-01 11:50:31 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/f7a945c81304

Note You need to log in before you can comment on or make changes to this bug.