943770 - Toolbars with toolboxid attribute set should be included in View --> Toolbars menu

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[GiTcode]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131119090945

Steps to reproduce:

Overlay a toolbar element into the navigator toolbox. Also add another toolbar outside of the navigator toolbox with the toolboxid attribute set to "navigator-toolbox" in an XUL addon.


Actual results:

The toolbar inside the navigator toolbox is listed in View --> Toolbars, and also in the toolbar-context-menu. But the toolbar outside of the navigator toolbox with the toolboxid set to "navigator-toolbox" is not included.


Expected results:

External toolbars should be inside the View --> Toolbars menu.

Comment 1

[GiTcode]

Sorry, I am not sure how to submit a patch, but here is my suggested fix:

/*From chrome://browser/content/browser.js*/

function onViewToolbarsPopupShowing(aEvent, aInsertPoint) {
   var popup = aEvent.target;
   if (popup != aEvent.currentTarget)
     return;
 
   // Empty the menu
   for (var i = popup.childNodes.length-1; i >= 0; --i) {
     var deadItem = popup.childNodes[i];
     if (deadItem.hasAttribute("toolbarId"))
       popup.removeChild(deadItem);
   }
 
   var firstMenuItem = aInsertPoint || popup.firstChild;
 
   let toolbarNodes = Array.slice(gNavToolbox.childNodes);
   /* my patch */
   toolbarNodes = Array.concat(toolbarNodes, Array.slice(gNavToolbox.externalToolbars));
   /* end of my patch */
 
   /* more code */
}

Comment 2

[Dão Gottwald [:dao]]

(In reply to goMobileFirefox from comment #1)
> Sorry, I am not sure how to submit a patch, but here is my suggested fix:

https://developer.mozilla.org/en-US/docs/Introduction should give you the relevant pointers.

Comment 3

[GiTcode]

(In reply to Dão Gottwald [:dao] from comment #2)
> https://developer.mozilla.org/en-US/docs/Introduction should give you the
> relevant pointers.

Okay, thanks! I am going through it. Hopefully will have the patch up before too long.

Comment 4

[GiTcode]

Attachment: patch-1

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8340702 [details] [diff] [review]
patch-1

gNavToolbox.externalToolbars is already an array, so you don't need to use Array.slice there. Also, since toolbarNodes is an array, you can call concat on it directly:

toolbarNodes = toolbarNodes.concat(gNavToolbox.externalToolbars)

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8340702 [details] [diff] [review]
patch-1

>   let toolbarNodes = Array.slice(gNavToolbox.childNodes);
>+   toolbarNodes = Array.concat(toolbarNodes, Array.slice(gNavToolbox.externalToolbars));

nit: please indent using two spaces instead of three

Comment 7

[GiTcode]

Attachment: patch-1-corrected

Comment 8

[:Gijs (he/him)]

Comment on attachment 8341192 [details] [diff] [review]
patch-1-corrected

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

r=me

If you could upload a patch that has a better description that might be helpful, otherwise I'll just make one up and land this on fx-team.

Comment 9

[GiTcode]

(In reply to :Gijs Kruitbosch from comment #8)
> If you could upload a patch that has a better description that might be
> helpful, otherwise I'll just make one up and land this on fx-team.

So something like: "make toolbars with toolboxid attribute set included in View --> Toolbars menu and toolbar-context-menu"?

[newbie question]: And then mark it as review + and the old patch as obsolete?

Comment 10

[:Gijs (he/him)]

(In reply to goMobileFirefox from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > If you could upload a patch that has a better description that might be
> > helpful, otherwise I'll just make one up and land this on fx-team.
> 
> So something like: "make toolbars with toolboxid attribute set included in
> View --> Toolbars menu and toolbar-context-menu"?

Right. It's customary to still leave the bug number and the reviewer, so it'd be something like:

"Bug 943770 - include toolbars with toolboxid attribute set in View -> Toolbars menu and toolbar context menu, r=gijs"

> [newbie question]: And then mark it as review + and the old patch as
> obsolete?

Yup! That sounds great!

Comment 11

[GiTcode]

Attachment: Bug 943770 - include toolbars with toolboxid attribute set in View -> Toolbars menu and toolbar context menu, r=gijs

Comment 12

[GiTcode]

[Another newbie question]: Where all will the patch be applied to? Just Nightly, or others such as Beta and Aurora too?

Comment 13

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

This patch will be applied to Nightly, and will make its way to Aurora when the rest of Australis reaches Aurora (which will not be Aurora 28, hopefully it will be Aurora 29).

Comment 14

[:Gijs (he/him)]

For future reference, when I asked to update the description, I meant the bit inside the patch file, which says [PATCH]: bug #943770. I've gone ahead and changed that, although I've added "Australis:" per discussion with Jared so this gets backed out on Holly (our backout branch to ensure we can "turn Australis off" as long as we're not ready to ship it yet). This means that, as Jared said, this will land on Nightly, but won't make Aurora 28.

remote:   https://hg.mozilla.org/integration/fx-team/rev/6cfa46dc9859

Comment 15

[GiTcode]

Okay, and thanks!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cfa46dc9859

Comment 17

[Ioana (away)]

Do you by any chance have an add-on that could demonstrate this bug?

Comment 18

[:Gijs (he/him)]

(In reply to Ioana Budnar, QA [:ioana] from comment #17)
> Do you by any chance have an add-on that could demonstrate this bug?

0. Enable Remote & Chrome Debugging from the developer tools' settings tab
1. Open Scratchpad
2. Set "Environment" to "Browser"
3. Run:

let toolbar = document.createElement("toolbar");
toolbar.id = "MyNonToolboxToolbar";
toolbar.setAttribute("toolbarname", "My Fancy Toolbar");
toolbar.setAttribute("toolboxid", "navigator-toolbox");

let bottomBox = document.getElementById("browser-bottombox");

bottomBox.appendChild(toolbar);

4. Right click the navigation toolbar

You should see "My Fancy Toolbar" with a checkmark in front of it. Clicking that item will hide the toolbar that appeared at the bottom. Repeating this will unhide it again.

To get rid of the extra toolbar completely, run:

document.getElementById("MyNonToolboxToolbar").remove()

from either scratchpad (clear what you put in previously!) or the browser console.

Comment 19

[Ioana (away)]

So I tried the steps in comment 18 (thanks for them Gijs :) ) and I get the same results with a buggy build and builds with the fix (Fx28.0a1, Fx28.0b4 / Windows 7, Ubuntu 13.04). The toolbar is added to the bottom of the window, but it doesn't show up in any menu (Toolbars menu, navigation bar contextual menu).

Comment 20

[:Gijs (he/him)]

(In reply to Ioana Budnar, QA [:ioana] from comment #19)
> So I tried the steps in comment 18 (thanks for them Gijs :) ) and I get the
> same results with a buggy build and builds with the fix (Fx28.0a1, Fx28.0b4
> / Windows 7, Ubuntu 13.04). The toolbar is added to the bottom of the
> window, but it doesn't show up in any menu (Toolbars menu, navigation bar
> contextual menu).

As comment #13 notes, this patch isn't in 28.

Comment 21

[Ioana (away)]

(In reply to :Gijs Kruitbosch from comment #20)
> As comment #13 notes, this patch isn't in 28.

My bad. Thought there was a change of mind since the TM wasn't changed.

Verified as fixed on the 02/20 Firefox 29.0a2 (Win7, Ubuntu 13.04, Mac OSX 10.5).