869334 - Find bar should auto-hide in customization mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement)

Description

[u428464]

The find bar currently stays open when entering customization mode, but it isn't a customization area. It should auto-hide in customization mode.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1 (original patch)

Well, this patch should work.... but it doesn't. Seems like something is broken on UX branch, as its not working as expected on about:addons either (cmd_find etc don't get disabled).


Also, I should note that the disablefastfind attribute only disables the find commands - if the find bar is already showing, it won't autohide. I think it should... but there's an existing bug in the handling of disablefastfind, IMO.

Comment 2

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

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #1)
> Created attachment 746284 [details] [diff] [review]
> Patch v1 - doesn't work :(
> 
> Well, this patch should work.... but it doesn't. Seems like something is
> broken on UX branch, as its not working as expected on about:addons either
> (cmd_find etc don't get disabled).

If I open about:addons on the UX branch, I cannot get Ctrl+F to open the find bar, so as far as I can tell, it's still working.

Comment 3

[u428464]

(In reply to Jared Wein [:jaws] from comment #2)
> (In reply to Blair McBride [:Unfocused] (Limited availability.) from comment
> #1)
> > Created attachment 746284 [details] [diff] [review]
> > Patch v1 - doesn't work :(
> > 
> > Well, this patch should work.... but it doesn't. Seems like something is
> > broken on UX branch, as its not working as expected on about:addons either
> > (cmd_find etc don't get disabled).
> 
> If I open about:addons on the UX branch, I cannot get Ctrl+F to open the
> find bar, so as far as I can tell, it's still working.

Yes, but the problem is that the find bar isn't dismissed like before (open the find bar and then open the add-ons manager : the find bar is still visible).

Comment 4

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

Attachment: Patch v1.1

This fixes it for all pages that have requested to disablefastfind.

Comment 5

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

(In reply to Guillaume C. [:ge3k0s] from comment #3)
> (In reply to Jared Wein [:jaws] from comment #2)
> > (In reply to Blair McBride [:Unfocused] (Limited availability.) from comment
> > #1)
> > > Created attachment 746284 [details] [diff] [review]
> > > Patch v1 - doesn't work :(
> > > 
> > > Well, this patch should work.... but it doesn't. Seems like something is
> > > broken on UX branch, as its not working as expected on about:addons either
> > > (cmd_find etc don't get disabled).
> > 
> > If I open about:addons on the UX branch, I cannot get Ctrl+F to open the
> > find bar, so as far as I can tell, it's still working.
> 
> Yes, but the problem is that the find bar isn't dismissed like before (open
> the find bar and then open the add-ons manager : the find bar is still
> visible).

I've never seen it get dismissed before when entering the add-ons manager. Can you point me to a build that does?

Comment 6

[u428464]

(In reply to Jared Wein [:jaws] from comment #5)
> (In reply to Guillaume C. [:ge3k0s] from comment #3)
> > (In reply to Jared Wein [:jaws] from comment #2)
> > > (In reply to Blair McBride [:Unfocused] (Limited availability.) from comment
> > > #1)
> > > > Created attachment 746284 [details] [diff] [review]
> > > > Patch v1 - doesn't work :(
> > > > 
> > > > Well, this patch should work.... but it doesn't. Seems like something is
> > > > broken on UX branch, as its not working as expected on about:addons either
> > > > (cmd_find etc don't get disabled).
> > > 
> > > If I open about:addons on the UX branch, I cannot get Ctrl+F to open the
> > > find bar, so as far as I can tell, it's still working.
> > 
> > Yes, but the problem is that the find bar isn't dismissed like before (open
> > the find bar and then open the add-ons manager : the find bar is still
> > visible).
> 
> I've never seen it get dismissed before when entering the add-ons manager.
> Can you point me to a build that does?

My bad I thought it was the default behavior since the find bar is useless in the add-ons manager.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 746414 [details] [diff] [review]
Patch v1.1

This unintentionally initializes the findbar because it doesn't use gFindBarInitialized.

I'm not sure why you're setting an attribute on gFindBar to keep track of whether it used to be open -- presumably you could make this a private property of XULBrowserWindow.

Comment 8

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

Attachment: Patch v1.2

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #2)
> (In reply to Blair McBride [:Unfocused] (Limited availability.) from comment
> #1)
> > Created attachment 746284 [details] [diff] [review]
> > Patch v1 - doesn't work :(
> > 
> > Well, this patch should work.... but it doesn't. Seems like something is
> > broken on UX branch, as its not working as expected on about:addons either
> > (cmd_find etc don't get disabled).
> 
> If I open about:addons on the UX branch, I cannot get Ctrl+F to open the
> find bar, so as far as I can tell, it's still working.

*sigh* Yea, seems I had a partially broken tree. Clobbering and rebuilding fixed it.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 746486 [details] [diff] [review]
Patch v1.2

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

I'm not familiar enough with this code to comment on the patch itself.... but it does beg for an addition to browser_zbug569342.js (not my typo!).

Comment 11

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

Attachment: Patch v1.3 (with test added)

Thanks for the link to the test Blair :)

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 746752 [details] [diff] [review]
Patch v1.3 (with test added)

>@@ -3457,16 +3457,21 @@ var XULBrowserWindow = {
>   statusText: "",
>   isBusy: false,
>   // The pages in this array should be kept in sync with what pages that
>   // panelUIOverlay.xul is set to overlay in
>   // browser/components/customizableui/content/jar.mn
>   inContentWhitelist: ["about:addons", "about:downloads", "about:permissions",
>                        "about:sync-progress", "about:preferences"],
> 
>+  // The findbar is temporarily hidden when viewing pages that have
>+  // disablefastfind set. The findbar state is restored when viewing
>+  // non-disablefastfind pages.
>+  findbarTemporarilyHidden: false,
>+

You can drop this.

>+        if (gFindBarInitialized) {
>+          if (!gFindBar.hidden && aDisable) {
>+            gFindBar.hidden = true;
>+            XULBrowserWindow.findbarTemporarilyHidden = true;
>+          } else if (XULBrowserWindow.findbarTemporarilyHidden && !aDisable) {
>+            gFindBar.hidden = false;
>+            XULBrowserWindow.findbarTemporarilyHidden = false;
>+          }
>+        }

Can you bind this function and use 'this' rather than XULBrowserWindow?

Also please rename findbarTemporarilyHidden to _findbarTemporarilyHidden.

I'd actually prefer if everything but the aboutCustomizing.xhtml change was in a separate patch (and bug) that can land on mozilla-central directly.

Comment 13

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

Comment on attachment 746284 [details] [diff] [review]
Patch v1 (original patch)

This patch will work as expected once bug 869919 is landed. It also blocks the find commands from working, and that part doesn't have to wait until bug 869919 lands.

Comment 14

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

https://hg.mozilla.org/projects/jamun/rev/ebc0e57b9755

Comment 15

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/ebc0e57b9755

Comment 16

[Petruta Horea [:phorea]]

The find bar is hidden in Customization mode in latest Nightly 29.0a1 (20140203030203) under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.9.

Can't be verified on Aurora 28.0a2 since Australis was backed out there.