If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fx4waldi, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
All
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8][Australis:P1])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Blocks: 872617
Component: Untriaged → Toolbars and Customization
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [Australis:M?]
Version: 24 Branch → Trunk
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
(Assignee)

Comment 1

4 years ago
AFAICT this is at least partially causing bug 885069. Taking and upping priority.
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P1]
(Assignee)

Comment 2

4 years ago
Created attachment 772638 [details] [diff] [review]
Patch

This WFM. Note that this is actually also reproducible on Win7.
Attachment #772638 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
OS: All → Windows 7
(Assignee)

Comment 3

4 years ago
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 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.
Attachment #772638 - Flags: review?(mconley) → review+
Why is this event dispatched on the toolbox in the first place rather than bubbling up to it?
(Assignee)

Comment 6

4 years ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(dao)
(Assignee)

Comment 8

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

https://hg.mozilla.org/projects/ux/rev/fa7675b67853
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M8][Australis:P1][fixed-in-ux]
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/mozilla-central/rev/fa7675b67853
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.