Closed Bug 732803 Opened 12 years ago Closed 10 years ago

'Menu Bar' menu entry not on top in View > Toolbars menu

Categories

(Thunderbird :: Toolbars and Tabs, defect)

All
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 3 obsolete files)

After landing of Bug 644169 the 'Menu Bar' menu entry is no more on top in View > Toolbars menu in main window. This menu entry should be the top most like in Firefox or in AB and Compozer.
Blocks: tb-tabsontop
And by default the menu bar should be above the tabs (like it is on firefox if you hit alt). Don't know if it's the same bug, but it sure looks very odd to have the menu bar below tabs, visible by default. I'd think a lot of the bad reactions to tabs on top (bug 724213) may be due to that.
Hardware: x86_64 → All
This bug is only for the menu entry. Menu bar below tabs was bwinton's decision and it should be only under Aero below the tabs. When it would be above, then the menu bar is in the transparent area and then the reactions would be it isn't readable as you can find on TB <11.
The menu bar is an external toolbar, so it comes after the primary toolbars. See <http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#280>.
One possible way of handling this would be to give the menu bar an attribute like insertbefore="true" that the code above would check for and then sort the array of toolbars appropriately.
Attached patch patch (obsolete) — Splinter Review
This patch changes the order which toolbars concatenates the other toolbars. Is this a valid way?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #820193 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 820193 [details] [diff] [review]
patch

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

I think we should keep the order the way it is in general. Granted, we don't have any other external toolbars at the moment, but we should leave this as is to match how Firefox does things. See comment 4 for a suggestion on how to put just the menu bar first.
Attachment #820193 - Flags: review?(squibblyflabbetydoo) → review-
Attached patch patch v2 (obsolete) — Splinter Review
The attribute "insertbefore" didn't work for me. I check now for mail-toolbar-menubar2 and insert it as the first menuitem. In AppMenu this would be wrong and I made a special case to insert it at the correct position. The menus in the other windows like Compozer or AB are working as before.
Attachment #820193 - Attachment is obsolete: true
Attachment #8351661 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 8351661 [details] [diff] [review]
patch v2

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

::: mail/base/content/mailCore.js
@@ +298,5 @@
>          menuItem.setAttribute("label", toolbarName);
>          menuItem.setAttribute("accesskey", toolbar.getAttribute("accesskey"));
>          menuItem.setAttribute("checked",
>                                toolbar.getAttribute(hidingAttribute) != "true");
> +        // Is the bar the mail-toolbar-menubar2?

We shouldn't special-case the code like this. It's messy and prone to breakage if we ever change anything (or if we want to apply this code in other areas).

As mentioned in comment 4, you should create a new attribute that you can set on external toolbars (in this case, the status bar) to modify the order in which they are listed. Then, instead of appending the external toolbars[1], you'll want to check the newly-created attribute to determine whether the external toolbar should be appended or *prepended* to the childToolbars array.

[1] http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#280
Attachment #8351661 - Flags: review?(squibblyflabbetydoo) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Now I use insertasfirst= "true" as attribute for this menubar, is this okay? I'm still using the popup.insertBefore(menuItem, popup.firstChild); because the prepend can't work with this fixed insertion point [1]. And point the insertion point to the previous toolbar would also not help when you have multiple toolbars (main toolbar, custom toolbar and then menubar).

[1] http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#272
Attachment #8351661 - Attachment is obsolete: true
Attachment #8351688 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 8351688 [details] [diff] [review]
patch v3

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

This is closer, but I think it would work better to modify the order of the array of toolbars *before* you iterate over them, like I show below. This way, you don't have to hardcode anything into onViewToolbarsPopupShowing. I also have some comments after the diff.

diff --git a/mail/base/content/mailCore.js b/mail/base/content/mailCore.js
--- a/mail/base/content/mailCore.js
+++ b/mail/base/content/mailCore.js
@@ -271,20 +271,27 @@ function onViewToolbarsPopupShowing(aEve
   // point is defined.
   let firstMenuItem = aInsertPoint || popup.firstChild;
 
   for (let [, toolboxId] in Iterator(toolboxIds)) {
     let toolbox = document.getElementById(toolboxId);
 
     // We'll consider either childnodes that have a toolbarname attribute,
     // or externalToolbars.
-    let childToolbars = Array.slice(toolbox.querySelectorAll("[toolbarname]"));
-    let potentialToolbars = childToolbars.concat(toolbox.externalToolbars);
+    let potentialToolbars = Array.slice(
+      toolbox.querySelectorAll("[toolbarname]")
+    );
+    for (let externalToolbar of toolbox.externalToolbars) {
+      if (externalToolbar.getAttribute("insertasfirst"))
+        potentialToolbars.unshift(externalToolbar);
+      else
+        potentialToolbars.push(externalToolbar);
+    }
 
-    for (let [, toolbarElement] in Iterator(potentialToolbars)) {
+    for (let toolbarElement of potentialToolbars) {
 
       // We have to bind to toolbar because Javascript doesn't do fresh
       // let-bindings per Iteration.
       let toolbar = toolbarElement;
 
       let toolbarName = toolbar.getAttribute("toolbarname");
       if (toolbarName) {
         let menuItem = document.createElement("menuitem");

::: mail/base/content/mailWindowOverlay.xul
@@ +1273,5 @@
>                        oncommand="openOptionsDialog();"/>
>              <menuitem id="appmenu_accountmgr"
>                        label="&accountManagerCmd.label;"
>                        oncommand="MsgAccountManager(null);"/>
> +            <menuseparator id="appmenu_beforeToolbarsSeparator"/>

With my suggestion above, this change isn't necessary.

@@ +2211,5 @@
>  #endif
> +           context="toolbar-context-menu"
> +           mode="icons"
> +           insertbefore="tabs-toolbar"
> +           insertasfirst= "true">

Hmm, there's got to be a clearer name for this. How about "prependmenuitem"? That name's not great either, but at least it won't get confused with "insertbefore", which is an XBL thing.
Attachment #8351688 - Flags: review?(squibblyflabbetydoo) → review-
Attached patch patch v4Splinter Review
This patch uses the way from comment 10. I'm also using now "prependmenuitem" as attribute.

Philipp: I added you for the Lightning part. This changes the order in toolbar context menu to menu on top and calendar-/task-toolbar below it.
Attachment #8351688 - Attachment is obsolete: true
Attachment #8361263 - Flags: review?(squibblyflabbetydoo)
Attachment #8361263 - Flags: review?(philipp)
Comment on attachment 8361263 [details] [diff] [review]
patch v4

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

Looks good to me!
Attachment #8361263 - Flags: review?(squibblyflabbetydoo) → review+
Attachment #8361263 - Flags: review?(philipp) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b4576d25f30f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.