1295759 - Highlight All doesn't work within iframe

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[atlanto]

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

Comment 1

[Loic]

Attachment: index.html (reduced testcase)

Comment 2

[Loic]

Mike, could you check this regression after your patches in bug 1281421.

Comment 3

[Mike de Boer [:mikedeboer]]

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.

Comment 4

[Marcia Knous [:marcia]]

Tracking 51+ to make sure this is addressed by the patch noted in Comment 3.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Recent regression in 50, tracked.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1295759 - make sure selected ranges in iframes are cleared when the findbar is hidden as well. Adds a test to guard against regressions.

Review commit: https://reviewboard.mozilla.org/r/74526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74526/

Comment 8

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78337668edec

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 10

[Mike de Boer [:mikedeboer]]

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.

Comment 11

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f3f009bcc76

Comment 12

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1274835d9654

Comment 13

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4011df07a197

Comment 14

[Mike de Boer [:mikedeboer]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74526/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f6d2c29d0c3

Comment 17

[Brindusa Tot, Desktop QA]

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.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mike, should we uplift this fix to Beta50?

Comment 19

[Mike de Boer [:mikedeboer]]

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

Comment 20

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7e8ac7a265

Comment 21

[Mike de Boer [:mikedeboer]]

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

Comment 22

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b33185943560

Comment 24

[Ciprian Georgiu, Desktop QA]

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)