Closed Bug 891638 Opened 7 years ago Closed 7 years ago

"Highlight All" button does not persist when switching tabs

Categories

(Firefox :: General, defect)

25 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 25

People

(Reporter: jaws, Assigned: hobophobe)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Thanks Jared!

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

Also adds a small test for this.
Assignee: nobody → unusualtears
Status: NEW → ASSIGNED
Attachment #773015 - Flags: review?(dao)
Summary: Selecting "Highlight All", switching tabs, then revisiting the tab with "Highlight All" shows the an inconsistent state → "Highlight All" button does not persist when switching tabs
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!
Attachment #773015 - Flags: review?(dao) → review+
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.
Attachment #773015 - Flags: review-
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.
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.
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.
Attachment #773015 - Attachment is obsolete: true
Attachment #773552 - Flags: review?(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.
Attachment #773552 - Flags: review?(dao) → review-
Thanks Dão!
Attachment #773552 - Attachment is obsolete: true
Attachment #773580 - Flags: review?(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 = ...;'
Attachment #773580 - Flags: review?(dao) → review+
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=67712386dfa3
Attachment #773580 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c2d85b34ff8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
QA Contact: manuela.muntean
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.