891638 - "Highlight All" button does not persist when switching tabs

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Open Findbar and search for a string present in the page
Choose "Highlight All"
Notice that the string is now selected with a purple background and the Highlight All button is pressed
Switch to a different tab
Return to the previous tab

Expected:
The Highlight All button is still pressed, the text is still purple.

Actual:
The Highlight All button is not pressed, but the text is still purple.

Comment 1

[Adam]

Attachment: Remove highlight disabling in browser.js, add test

Thanks Jared!

Bug 253793 disabled highlight via onLocationChange, but that's not needed since bug 537013.

Also adds a small test for this.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 773015 [details] [diff] [review]
Remove highlight disabling in browser.js, add test

Review of attachment 773015 [details] [diff] [review]:
-----------------------------------------------------------------

Nice turn-around-time!

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 773015 [details] [diff] [review]
Remove highlight disabling in browser.js, add test

(In reply to Adam [:hobophobe] from comment #1)
> Created attachment 773015 [details] [diff] [review]
> Remove highlight disabling in browser.js, add test
> 
> Thanks Jared!
> 
> Bug 253793 disabled highlight via onLocationChange, but that's not needed
> since bug 537013.

Bug 253793 wasn't about tab switching but about a new document being loaded in the same tab. Bug 537013 shouldn't matter in this regard, so this code should still be needed, although it should move to tabbrowser.xml's or TabsProgressListener's onLocationChange handler.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

The bug as I filed it is more about the inconsistent state (highlight all being unchecked but all of the selections still being present). When we disable Highlight All we should also clear the selections.

Comment 5

[Dão Gottwald [:dao]]

Moving it to the right place (as mentioned in comment 3) should fix that, as this code wouldn't run anymore in response to tab switching.

Comment 6

[Adam]

Attachment: Move highlight disabling to tab progress listener

Moved it to the tabbrowser.xml handler and updated it to run for the tab whose location changed, since it could be a background tab with a findbar.

Also a test for bug 253793.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 773552 [details] [diff] [review]
Move highlight disabling to tab progress listener

>+      // Close the Find toolbar if we're in old-style TAF mode
>+      if (gFindBarInitialized && gFindBar.findMode != gFindBar.FIND_NORMAL)
>           gFindBar.close();

indentation is off ...

... but instead of fixing it, move this code over to tabbrowser.xml, because it really belongs there as well.

Comment 8

[Adam]

Attachment: Move closing quickfind onLocationChange too

Thanks Dão!

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 773580 [details] [diff] [review]
Move closing quickfind onLocationChange too

>+                if (this.mTabBrowser.isFindBarInitialized(this.mTab)) {
>+                  let findBar = this.mTabBrowser.getFindBar(this.mTab);
>+                  // Close the Find toolbar if we're in old-style TAF mode
>+                  if (findBar.findMode != findBar.FIND_NORMAL)
>+                    findBar.close();
>+
>+                  // fix bug 253793 - turn off highlight when page changes
>+                  findBar.getElement("highlight").checked = false;
>+                }

nit: add an empty line after 'let findBar = ...;'

Comment 10

[Adam]

Attachment: Prepared for checkin

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=67712386dfa3

Comment 11

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2d85b34ff8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c2d85b34ff8

Comment 13

[Manuela Muntean [Away]]

While testing for the pre-beta sign-off of the Find Bar Redesign feature, I verified this bug with the latest Aurora (build ID: 20130903004001), and this works as expected on Win 8 32-bit, Mac OS X 10.8 and Ubuntu 12.10 32-bit.