STR_1: (new tab) 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. Open url in a new tab: data:text/html,asdf_1___asdf_2___asdf_3 4. Press Ctrl+F to open findbar with prefilled value "asdf", press Enter 5. Press Ctrl+Enter to enable "Highlight All", press Enter AR: "Highlight All" button state and highlight state (enabled/disabled) are different ER: "Highlight All" button state and highlight state should match 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 3 times to search for "asdf" 5. Press Ctrl+Enter to enable "Highlight All" AR: "Highlight All" button state and highlight state (enabled/disabled) are different ER: "Highlight All" button state and highlight state should match

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

I didn't think the new find bar was shipping in 50 by default.

Nevermind, I see what you mean now.

This should also be fixed when bug 1316513 lands.

(Removing regression keyword because bug 1316513 has it)

Sorry, shouldn't have removed the keyword. Instead, I'm making the tracking flags mostly reflect the dupe.

Hello, Roxana I filed this bug separately, because regression range was different from bug 1316513. I saw you verified bug 1316513, could you please check that this bug is really fixed, at least case "both prefs disabled"? I have very bad internet connection now. Thanks.

Hello arni2033, Verified on OSes: Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11. Verified on versions: FF latest Nightly 53.0a1 (Build ID: 20161211030209) FF latest Aurora 52.0a2 (Build ID: 20161212004004) FF latest Beta 51.0b6 (Build ID: 20161201172143) Verified as fixed for STR_2 with the above mentioned OSes, FF versions and config.parameters "findbar.modalHighlight" and "findbar.highlightAll" set to False, but for STR_1, the bug is reproducible on FF latest Nightly 53 and FF Aurora 52. Verified as fixed for STR_1 and STR_2 with the above mentioned OSes, FF versions and config.parameters "findbar.modalHighlight" and "findbar.highlightAll" set to True. Note: For STR_1 with config.parameters set to True, the issue is still reproducible on FF Beta , but as per comment 23 and comment 24 from bug 1316513, the feature is disabled and not supported on Beta. I will reopen the bug.

Roxana, it's not clear to me what the remaining issue on 52 is (modalHighlight and highlightAll are only set to true on nightly).

Hi Julien, On FF latest Aurora 52.0a2 (Build ID: 20161221004017), with config.parameters "findbar.modalHighlight" and "findbar.highlightAll" set to False, the issue is still reproducible using the STR_1. The issue is verified as fixed on FF latest Beta 51.0b9 with config.parameters set to false using STR_1 and STR_2.

Mike, from comment 11, it sounds like there's still an issue on 52 here in the configuration we intend to ship it with. Can you please take a look?

I see the regression now - it's subtly different. Patch should be easy & simple, meaning uplift won't be a hassle.

Comment on attachment 8825379 [details] Bug 1316514 - if we attempt to make the 'Highlight All' toggle a global setting, then we need to set the pref when toggled. https://reviewboard.mozilla.org/r/103550/#review104226 ::: toolkit/content/tests/chrome/bug451540_window.xul:91 (Diff revision 2) > > // Perform search and turn on highlighting > gFindBar._find(); > highlightButton.click(); > + yield promiseHighlightFinished(); > yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout)); do we still need this timeout given the other promise? Why? ::: toolkit/content/tests/chrome/chrome.ini (Diff revision 2) > [test_chromemargin.xul] > support-files = window_chromemargin.xul > skip-if = toolkit == "cocoa" > [test_bug451540.xul] > support-files = bug451540_window.xul > -skip-if = toolkit == "cocoa" Are we sure this works on mac on infra now? :-) ::: toolkit/content/tests/chrome/findbar_window.xul:531 (Diff revision 2) > > // Regression test for bug 1316515. > searchStr = "e"; > gFindBar.clear(); > yield enterStringIntoFindField(searchStr); > + yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT + 20)); Hrmpf. Don't suppose we can yield for something other than a setTimeout here? This seems like asking for intermittents. :-( ::: toolkit/content/tests/chrome/findbar_window.xul:565 (Diff revision 2) > + // Uncheck at test end to not interfere with other tests. > + highlightButton.click(); Add a registerCleanupFunction instead that flips the pref back? ::: toolkit/content/widgets/findbar.xml:518 (Diff revision 2) > if (aHighlight === this._highlightAll) { > return; > } > - this._highlightAll = aHighlight; > > this.browser.finder.onHighlightAllChange(aHighlight); > > + this._setHighlightAll(aHighlight, aFromPrefObserver); > + > if (!this._dispatchFindEvent("highlightallchange")) { > return; > } > > - this._setHighlightAll(aHighlight, aFromPrefObserver); > - NS_ERROR_TOO_MUCH_MAGIC. For this poor soul who's never seen this code before, can you explain what this does and why it fixes it and why it needs all these test changes? I looked in the bug and the commit messages but didn't see an explanation.

