If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Highlight All doesn't work within iframe

VERIFIED FIXED in Firefox 50

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: atlanto, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla51
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50+ verified, firefox51+ verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
STR
1. Open http://www.ibm.com/support/knowledgecenter/en/SSYKE2_6.0.0/com.ibm.java.security.component.60.doc/security-component/whats_new/security_changes_60/security_whatsnew_sr16.html
 (note that the page contains iframe)
2. Find a word (e.g. "fix")
3. Push "Highlight All"

ER
All occurrences are highlighted

AR
Not highlighted

mozregression says...
INFO: Last good revision: 5e41e5886c0f7e92c8adb338d8c939c0f25807ea
INFO: First bad revision: f962e7e8fb1099c29e3ff816ff0eb0a2c8fff5df
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e41e5886c0f7e92c8adb338d8c939c0f25807ea&tochange=f962e7e8fb1099c29e3ff816ff0eb0a2c8fff5df
INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1281421
(Reporter)

Updated

a year ago
Blocks: 1281421

Comment 1

a year ago
Created attachment 8781972 [details]
index.html (reduced testcase)

Updated

a year ago
Attachment #8781972 - Attachment description: index.html → index.html (reduced testcase)

Comment 2

a year ago
Mike, could you check this regression after your patches in bug 1281421.
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
Ever confirmed: true
Flags: needinfo?(mdeboer)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 50 Branch
(Assignee)

Comment 3

a year ago
This will be mostly fixed by my patch in bug 1282070. However, there is still something I need to do here and that is frame support in the hide() method to clear the highlighted ranges from the selection inside the (i)frames too.
So, bug 1282070 will fix showing the found occurrences, this bug will have to take care of hiding them as well and incorporate a test case so that this doesn't regress anymore.
Depends on: 1282070
Flags: needinfo?(mdeboer)
Tracking 51+ to make sure this is addressed by the patch noted in Comment 3.
tracking-firefox51: ? → +
(Assignee)

Updated

a year ago
Blocks: 1291278
(Assignee)

Updated

a year ago
Duplicate of this bug: 1286660
Recent regression in 50, tracked.
tracking-firefox50: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78337668edec
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED

Comment 9

a year ago
mozreview-review
Comment on attachment 8785247 [details]
Bug 1295759 - make sure selected ranges in iframes are cleared when the findbar is hidden as well. Adds a test to guard against regressions.

https://reviewboard.mozilla.org/r/74526/#review72436

::: toolkit/content/tests/chrome/bug263683_window.xul:171
(Diff revision 1)
> +        function getSelection(docShell) {
> +          let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                   .getInterface(Ci.nsISelectionDisplay)
> +                                   .QueryInterface(Ci.nsISelectionController);
> +          return controller.getSelection(controller.SELECTION_FIND);
> +        }
> +

This appears duplicated. Can you pull this out so it's only defined once?

::: toolkit/content/tests/chrome/bug263683_window.xul:191
(Diff revision 1)
> +        Assert.equal(selection.rangeCount, 1,
> +          "Correctly added a match to the selection type");

The test confirms that there is one selection for the top window and one selection for the iframe, but can you add an assertion to make sure that the findbar is reporting a count of 2?

::: toolkit/modules/Finder.jsm
(Diff revision 1)
> -      this._highlighter.clear();
>        this._highlighter.hide();
> +      this._highlighter.clear();

Can you add a comment here, and below, explaining why the order here is important?

::: toolkit/modules/FinderHighlighter.jsm:373
(Diff revision 1)
>  
>      window = (window || this.finder._getWindow()).top;
>      let dict = this.getForWindow(window);
>  
>      this._clearSelection(this.finder._getSelectionController(window), skipRange);
> -    for (let frame of dict.frames)
> +    for (let frame of dict.frames.keys())

Did this just *not work* before? If that's the case, do we need a specific test to confirm this works and make sure it doesn't regress?
Attachment #8785247 - Flags: review?(jaws) → review+
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8785247 [details]
Bug 1295759 - make sure selected ranges in iframes are cleared when the findbar is hidden as well. Adds a test to guard against regressions.

https://reviewboard.mozilla.org/r/74526/#review72436

> This appears duplicated. Can you pull this out so it's only defined once?

Well, the problem here is that it's inside a ContentTask, which doesn't allow for much code sharing, unless we pass in the function as a string and use `eval`

> The test confirms that there is one selection for the top window and one selection for the iframe, but can you add an assertion to make sure that the findbar is reporting a count of 2?

Will do!

> Can you add a comment here, and below, explaining why the order here is important?

Sure. For posterity: if we clear all the references before we hide the highlights (in both highlighting modes), we simply can't use them to find the ranges we need to clear from the selection.

> Did this just *not work* before? If that's the case, do we need a specific test to confirm this works and make sure it doesn't regress?

This code path was a no-op before, when the references were cleared before the highlighting was hidden. Now it's been corrected in this patch and the test case I added above will fail when it regresses in the future.
(Assignee)

Comment 11

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f3f009bcc76
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1274835d9654
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4011df07a197
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f6d2c29d0c3
make sure selected ranges in iframes are cleared when the findbar is hidden as well. Adds a test to guard against regressions. r=jaws

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f6d2c29d0c3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Verified the bug on Nightly 51.0a1, build id: 20160914030200, on Windows 7, Ubuntu 16.04 and Mac 10.11 and the scenario from bug is not reproducible, but other issue was found when scrolling down on a page with iframe - logged bug 1302967 for this.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Hi Mike, should we uplift this fix to Beta50?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 19

a year ago
Working on a patch for beta, but can't get an OSX build running atm. Checking why, currently. Leaving n-i.
(Assignee)

Comment 20

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7e8ac7a265
(Assignee)

Comment 21

a year ago
Created attachment 8798348 [details] [diff] [review]
Patch for Beta: make sure to add found ranges to the selection controller of the correct window/ frame

Carrying over r=jaws.

Approval Request Comment
[Feature/regressing bug #]: bug 1281421.
[User impact if declined]: User will not see highlights inside iframes when they have Highlight All enabled.
[Describe test coverage new/current, TreeHerder]: 51 and 52 already have this patch and running without this regression present. Tests all pass, including a regression test for this case specifically.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.
Flags: needinfo?(mdeboer)
Attachment #8798348 - Flags: review+
Attachment #8798348 - Flags: approval-mozilla-beta?
Comment on attachment 8798348 [details] [diff] [review]
Patch for Beta: make sure to add found ranges to the selection controller of the correct window/ frame

Fix a regression before it reaches our release audience. Should be in 50 beta 5.
Ritu, I guess you don't mind as you are the one who asked for the uplift.
Attachment #8798348 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b33185943560
status-firefox50: affected → fixed
Flags: qe-verify+
I've managed to reproduce this issue on older Nightly build 2016-08-16, with STR from comment 0.

This is verified on Firefox Beta 50.0b6 (2016-10-10), on the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- macOS 10.12.1 (Sierra)
status-firefox50: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.