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

VERIFIED FIXED in Firefox 29

Status

()

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

People

(Reporter: GiTcode, Assigned: GiTcode)

Tracking

unspecified
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

4 years ago
Component: Untriaged → Toolbars and Customization
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
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 */
}

Updated

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

Comment 3

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

Comment 4

4 years ago
Created attachment 8340702 [details] [diff] [review]
patch-1
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-

Updated

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

Comment 7

4 years ago
Created attachment 8341192 [details] [diff] [review]
patch-1-corrected
Attachment #8340702 - Attachment is obsolete: true
Attachment #8341192 - Flags: review?

Comment 8

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

Comment 9

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

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

Comment 11

4 years ago
Created attachment 8341287 [details] [diff] [review]
Bug 943770 - include toolbars with toolboxid attribute set in View -> Toolbars menu and toolbar context menu, r=gijs
Attachment #8341192 - Attachment is obsolete: true
Attachment #8341287 - Flags: review+
(Assignee)

Comment 12

4 years ago
[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).

Comment 14

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

Comment 15

4 years ago
Okay, and thanks!
https://hg.mozilla.org/mozilla-central/rev/6cfa46dc9859
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28

Updated

4 years ago
Keywords: verifyme

Comment 17

4 years ago
Do you by any chance have an add-on that could demonstrate this bug?
Flags: needinfo?(GiTcode)

Comment 18

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

Comment 19

4 years ago
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

Comment 20

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

Comment 21

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