886030 - When you disable the menubar in customization mode, menubar is visible

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[fx4waldi]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20130622 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130622040202

Steps to reproduce:

1. Enable menubar
2. Open customization mode
3. Disable menubar
4. Close customization mode


Actual results:

menubar is visible and you can not disable it

Comment 1

[:Gijs (he/him)]

AFAICT this is at least partially causing bug 885069. Taking and upping priority.

Comment 2

[:Gijs (he/him)]

Attachment: Patch

This WFM. Note that this is actually also reproducible on Win7.

Comment 3

[:Gijs (he/him)]

Comment on attachment 772638 [details] [diff] [review]
Patch

>+  if (toolbar.id == "PlacesToolbar") {
>+    PlacesToolbarHelper.init();
>+    BookmarkingUI.onToolbarVisibilityChange();
>+  }


This is wrong (should be PersonalToolbar), and also unnecessary, strictly speaking. With the ID corrected, it might still be worthwhile (we could even convert those consumers to use the event rather than this rather-tight-coupling) but I'll leave that up to you Mike. :-)

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 772638 [details] [diff] [review]
Patch

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

r=me with the following fixes. Thanks!

::: browser/base/content/browser.js
@@ +4308,5 @@
> +      visible: isVisible
> +    }
> +  };
> +  let event = new CustomEvent("toolbarvisibilitychange", eventParams);
> +  gNavToolbox.dispatchEvent(event);

I'm not sure if there are outside callers of this function that use toolboxes and toolbars of their own, but if so, we might want to fire this event on toolbar.toolbox, instead of assuming it's on gNavToolbox.

@@ +4310,5 @@
> +  };
> +  let event = new CustomEvent("toolbarvisibilitychange", eventParams);
> +  gNavToolbox.dispatchEvent(event);
> +
> +  if (toolbar.id == "PlacesToolbar") {

As you say, this id is wrong. I think we can safely forgo this conditional, and leave it the way it was.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +133,5 @@
>        this.visiblePalette.addEventListener("dragexit", this, true);
>        this.visiblePalette.addEventListener("drop", this, true);
>        this.visiblePalette.addEventListener("dragend", this, true);
>  
> +      window.gNavToolbox.addEventListener("toolbarvisibilitychange", this, false);

The last argument, "false", is the default, and can be omitted.

@@ +186,5 @@
>        browser.parentNode.selectedPanel = browser;
>  
>        yield this.depopulatePalette();
>  
> +      window.gNavToolbox.removeEventListener("toolbarvisibilitychange", this, false);

Same as above.

Comment 5

[Dão Gottwald [:dao]]

Why is this event dispatched on the toolbox in the first place rather than bubbling up to it?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Why is this event dispatched on the toolbox in the first place rather than
> bubbling up to it?

This is an interesting idea. The customization events have used the toolbox since before Australis, so it seemed the natural place to add a new event, but in this case perhaps the bubbling approach make more sense because then we get event.target for free? I'm not completely decided. Would you prefer that, Dao?

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Why is this event dispatched on the toolbox in the first place rather than
> > bubbling up to it?
> 
> This is an interesting idea. The customization events have used the toolbox
> since before Australis, so it seemed the natural place to add a new event,

Those events didn't relate to any specific toolbar.

> but in this case perhaps the bubbling approach make more sense because then
> we get event.target for free? I'm not completely decided. Would you prefer
> that, Dao?

Yes.

Comment 8

[:Gijs (he/him)]

Pushed with events firing on the toolbar, and other nits from comment #4 fixed:

https://hg.mozilla.org/projects/ux/rev/fa7675b67853

Comment 9

[:Gijs (he/him)]

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