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

VERIFIED FIXED in Firefox 51

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: arni2033, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla53
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox49 unaffected, firefox50 wontfix, firefox51 verified, firefox52 verified, firefox53 verified)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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))
Flags: needinfo?(mdeboer)
(Reporter)

Updated

2 years ago
Blocks: 1316516
Blocks: 1291278
(Reporter)

Updated

2 years ago
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected

Comment 1

2 years ago
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
Blocks: 1291284
Keywords: regressionwindow-wanted
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
(Assignee)

Updated

2 years ago
Blocks: 253793
(Assignee)

Updated

2 years ago
Depends on: 1316515
(Assignee)

Comment 3

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

2 years ago
mozreview-review
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.
Attachment #8810078 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

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

2 years ago
Pushed by mdeboer@mozilla.com:
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
(Assignee)

Updated

2 years ago
Blocks: 1316514
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1316514

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27004183fb64
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please nominate this for uplift when you get a chance.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 12

2 years ago
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.
Flags: needinfo?(mdeboer)
Attachment #8810078 - Flags: approval-mozilla-beta?
Attachment #8810078 - Flags: approval-mozilla-aurora?
Hi Brindusa,
could you help verify if this issue is fixed as expected on the latest Nightly build before uplifting to beta/aurora?
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
Change NI to Roxana.
Hi Roxana,
Please verify this bug together with comment #12 in bug 1316515.
Flags: needinfo?(brindusa.tot) → needinfo?(roxana.leitan)
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?
Flags: needinfo?(roxana.leitan)
Flags: needinfo?(mdeboer)
(Assignee)

Comment 16

2 years ago
Yup, means we fixed it! :)
Flags: needinfo?(mdeboer)
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
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.
Attachment #8810078 - Flags: approval-mozilla-beta?
Attachment #8810078 - Flags: approval-mozilla-beta+
Attachment #8810078 - Flags: approval-mozilla-aurora?
Attachment #8810078 - Flags: approval-mozilla-aurora+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/adf273962957
status-firefox52: affected → fixed
Flags: in-testsuite+

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/722e32265340
status-firefox51: affected → fixed
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
status-firefox51: fixed → affected
status-firefox52: fixed → affected
Flags: needinfo?(mdeboer)
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.
status-firefox52: fixed → verified
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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 24

2 years ago
(In reply to roxana.leitan@softvision.ro 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!
Flags: needinfo?(mdeboer)
status-firefox51: fixed → verified
Too late to fix in 50.1.0 release
status-firefox50: affected → wontfix
You need to log in before you can comment on or make changes to this bug.