1316513 - Findbar: sometimes "highlight all" button is enabled while the actual highlight feature is disabled (because of hidden regression bug 384458)

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[arni2033]

STR_1:  (close/open findbar)
1. Open url in a new tab:   data:text/html,asdf_1___asdf_2___asdf_3
2. Press Ctrl+F to open findbar. Type "a", enable "Highlight all", type "sdf"   (you'll get "asdf")
3. Close findbar (Escape). Open findbar (Ctrl+F)
4. Press Enter

AR:  "Highlight All" button is enabled, and the actual highlight is disabled
ER:  "Highlight All" button state and highlight state should match.
      For example, both enabled, like it was before (I tested on Nightly 49 (2016-05-26))


STR_2:  (reload)
1. Open url in a new tab:   data:text/html,asdf_1___asdf_2___asdf_3
2. Press Ctrl+F to open findbar. Type "a", enable "Highlight all", type "sdf"   (you'll get "asdf")
3. Reload the page (Ctrl+R)
4. Press Enter to search for "asdf"
5. Press Ctrl+Enter to enable "Highlight All"

AR:  "Highlight All" button is enabled, and the actual highlight is disabled
ER:  "Highlight All" button state and highlight state should match.
      For example, both enabled, like it was before (I tested on Nightly 49 (2016-05-26))

Comment 1

[Hani Yacoub, Desktop QA]

Using the mozregression tool I got to this regression window:

Last good revision: 287ff570b1a18bc84c82309e23163557cb9fae6d
First bad revision: 5febcc03a7299c1fca11af2bf529b8b5be8bb900
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=287ff570b1a18
bc84c82309e23163557cb9fae6d&tochange=5febcc03a7299c1fca11af2bf529b8b5be8bb900

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793.

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

Comment 3

[Mike de Boer [:mikedeboer]]

This is a bug caused primarily by bug 1316515: it changes the _appearance_ of the find toolbar (by unchecking the 'Highlight All' button), but does not _toggle_ the feature.
Noteworthy is that this has been the case ever since Fx 1.0, but never reported before.
We can now implement this properly due to the much more sophisticated highlighter implementation.

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8810078 [details]
Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793.

https://reviewboard.mozilla.org/r/92498/#review92660

::: toolkit/content/tests/chrome/findbar_window.xul:552
(Diff revision 1)
> +      ok(!highlightButton.checked, "testFindWithHighlight - 1316513: Highlight All should be unchecked.");
> +
> +      yield enterStringIntoFindField(searchStr);
> +
> +      highlightButton.click();
> +      ok(highlightButton.checked, "testFindWithHighlight - 1316513: Highlight All should be checked.");

Feels like we should assert that this is the case after the reload as well (with a unique message, please!)

::: toolkit/content/tests/chrome/findbar_window.xul:565
(Diff revision 1)
> +      let promise = ContentTask.spawn(gBrowser, null, function* () {
> +        return new Promise(resolve => {
> +          addEventListener("DOMContentLoaded", function listener() {
> +            removeEventListener("DOMContentLoaded", listener);
> +            resolve();
> +          });
> +        });
> +      });

Can't you just use BTU.browserLoaded(gBrowser.selectedBrowser); ?

::: toolkit/content/tests/chrome/findbar_window.xul:565
(Diff revision 1)
> +        let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
> +        // The other 4 ranges are in a different controller.
> +        Assert.equal(sel.rangeCount, 3, "testFindWithHighlight - 1316513: more highlights, please [1]");
> +      });
> +
> +      let promise = ContentTask.spawn(gBrowser, null, function* () {

Nit: useful variable name please.

::: toolkit/content/tests/chrome/findbar_window.xul:580
(Diff revision 1)
> +      yield ContentTask.spawn(gBrowser, {}, function* () {
> +        let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsISelectionDisplay)
> +                                 .QueryInterface(Ci.nsISelectionController);
> +        let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
> +        // The other 4 ranges are in a different controller.
> +        Assert.equal(sel.rangeCount, 3, "testFindWithHighlight - 1316513: more highlights, please [2]");
>      }

It seems like this pattern could do with being a utility function that gets invoked like:

```
yield expectRangeCount(rangeCount, message);
```

which would also deal with waiting for this to be the case so we could stop having to insert these odd timeouts everywhere.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8810078 [details]
Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793.

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

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8810078 [details]
Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793.

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

Comment 7

[Mike de Boer [:mikedeboer]]

I've addressed all your comments, which were all excellent, btw! It makes the test way shorter, succinct and easier to read.
Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27004183fb64
make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793. r=Gijs

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27004183fb64

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for uplift when you get a chance.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8810078 [details]
Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793.

Approval Request Comment
[Feature/regressing bug #]: bug 384458.
[User impact if declined]: When 'Highlight All' is flipped on, the highlights wouldn't re-appear when searching with the same find-in-page string on the same page after a reload or closing & (re-)opening the findbar. The state of the 'Highlight All' would also be reset erroneously.
[Describe test coverage new/current, TreeHerder]: landed on m-c, added a regression test, which passes.
[Risks and why]: small. As I mentioned in comment 3, this was already largely broken since Fx 1.0(!)
[String/UUID change made/needed]: n/a.

Comment 13

[Gerry Chang [:gchang]]

Hi Brindusa,
could you help verify if this issue is fixed as expected on the latest Nightly build before uplifting to beta/aurora?

Comment 14

[Gerry Chang [:gchang]]

Change NI to Roxana.
Hi Roxana,
Please verify this bug together with comment #12 in bug 1316515.

Comment 15

[roxleitan]

Verified fixed on the  latest Nightly 53.0a1 (Build ID:20161117030212), with pref "findbar.highlightAll" and pref "findbar.modalHighlight" set to True and to False on Windows 10, Ubuntu 16.04 and Mac OS X 10.11.

Noticed that when preferences are set to False, "Highlight All" button is enabled after the page is reloaded , so no need to follow step 5 from STR_2.
Mike, this is intended?

Comment 16

[Mike de Boer [:mikedeboer]]

Yup, means we fixed it! :)

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8810078 [details]
Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793.

Fix a regression related to "highlight all" in search. Beta51+ and Aurora52+. Should be in 51 beta 2.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/adf273962957

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/722e32265340

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1316513 and bug 1316515 for failing test_findbar.xul (+ assertion on debug) and browser_bug537013.js:

Bug 1316513:
https://hg.mozilla.org/releases/mozilla-beta/rev/cee1c1fdbe7b991bdf756f188e81123c2fa8ceb5
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6cdaa21d3df0e9a0009c52f9cf31c2a6f767603

Bug 1316515:
https://hg.mozilla.org/releases/mozilla-beta/rev/8c348d0e715122df9906583cfba66afcce83394f
https://hg.mozilla.org/releases/mozilla-aurora/rev/a99c9599b45835479e6e18da0614b0652d42435a

Pushes with failure:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=2bf05af8c02db8c5a6842d0a5436f2fdf9872fff
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=a99c9599b45835479e6e18da0614b0652d42435a

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=1946115&repo=mozilla-beta

[task 2016-11-20T19:51:16.414947Z] 19:51:16     INFO -  76 INFO TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | testFindWithHighlight 5: t, t, t.
[task 2016-11-20T19:51:16.415324Z] 19:51:16     INFO -  77 INFO TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | testFindWithHighlight: Highlight All should be unchecked.
[task 2016-11-20T19:51:16.415556Z] 19:51:16     INFO -  78 INFO TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | Expected the correct amount of ranges inside the Find selection - 0 == 0
[task 2016-11-20T19:51:16.415916Z] 19:51:16     INFO -  79 INFO TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | testFindWithHighlight: Highlight All should be checked.
[task 2016-11-20T19:51:16.417579Z] 19:51:16     INFO -  80 INFO TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | Expected the correct amount of ranges inside the Find selection - 3 == 3
[task 2016-11-20T19:51:16.417973Z] 19:51:16     INFO -  81 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar.xul | Expected the correct amount of ranges inside the Find selection - 3 == 0
[task 2016-11-20T19:51:16.418333Z] 19:51:16     INFO -  receiveMessage@resource://testing-common/ContentTask.jsm:117:7

[task 2016-11-20T19:51:56.233679Z] 19:51:56     INFO -  2 INFO TEST-START | browser/base/content/test/general/browser_bug537013.js
[task 2016-11-20T19:52:00.331165Z] 19:52:00     INFO -  TEST-INFO | started process screentopng
[task 2016-11-20T19:52:00.643270Z] 19:52:00     INFO -  TEST-INFO | screentopng: exit 0
[task 2016-11-20T19:52:00.644111Z] 19:52:00     INFO -  3 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Set the field correctly! -
[task 2016-11-20T19:52:00.644743Z] 19:52:00     INFO -  4 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | 'TabFindInitialized' event properly dispatched! -
[task 2016-11-20T19:52:00.645319Z] 19:52:00     INFO -  5 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Second tab doesn't show find bar! -
[task 2016-11-20T19:52:00.645425Z] 19:52:00     INFO -  6 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Second tab kept old find value for new initialization! -
[task 2016-11-20T19:52:00.646001Z] 19:52:00     INFO -  7 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Set the field correctly! -
[task 2016-11-20T19:52:00.646560Z] 19:52:00     INFO -  8 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | First tab shows find bar! -
[task 2016-11-20T19:52:00.646658Z] 19:52:00     INFO -  9 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | First tab persists find value! -
[task 2016-11-20T19:52:00.647220Z] 19:52:00     INFO -  10 INFO TEST-PASS | browser/base/content/test/general/browser_bug537013.js | Highlight button state persists! -
[task 2016-11-20T19:52:00.647821Z] 19:52:00     INFO -  11 INFO Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“r†modifiers=“accel,alt†id=“toggleReaderModeâ€" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 776}]
[task 2016-11-20T19:52:00.648424Z] 19:52:00     INFO -  12 INFO Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“i†modifiers=“accel,alt,shift†id=“key_browserToolboxâ€" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 776}]
[task 2016-11-20T19:52:00.649046Z] 19:52:00     INFO -  13 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug537013.js | Highlight never reset! -
[task 2016-11-20T19:52:00.649101Z] 19:52:00     INFO -  Stack trace:
[task 2016-11-20T19:52:00.649399Z] 19:52:00     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/head.js:waitForCondition/interval<:111
[task 2016-11-20T19:52:00.649460Z] 19:52:00     INFO -  Not taking screenshot here: see the one that was previously logged
[task 2016-11-20T19:52:00.649528Z] 19:52:00     INFO -  14 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug537013.js | Highlight button reset! -
[task 2016-11-20T19:52:00.649567Z] 19:52:00     INFO -  Stack trace:
[task 2016-11-20T19:52:00.649629Z] 19:52:00     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug537013.js:continueTests3:88
[task 2016-11-20T19:52:00.649695Z] 19:52:00     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/head.js:waitForCondition/moveOn:126
[task 2016-11-20T19:52:00.649760Z] 19:52:00     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/head.js:waitForCondition/interval<:112

Comment 21

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c58b40f5e8b398c225df819b28314c57181273dd
https://hg.mozilla.org/releases/mozilla-beta/rev/83e1d3c7f75096287834ddea0f4d478ab3032604

Comment 22

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161123030208

Verified as fixed using the latest Firefox Aurora 52.0a2  (having preferences findbar.modalHighlight and findbar.highlightAll true and false) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11. 

Setting status-firefox52 to verified.

Comment 23

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161124073320

Verified on OSes: Windows 10 x64, Ubuntu 15.10 and Mac OS X 10.11. 

Verified as fixed for STR_2 on the latest Firefox Beta 51.0b3 (with config. parameters "findbar.modalHighlight" and "findbar.highlightAll" true and false).

For STR_1, on the latest Firefox Beta 51.0b3, bug is fixed for config. parameters set to False, but for config. parameters set to True the issue is still reproducible.

Mike, should I log a new bug for this scenario on Beta ?

Note: This issue is not reproducible(using STR_1 and STR_2 with parameters true and false) on Nightly 53 and Aurora 52.

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to [e-mail] from comment #23)
> Mike, should I log a new bug for this scenario on Beta ?

No thanks, Roxana, this feature is disabled and not supported on Beta. Thanks for verifying!

Comment 25

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

Too late to fix in 50.1.0 release