Comment on attachment 8825379 [details] Bug 1316514 - if we attempt to make the 'Highlight All' toggle a global setting, then we need to set the pref when toggled. https://reviewboard.mozilla.org/r/103550/#review104228 Uhm, I hate doing this, but I really did mean r- the first time. Sorry.

(In reply to :Gijs from comment #18) > do we still need this timeout given the other promise? Why? Yes, because the timeout is built into the FinderHighlighter to make sure we don't overuse/ restart it too often. (timer based throttling, see '_scheduleRepaintOfMask') The confusing thing _here_ is that we're using kIteratorTimeout, not kModalHighlightRepaintLoFreqMs which is used in FinderHighlighter. Perhaps I should unify these two? > Are we sure this works on mac on infra now? :-) Try will tell :) > Hrmpf. Don't suppose we can yield for something other than a setTimeout > here? This seems like asking for intermittents. :-( Nope, I'm afraid I've exhausted the options here. > Add a registerCleanupFunction instead that flips the pref back? I meant to say 'for the tests functions that are run after this one', thus strictly in this test file only. > NS_ERROR_TOO_MUCH_MAGIC. > > For this poor soul who's never seen this code before, can you explain what > this does and why it fixes it and why it needs all these test changes? I > looked in the bug and the commit messages but didn't see an explanation. Of course! (I'll add it to the commit message too) What you see first is the removal of the line `this._highlightAll = aHighlight;`, which is repeated in the `_setHighlightAll` method. This line was put here initially to make the test_findbar_events.xul test pass but in fact makes it so that the pref is never set in `_setHighlightAll`! In other words, we never actually persisted the 'Highlight All' state properly. Reading further: the `_dispatchFindEvent` attaches some findbar state flags to the event details, including the value of `_highlightAll`. Even though none of our consumers use it currently (haven't checked if TB does, though), you can cancel further execution of highlighting all ranges. Since the `_setHighlightAll` doesn't do that kind of processing, but merely makes sure the internal state is up to snuff, is persisted properly and the buttons are updated, I moved it up to be invoked before dispatching the event.

Gijs, what do you think of me attempt at explaining things in comment 20?

(In reply to Mike de Boer [:mikedeboer] from comment #20) > (In reply to :Gijs from comment #18) > > do we still need this timeout given the other promise? Why? > > Yes, because the timeout is built into the FinderHighlighter to make sure we > don't overuse/ restart it too often. (timer based throttling, see > '_scheduleRepaintOfMask') > The confusing thing _here_ is that we're using kIteratorTimeout, not > kModalHighlightRepaintLoFreqMs which is used in FinderHighlighter. Perhaps I > should unify these two? It does seem like we should use the 'right' timeout, for one, yes. But really, ideally there should be an event that we can use to wait for instead. Worst case, we can add a test-only one. > > Are we sure this works on mac on infra now? :-) > > Try will tell :) Only the opt tests ran. I retriggered debug. > > Add a registerCleanupFunction instead that flips the pref back? > > I meant to say 'for the tests functions that are run after this one', thus > strictly in this test file only. OK, can you add/update the comment? > > NS_ERROR_TOO_MUCH_MAGIC. > > > > For this poor soul who's never seen this code before, can you explain what > > this does and why it fixes it and why it needs all these test changes? I > > looked in the bug and the commit messages but didn't see an explanation. > > Of course! (I'll add it to the commit message too) > What you see first is the removal of the line `this._highlightAll = > aHighlight;`, which is repeated in the `_setHighlightAll` method. This line > was put here initially to make the test_findbar_events.xul test pass but in > fact makes it so that the pref is never set in `_setHighlightAll`! > In other words, we never actually persisted the 'Highlight All' state > properly. > Reading further: the `_dispatchFindEvent` attaches some findbar state flags > to the event details, including the value of `_highlightAll`. Even though > none of our consumers use it currently (haven't checked if TB does, though), > you can cancel further execution of highlighting all ranges. Since the > `_setHighlightAll` doesn't do that kind of processing, but merely makes sure > the internal state is up to snuff, is persisted properly and the buttons are > updated, I moved it up to be invoked before dispatching the event. Thanks, this helps - it'd be good to have some of this in the commit msg, indeed. :-)

(In reply to Mike de Boer [:mikedeboer] from comment #20) > Yes, because the timeout is built into the FinderHighlighter to make sure we > don't overuse/ restart it too often. (timer based throttling, see > '_scheduleRepaintOfMask') > The confusing thing _here_ is that we're using kIteratorTimeout, not > kModalHighlightRepaintLoFreqMs which is used in FinderHighlighter. Perhaps I > should unify these two? Well, this was a bunch of bullocks from my end... so please allow me to try again. First off, I'm very happy with your fresh set of eyes on the code, Gijs! I started diving deep into the FinderIterator and FinderHighlighter code interaction and noticed something fishy was going on that made the `onHighlightFinished` listener get called way too early, meanwhile resolving all the promises in the tests too. So that's why I added all the setTimeouts there before. With a small correction I was able to remove all of them! \o/

Comment on attachment 8825379 [details] Bug 1316514 - if we attempt to make the 'Highlight All' toggle a global setting, then we need to set the pref when toggled. https://reviewboard.mozilla.org/r/103550/#review105224 Much nicer, thanks! ::: toolkit/modules/Finder.jsm:241 (Diff revision 3) > this.clipboardSearchString = searchString; > return searchString; > }, > > highlight: Task.async(function* (aHighlight, aWord, aLinksOnly) { > - yield this.highlighter.highlight(aHighlight, aWord, null, aLinksOnly); > + yield this.highlighter.highlight(aHighlight, aWord, aLinksOnly); Huh, I thought we had tests for the `links only` mode. Do we not? If not, can you file a followup bug?

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e8838212694 if we attempt to make the 'Highlight All' toggle a global setting, then we need to set the pref when toggled. r=Gijs

Is there another bug for the behavior of the Highlight All button? After seeing this bug marked fixed I experimented with the Highlight All button in the freshly minted 14 January Nightly and the button seems mostly broken. I can't turn highlighting back on, and sometimes clicking the button exits Find mode.

(In reply to B.J. Herbison from comment #30) > Is there another bug for the behavior of the Highlight All button? I'm not aware of any. > After > seeing this bug marked fixed I experimented with the Highlight All button in > the freshly minted 14 January Nightly and the button seems mostly broken. I > can't turn highlighting back on, and sometimes clicking the button exits > Find mode. Please file a separate bug with more details about what you're doing and what you mean specifically by "exits Find mode", and needinfo :mikedeboer . On the new bug, please also include whether you saw this behaviour before this bug was fixed.

Roxana, can you please verify that things work correctly now? Mike, can you please request Aurora approval on this when you get a chance?

Verifying this I noticed that :if the preferences "findbar.modalHighlight" and "findbar.highlightAll" are set to false and "Highlight All" button is clicked , the pref "findbar.highlightAll" is automatically changed to True. So, the bug is not reproducible anymore, but the preference "findbar.highlightAll" is automatically changed to true. Is this intended?

Yes!

Verified as fixed for STR_1 and STR_2 using the latest Firefox Nightly 53.0a1 (Build ID: 20170116030326), (having preferences findbar.modalHighlight and findbar.highlightAll true and false) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11.

Created bug 1331293.

(In reply to B.J. Herbison from comment #36) > Created bug 1331293. Thanks! (In reply to Ryan VanderMeulen [:RyanVM] from comment #32) > Roxana, can you please verify that things work correctly now? > > Mike, can you please request Aurora approval on this when you get a chance? I think we should hold off on this until we figure out bug 1331293. AIUI we're not shipping the modal highlight on beta, and aurora is about to merge to beta, so this will make its way to devedition/aurora in 1 week anyway, and will be disabled on beta so won't be reproducible with the modalHighlight pref off anyway (I think? Mike, can you confirm?) so perhaps we should just leave it at that...

Well, this bug is with the modal highlighting feature turned OFF, so I think it's still eligible for uplift...

Yeah, my understanding from Roxana's initial verification is that there were remaining regressions in the disabled case, which is why I was pushing to get this fixed for Beta :)

I just confirmed Roxana's findings that this only affects Fx 52. I'll request uplift now.

Comment on attachment 8825379 [details] Bug 1316514 - if we attempt to make the 'Highlight All' toggle a global setting, then we need to set the pref when toggled. Approval Request Comment [Feature/Bug causing the regression]: Findbar highlighting refactor [User impact if declined]: the highlight all feature of the findbar may show inconsistent behavior when toggling its state using the button or pref. [Is this code covered by automated tests?]: Yes, tests were added. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Already done. [List of other uplifts needed for the feature/fix]: none. [Is the change risky?]: no. [Why is the change risky/not risky?]: n/a. [String changes made/needed]: n/a.

I just want to add some more clarifications here before the uplift to Aurora: The scenario from this bug it is not reproducible on Nightly 53, but one other issue is happening after the fix of this bug: With preferences "findbar.modalHighlight" and "findbar.highlightAll" set to FALSE, when I click on "Highlight All" button, the pref "findbar.highlightAll" is automatically changed to TRUE. I also investigated with mozregression and it gets me to the commit of this bug: Last good revision: de67fccc4c64a49f261aea29141357b94c7b3b9c First bad revision: 671a2f8542cadcf9aad318ab76250a730a03b240 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=de67fccc4c64a49f261aea29141357b94c7b3b9c&tochange=671a2f8542cadcf9aad318ab76250a730a03b240 9:56.38 ERROR: Unable to exploit the merge commit. Origin branch is mozilla-central, and the commit message for 671a2f85 was: Merge autoland to central, a=merge MozReview-Commit-ID: Ano2O7fFKEW

(In reply to roxana.leitan@softvision.ro from comment #42) > I just want to add some more clarifications here before the uplift to Aurora: > > The scenario from this bug it is not reproducible on Nightly 53, but one > other issue is happening after the fix of this bug: > With preferences "findbar.modalHighlight" and "findbar.highlightAll" set to > FALSE, when I click on "Highlight All" button, the pref > "findbar.highlightAll" is automatically changed to TRUE. > I also investigated with mozregression and it gets me to the commit of this > bug: ...which is exactly what this bug intended to fix.

Comment on attachment 8825379 [details] Bug 1316514 - if we attempt to make the 'Highlight All' toggle a global setting, then we need to set the pref when toggled. "highlight all" findbar fix, aurora52+

It'd be great if QE could make sure the find bar works as expected in aurora after this lands. Thanks!

Verified as fixed for STR_1 and STR_2 using the latest Firefox Aurora 52.0a2 (Build ID: 20170122004006), on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.10.