Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Toolkit
Find Toolbar
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: graememcc)

Tracking

(Depends on: 1 bug, {testcase})

Trunk
mozilla1.9.1b1
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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

9 years ago
Add the appropriate ranges to where?  To the selection for the iframe's presshell?  Or to the selection for the parent document presshell?
(Assignee)

Comment 3

9 years ago
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

9 years ago
> 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.
(Assignee)

Comment 5

9 years ago
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.
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Attachment #337921 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
Blocks: 263683

Comment 6

9 years ago
Test by doing the Find thing and doing manual add-to-selection stuff and comparing the resulting images?
(Assignee)

Comment 7

9 years ago
> 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

9 years ago
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.
(Assignee)

Comment 9

9 years ago
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.
Attachment #337921 - Attachment is obsolete: true
Attachment #338180 - Flags: review?(mano)
Attachment #337921 - Flags: review?(mano)
Attachment #338180 - Flags: review?(mano) → review+
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.
(Assignee)

Comment 11

9 years ago
Created attachment 340337 [details] [diff] [review]
Patch v3 - for checkin
[Checkin: Comment 12]

For checkin.

Carrying forward r=mano.
Attachment #338180 - Attachment is obsolete: true
Attachment #340337 - Flags: review+
(Assignee)

Updated

9 years ago
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
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1

Updated

8 years ago
Depends on: 493658
(Reporter)

Updated

8 years ago
Depends on: 495141
(Reporter)

Updated

8 years ago
Depends on: 495153
(Assignee)

Comment 13

8 years ago
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.
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+
(Assignee)

Comment 15

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/f7a945c81304
Depends on: 645955
You need to log in before you can comment on or make changes to this bug.