Closed Bug 943770 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29

People

(Reporter: GiTcode, Assigned: GiTcode)

Details

Attachments

(1 file, 2 obsolete files)

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.
Component: Untriaged → Toolbars and Customization
OS: Linux → All
Hardware: x86_64 → All
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 */
}
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
(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.
Attached patch patch-1 (obsolete) — Splinter Review
Attachment #8340702 - Flags: review?(mconley)
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)
Attachment #8340702 - Flags: review?(mconley) → review-
Assignee: nobody → goMobileFirefox
Status: NEW → ASSIGNED
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
Attached patch patch-1-corrected (obsolete) — Splinter Review
Attachment #8340702 - Attachment is obsolete: true
Attachment #8341192 - Flags: review?
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.
Attachment #8341192 - Flags: review? → review+
(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?
(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!
[Another newbie question]: Where all will the patch be applied to? Just Nightly, or others such as Beta and Aurora too?
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).
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
Whiteboard: [fixed-in-fx-team]
Okay, and thanks!
https://hg.mozilla.org/mozilla-central/rev/6cfa46dc9859
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Keywords: verifyme
Do you by any chance have an add-on that could demonstrate this bug?
Flags: needinfo?(GiTcode)
(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.
Flags: needinfo?(GiTcode)
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).
Keywords: verifyme
(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.
Keywords: verifyme
Target Milestone: Firefox 28 → Firefox 29
(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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.