Closed Bug 1316515 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

50 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox49 --- unaffected
firefox50 --- wontfix
firefox51 + verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: arni2033, Assigned: mikedeboer)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
Flags: needinfo?(mdeboer)
Blocks: 1316516
Version: unspecified → 50 Branch
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(mdeboer)
Resolution: --- → DUPLICATE
Duplicate of bug: 1301941
Have you tested it? Pay attention to the flags. Pay attention to word "undetectable" in bug 1316516
Status: RESOLVED → REOPENED
Flags: needinfo?(mdeboer)
Resolution: DUPLICATE → ---
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.
Flags: needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: REOPENED → NEW
Status: NEW → ASSIGNED
(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.
Blocks: 1316513
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?
Attachment #8810077 - Flags: review?(gijskruitbosch+bugs) → review+
(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!
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/785cc517d781
clear the find selection when the findbar input box is cleared. r=Gijs
Track 51+ as bug 1315428 is duplicated of bug 1301941 and bug 1301941 needs this.
https://hg.mozilla.org/mozilla-central/rev/785cc517d781
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi Florin,
Can you help find someone to verify this one, bug 1315428 and bug 1301941 are fixed as expected?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Moving to Brindusa since she is the owner of the new Find Toolbar (bug 384458).
Flags: needinfo?(florin.mezei) → needinfo?(brindusa.tot)
Flags: needinfo?(brindusa.tot)
QA Contact: roxana.leitan
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.
Attachment #8810077 - Flags: approval-mozilla-beta?
Attachment #8810077 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
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.
Attachment #8810077 - Flags: approval-mozilla-beta?
Attachment #8810077 - Flags: approval-mozilla-beta+
Attachment #8810077 - Flags: approval-mozilla-aurora?
Attachment #8810077 - Flags: approval-mozilla-aurora+
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
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.
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.
Too late to fix in 50.1.0 release
You need to log in before you can comment on or make changes to this bug.