1316515 - Findbar: highlight from "Highlight All" button isn't canceled when I delete all text in findbar (because of hidden regression bug 384458)

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[arni2033]

STR_1:
1. Open url in a new tab:   data:text/html,asdf<iframe src="data:text/html,asdf zxcv">
2. Press Ctrl+F to open findbar. Type "a", enable "Highlight all"
3. Press Backspace to clear findbar

AR:  Highlight stays
ER:  No highlight

Comment 2

[arni2033]

Have you tested it? Pay attention to the flags. Pay attention to word "undetectable" in bug 1316516

Comment 3

[Mike de Boer [:mikedeboer]]

Not sure what you mean by 'undetectable', but you've got a good point! There's definitely something fishy going on with toggling Highlight All. I'll make sure to restore it to its good old self.

Comment 4

[arni2033]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Not sure what you mean by 'undetectable'
I meant that it cannot be detected by (experienced) testers on Nightly, because unlike Nightly,
other branches use "findbar.highlightAll" set to false, "findbar.modalHighlight" set to false

> There's definitely something fishy going on with toggling Highlight All.
> I'll make sure to restore it to its good old self.
Please read bug1316516 and its blocking bugs. I would appreciate if you made decision on further plans
in bug1316516 before doing some work, because it's the most rational way. Bug 1316516 assumptions 2,3:
 2) There may be more bugs in good old findbar that I haven't detected, triggered by bug 384458 & Co
 3) Replacing the code to its old (already known) version is easier than messing with new bugs
I assumed that backing out would be a temporary solution (because you'll need to do that every
1.5 month), and creating a reliable separation between old/new code is a long and complicated task.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1316515 - clear the find selection when the findbar input box is cleared.

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

Comment 6

[:Gijs (he/him)]

Comment on attachment 8810077 [details]
Bug 1316515 - clear the find selection when the findbar input box is cleared.

https://reviewboard.mozilla.org/r/92494/#review92658

Tentative r+ but the timeout in the test is yucky.

Also, that test could really do with splitting up into individual tests.

::: toolkit/content/tests/chrome/findbar_window.xul:524
(Diff revision 1)
> +      });
> +
> +      highlightButton.click();
> +      ok(highlightButton.checked, "testFindWithHighlight: Highlight All should be checked.");
> +
> +      yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT * 4 + 20));

Ew. Why do we need this? Can't we wait for something more deterministic instead?

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #6)
> Ew. Why do we need this? Can't we wait for something more deterministic
> instead?

Ahum, yes. We actually have something quite elegant, which I didn't think of using. Pushed to MozReview. Thanks!

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8810077 [details]
Bug 1316515 - clear the find selection when the findbar input box is cleared.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/785cc517d781
clear the find selection when the findbar input box is cleared. r=Gijs

Comment 10

[Gerry Chang [:gchang]]

Track 51+ as bug 1315428 is duplicated of bug 1301941 and bug 1301941 needs this.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/785cc517d781

Comment 12

[Gerry Chang [:gchang]]

Hi Florin,
Can you help find someone to verify this one, bug 1315428 and bug 1301941 are fixed as expected?

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Moving to Brindusa since she is the owner of the new Find Toolbar (bug 384458).

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8810077 [details]
Bug 1316515 - clear the find selection when the findbar input box is cleared.

Approval Request Comment
[Feature/regressing bug #]: bug 384458.
[User impact if declined]: When 'Highlight All' is flipped on and the user removes the current find-in-page string from the findbar's inputbox, the magenta highlights will not be removed or change until the find-in-page string is changed or the findbar is closed.
[Describe test coverage new/current, TreeHerder]: landed on m-c, added a regression test, which passes.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 15

[roxleitan]

Hi Gerry,

Bug 1316515, bug 1301941 and bug 1315428 are verified as fixed using 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.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8810077 [details]
Bug 1316515 - clear the find selection when the findbar input box is cleared.

Fix a regression related to "Highlight All" in search and was verified by QE. Beta51+ and Aurora52+. Should be in 51 beta 2.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9dbda637a860

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6297f6001001

Comment 19

[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 20

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3a479641b2e4af2f4a9f181c599e0abf954077e
https://hg.mozilla.org/releases/mozilla-beta/rev/86e7b22b735135ca9f6c6781703e649e56db1bde

Comment 21

[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 22

[roxleitan]

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

Verified as fixed using the latest Firefox Beta 51.0b3 (having preferences findbar.modalHighlight and findbar.highlightAll true and false) on Windows 10 x64, Ubuntu 15.10 and Mac OS X 10.11. 

Setting status-firefox51 to verified.

Comment 23

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

Too late to fix in 50.1.0 release