650170 - (TB-AppMenu) Firefox-like Application Button/Menu for Thunderbird (Appmenu/hamburger)

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[Stanzilla]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/534.29 (KHTML, like Gecko) Chrome/12.0.733.0 Safari/534.29
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.2pre) Gecko/20110414 Thunderbird/3.3a4pre

Now with Firefox 4 being released and the imo great success of the new Firefox menu, it's time to bring that beauty to Thunderbird, too.

Reproducible: Always

Comment 1

[rsx11m]

Personally, I don't see the benefit of the application menu as it's adding redundancy to provide the same functionality in a new menu hierarchy and also implies additional work to implement it; but, if the aim for Thunderbird is
to maintain parity with Firefox, then this would be something to consider.

Comment 2

[Andreas Nilsson (:andreasn)]

One good thing about a one-button-menu would be that it frees up some horizontal space. It would need a testpilot study too see what most people use and some reorganization to see what goes where.

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to comment #2)
> One good thing about a one-button-menu would be that it frees up some
> horizontal space. It would need a testpilot study too see what most people
> use and some reorganization to see what goes where.

Personally I like it Fx. I suspect for the majority of TB users it would be a net benefit because most use toolbar. However, trimming down menus also potentially results in less discoverability.


(In reply to comment #1)
> Personally, I don't see the benefit of the application menu as it's adding
> redundancy to provide the same functionality in a new menu hierarchy and
> also implies additional work to implement it; but, if the aim for

rsx11m, I don't understand what you mean by redundancy.  can you explain?

Comment 4

[rsx11m]

Redundancy in the sense of having two independent set of menus at different locations, one the established menu bar, the other the new application menu. Meaning, both versions may need to be maintained, considered by add-ons, etc.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to comment #4)
> Redundancy in the sense of having two independent set of menus at different
> locations

I would expect that there would not be both.

Comment 6

[rsx11m]

Firefox didn't remove the traditional menu bar on aero platforms, it's just hiding it so that one or the other is shown (and you still need it for XP and non-Windows platforms anyway). I can also imagine an outburst if indeed the option for the old menus would be removed (including myself in that list).

Comment 7

[Wayne Mery (:wsmwk)]

sorry, yeah. I meant "both visible".  And removing the original menu might be a calamity in obsoleting tons of existing documentation.

Comment 8

[Andreas Nilsson (:andreasn)]

For test pilot implementation there is https://wiki.mozilla.org/Thunderbird:UX:Test_Pilot and bug 533787

Comment 9

[Károly Marton]

Please make a Firefox 6 like TB header. One button menubar, Tabs to the top, etc... It would be great. So many "glassy" unused surface will be killed:)

Comment 10

[andipeer]

(In reply to Marton Károly from comment #9)
> Please make a Firefox 6 like TB header. One button menubar, Tabs to the top,
> etc... It would be great. So many "glassy" unused surface will be killed:)

I completely agree with you, in its current state too much space is wasted. And in my eyes it looks really ugly and "unprofessional", with all that empty space up there.

Comment 11

[Paul [pwd]]

This bug appears on AreWePrettyYet.com/Thunderbird and yet doesn't even appear to be marked NEW?

Comment 12

[Károly Marton]

I found a workaround: 2 thunderbird addons:
Ignore Aero
Hide Menubar

But firefox-like header would be better:)

Comment 13

[rsx11m]

(In reply to Paul [sabret00the] from comment #11)
> doesn't even appear to be marked NEW?

Yes, this is a valid RFE, thus confirming. The ability to hide the menus is bug 581661 and will come with Thunderbird 9.0, so this bug remains specifically for the application button.

Comment 14

[Notlost]

Attachment: Mockup of what it might look like

Comment 15

[Károly Marton]

Attachment: Another mockup: i would like something like this

Comment 16

[Paul [pwd]]

Comment on attachment 576647 [details]
Mockup of what it might look like

As per Firefox behaviour, should the button be showing, the menu bar should be hidden unless a user clicks ALT.

Comment 17

[Károly Marton]

(In reply to Paul [sabret00the] from comment #16)
> Comment on attachment 576647 [details]
> Mockup of what it might look like
> 
> As per Firefox behaviour, should the button be showing, the menu bar should
> be hidden unless a user clicks ALT.

Yes! Thats why i created my version:)

Comment 18

[rsx11m]

The button itself is probably the "easy" part, given that such an implementation already exists somewhere in Firefox that could be used as a template. The more tricky design decision should be which menu items and submenus form the current main menu bar should be mirrored in that application button and in which layout.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Those are both pretty good suggestions, and similar to the one at http://areweprettyyet.com/thunderbird/1/index.htm#

I agree that once we add the button, the menu bar should be hidden unless a user clicks ALT (or unless the user chooses to always show the menu bar, in which case I think we should hide the button).

I'm undecided as to whether we should move the tabs up to be inline with the button or not (as per Marton's mockup).  I suspect I'll have to test both out for a while, and see which one feels better.

And yes, figuring out what should be in the button is the tricky part.  :)

Thanks,
Blake.

Comment 20

[rsx11m]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #19)
> I agree that once we add the button, the menu bar should be hidden unless a
> user clicks ALT (or unless the user chooses to always show the menu bar, in
> which case I think we should hide the button).

I think that's the behavior implemented in Firefox 4.0 and later.

Comment 21

[Paul [pwd]]

Has a Test Pilot study been done to figure out what the most commonly used menu items are?

Comment 24

[ullebe1]

I think it is about time this is implemented.
personally I like Károly Marton's mockup the most, and i honestly think that this UI-update could attract a lot of users.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

(In reply to ullebe1 from comment #24)
> I think it is about time this is implemented.
> personally I like Károly Marton's mockup the most, and i honestly think that
> this UI-update could attract a lot of users.

Excellent!  I can't wait to review your first patch!  :)

If you need any help getting started, please email me, and I'll be glad to point you at the resources we have.

Thanks,
Blake.

Comment 26

[Julien Schmidt]

Do we have a list of needed menu items yet?

Comment 27

[Blake Winton (:bwinton) (:☕️)]

There are some suggestions at http://breakingtheegg.tumblr.com/post/19351338750/button-poll  We don't have a definitive list yet, but if the person implementing the feature wants to pick one and go with it, I'll be happy to tweak as necessary.

Comment 28

[Paul [pwd]]

I thought we were gonna use TP to provide the necessary data? Also with the application menu set to be removed from Firefox? Does it really make sense to have it on TB?

Comment 29

[Charles]

(In reply to Paul [sabret00the] from comment #28)
> Also with the application menu set to be removed from Firefox?

Eh? First they replaced all of the menus with an App Button (which I like, but I also use TinyMenu to give me quick/easy access to the old menu tree), now they are going to lose the App Button too? What will it be replaced with, an invisible magic mushroom?

Please provide a link to where this is being discussed, so I can see what is being planned, so I can decide if it is time to commit seppuku now and just be done with it... ;)

> Does it really make sense to have it on TB?

I *need* it, because until that is done, the most excellent 'Personal Titlebar' extension that I cannot live without in Firefox cannot be made to support Thunderbird (so until then I'm forced to continue using the 'Hide Menubar' extension in TB). The 'Personal Titlebar' extension allows me to place everything I use on the menu toolbar, then when I hide it, it automatically puts everything on the menu toolbar into the window titlebar, maximizing usable screen real estate.

I have both Firefox and Thunderbird UIs exactly like I like them, and do not want any more major changes that break them, but of course I'm ok as long as I can revert the changes and/or an extension update gives me back my UI.

Comment 30

[Charles]

(In reply to Charles from comment #29)
> I have both Firefox and Thunderbird UIs exactly like I like them, and do not
> want any more major changes that break them, but of course I'm ok as long as
> I can revert the changes and/or an extension update gives me back my UI.

Except, of course, this bug, which is the last missing piece for Thunderbird - once I can use the Personal Titlebar extension in TB, I am totally satisfied with the UI layout, then the only things left are the minor annoyances/bugs that will hopefully slowly get fixed over time)...

Comment 31

[Paul [pwd]]

Just to clarify. Firefox currently have an Application Menu at the top left which will be replaced with an Application Button at the far right of the navigation (URL) bar as part of the move to Australis.

Mockups: https://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windows7-mainWindow.html

Comment 32

[Charles]

(In reply to Paul [sabret00the] from comment #31)
> Just to clarify. Firefox currently have an Application Menu at the top left
> which will be replaced with an Application Button at the far right of the
> navigation (URL) bar as part of the move to Australis.

Will it (hopefully) still be movable though?

Comment 33

[Julien Schmidt]

There are even mockups for Thunderbird with Australis theme on http://breakingtheegg.tumblr.com/

For Windows 7:
https://s3.amazonaws.com/data.tumblr.com/tumblr_m0iwtyy6nh1qkoea4o1_1280.png

For Max OS X:
https://s3.amazonaws.com/data.tumblr.com/tumblr_m0it9bMzM51qkoea4o1_1280.png

Like every toolbar element, the menu button will be movable.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

(In reply to Paul [sabret00the] from comment #28)
> I thought we were gonna use TP to provide the necessary data?

I think we've waited a long time for that data, and I don't really want to wait even longer.  I'm happy to use that data to re-arrange the items once we get it, but let's get something in place to tweak.

> Also with the
> application menu set to be removed from Firefox? Does it really make sense
> to have it on TB?

Yes.  It'll be the Menu button, not the orange Firefox button, but it'll do essentially the same thing.

Thanks,
Blake.

Comment 35

[Charles]

Sounds great... any chance this will make the next ESR version?

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Well, no-one's posted a patch yet, so the signs aren't great.  On the other hand, the next ESR isn't until version 17, so we've still got a fair bit of time…

Comment 37

[Micah]

I would love to see this for Thunderbird!  To be honest though, it's mainly about being aesthetically pleasing to me.  The "App" Menu would provide a consistent signature/brand, if you will, of Mozilla apps on Windows. I don't know of anyone else that does this, and it makes the app instantly recognizable.  Not that appearance alone should be a driving factor for the feature, I would think it would be a consistent end-user experience with Mozilla applications on Windows.  

Anyways, my two cents & vote for the feature.

Comment 38

[Tubusy]

Advances are starting to make their way into Earlybird but we're not quite there yet. I think the UI is a sad downside to this great software. With Windows Live Essentials steadily falling to pieces this might be a good time to redouble efforts to get TB looking great. I would work on it myself if necessary but just can't at the moment.

Comment 39

[Richard Marti (:Paenglab)]

Attachment: WIP v1

My first shot for this bug. Some labels aren't localized.

Only tested under Windows (XP and Win7) but should also work under Linux and OSX. Styles are made for all but especially I don't know if the splitmenus are working under OSX.

Mike, like Bug 751527 this bug also needs a attribute to know when the menubar is hidden or not. Best would be like #messengerWindow[menuautohide="false"] and [menuactive="false"]. The first when the menubar is enabled and the second when in autohide mode and hidden.

I have problems with some menus: Find, Print, Save As, Layout and Message Body. When starting they are disabled. Only after use of the normal menus the AppButton menus are working. What is wrong? Do I need some initialization? Please can you check this?

Comment 40

[Richard Marti (:Paenglab)]

Attachment: menu screenshot

Blake, what do you think? Is this a good starting point? Is too much in this menu or is something important missing?

When should the AppButton be shown on the toolbar? Always or only when the menubar is hidden? When always, OSX can use it also. Or should OSX show the button also with the menubar and Win/Linux only with hidden menubar?

Comment 41

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 642233 [details]
menu screenshot

I think it's an excellent starting point!  f=me!

A few notes:

On OSX, it looks like https://dl.dropbox.com/u/2301433/Screenshots/AppMenu.png which isn't perfect, but I'm sure we can fix it before landing.  :)

Firefox is going to use a panel that looks a little more like the one at https://people.mozilla.com/~bwinton/australis/customization/mac/  I'm not sure if we also want to go that way, or if we should stay with the more traditional menu structure, since we have more used options.

There are a number of sugggestions for what we should include at http://breakingtheegg.tumblr.com/post/19351338750/button-poll  It would probably be worth going through them and seeing what the least-mentioned things are, and thinking about removing those.  ;)  (They will still be available through the menu, which shows up when people hit the alt key, so I'm okay with hiding them a little more than we currently do.)

And finally, thank you very much for taking on this work!  :)

Comment 42

[Magnus Melin [:mkmelin]]

Great to see this move forward! I'd ditch Layout and Headers from those as i can't think people actually use those much. What i miss is Get Messages and probably the Message menu.

Comment 43

[Richard Marti (:Paenglab)]

I wait what Blake thinks about Layout and Headers.

I think Get Messages isn't needed in this menu. We would have a redundant access to the standard "Get Mail" button on the toolbar (which is also one click less than clicking the AppButton and then clicking the menuitem).

The Message menu would make the AppButton menu complicated with the lot of entries. With the context menu on the messages the items of the Message menu are also accessible.

This is my opinion. If the majority thinks it's needed, then I'll add them.

Comment 44

[Richard Marti (:Paenglab)]

Attachment: WIP v2

Patch with styling for OSX. The other things are unchanged.

Comment 45

[Richard Marti (:Paenglab)]

Attachment: AppButton menu under OSX

Comment 46

[Thomas D. (:thomas8)]

Food for thought:

In current Firefox, the Application Button is conceptually a *full* representation of *all* existing FF menus (except one or two). Menus have just been condensed and reorganized so that more important menus have top-level entries (while everything else goes into nested submenus). Condensed e.g. by using icons (as for cut/copy/paste), or by using dualmenu buttons (e.g. print combi menu).

I strongly recommend we take the same route for TB: Do *not* exclude any menu entries (read "commands") from the App button menu(s), just reorganize them in a different nested hierarchy. Excluding menu entries will practically make them invisible and thus unavailable for most users. Forcing users to switch back to the traditional menu bar for certain commands is very annoying UX, and spoils the purpose of the menu button because such users will probably just (have to) keep using the menu bar.

I have some doubts if the App button concept really helps for TB, but taking the "Australis" route of *completely* removing many options from the App button menu will imo just make things even worse. It will also contribute to the demise of TB, because hiding the variety of those very options that make up the added benefit and customized UX of a *desktop* mail client just makes us more similar to all the web mail interfaces out there with their much more limited options compared to TB.

Comment 47

[Richard Marti (:Paenglab)]

Yeah, but have in mind FX is a "simple" program with not so much menu entries (and not all entries are in AppMenu like Page Style, Show All Tabs to mention two). TB is a lot complexer on the menu side. If we would add every menu item into the AppMenu then this would be huge and not so easy to find the needed items. We could also do the Compact menu approach with only moving the menus into the AppButton and only expose some items to the main level.

We should also have in mind a lot of menu items are also accessible through context menu. Should we really double (or triple when we also count the old menu) this items in context menu and in AppMenu.

Maybe you could create a proposal how the AppMenu could be organised.

A TestPilot survey with a menu heat map would be really helpful to know which menu items are used.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

(In reply to Richard Marti [:paenglab] from comment #47)
> A TestPilot survey with a menu heat map would be really helpful to know
> which menu items are used.

Yes, I want one of those too.

Unfortunately, that study requires a slight re-engineering of how our menus work (using the XUL command pattern as opposed to firing JS via a menuitems oncommand). I started doing that work in bug 767118, but have been pulled off to work on IM, and a few other things.

Comment 49

[Thomas D. (:thomas8)]

(In reply to Richard Marti [:paenglab] from comment #47)
> Yeah, but have in mind FX is a "simple" program with not so much menu
> entries (and not all entries are in AppMenu like Page Style, Show All Tabs
> to mention two).

Perhaps Page Style has just been forgotten... - it could easily be included in the web developer advanced menu. Show all Tabs - don't know.

> TB is a lot complexer on the menu side. If we would add
> every menu item into the AppMenu then this would be huge and not so easy to
> find the needed items. We could also do the Compact menu approach with only
> moving the menus into the AppButton and only expose some items to the main
> level.

Yes, something like "compact menu" approach + expose heat items on top-level sounds good. TB only has 7 main menus currently; my Firefox current app menu has 18 entries of which 10 include submenus, and there's free space on the bottom right side of app menu for another 6 entries. I'm not saying we necessarily have to keep all 7 main menus as top level entries of new app menu, but in terms of numbers, I don't see a problem doing that either.

> We should also have in mind a lot of menu items are also accessible through
> context menu. Should we really double (or triple when we also count the old
> menu) this items in context menu and in AppMenu.

Hmmm. Historically, I'd think it's the other way round: Context menus purposefully duplicate the main menu functionality for an alternative efficient access, mainly for mouse users. Main menus offer a central access point (more so if it's an app menu!) and a systematic overview of all commands, while context menus are scattered all over the place. Duplicating command items really isn't something unusual or bad style, on the contrary, it's usually prescribed by design rules, especially with respect to disability access. And preserving the traditional menu bar that we are all used to really isn't tripling, because you can never have app menu and menu bar at the same time. I appreciate that an app menu is somewhat fashionable and space-saving; I'm not sure it's so much more practical given TB's complex and currently well-organized menu structure.

> Maybe you could create a proposal how the AppMenu could be organised.

It's difficult and I'm not very inclined to spend my time on that because I don't need it in the first place. In Firefox, I keep reverting to the traditional menu bar because it's so much easier to find things there. Which might be habit formation, or the more complex structure of the app menu, or both.
 
> A TestPilot survey with a menu heat map would be really helpful to know
> which menu items are used.

I don't fully trust these surveys, because many average users won't participate. Furthermore, while such studies might answer the questions of which particular commands should be promoted to top-level, I doubt it can answer the basic design question of "include (almost) *all* commands somewhere in nested app menu" or not.

Comment 50

[Mihovil Stanic [:Mikeyy - L10n HR]]

Im for putting all menu options in TB appmenu button, but not like this example of OSX appmenu (attachment).

I would put them under 1 entry on right side of appmenu split and loose most of entries bellow "Print" entry.
Appmenu button first screen should have only most important and used options.

Comment 51

[Blake Winton (:bwinton) (:☕️)]

I'm not entirely sure what you mean here, Mihovil.  Could you post a screenshot?

Comment 52

[Mihovil Stanic [:Mikeyy - L10n HR]]

Attachment: TB appmenu button suggestion

Something like this. It can be 100% indentical to current menus or changed.

Comment 53

[Richard Marti (:Paenglab)]

Attachment: WIP v3

WIP v3 with every menuitem on it. I could solve almost every problem with disabled/enabled states and initialization of items except the attachment menu.

This patch is complete except the localization.

Blake, what do you think about the menu items? Is it okay or too much. Should things be arranged different? And what do you think should I ask for tracking-TB 17?

Mike, please could you have a look why the attachment menu isn't filled with the items like on the main menu? If you want you can already check if my changes in JS are okay or are such a catastrophe?

Comment 54

[Richard Marti (:Paenglab)]

Attachment: WIP v3 screenshot

Comment 55

[Micah]

Richard, the menu looks great, would it be possible to add Accounts in the right section above Activity Manager? I have several accounts setup with Thunderbird and it would be nice to get to that.  If not, it's no biggie.  I'm loving the progress!! :D

Comment 56

[Richard Marti (:Paenglab)]

Micah, I'm not sure what you mean with Accounts. Is it the Get Mails menu or do you mean the Account manager? Both are in the menu but not on top level.

I'm awating now Blake's feedback and let the menu like it is.

Comment 57

[Micah]

The Accounts Manager. It's a not a big deal, it would be available under tools anyway.

Comment 58

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 648358 [details] [diff] [review]
WIP v3

I think it's too much, yeah.  I guess, Firefox has almost the same number of items vertically, but their second half of the menu is much smaller horizontally, which makes it feel lighter.

Things I think we can make less visible:
Offline,
Layout, (in Options, perhaps?)
Compact Folders,
All of the Filter stuff (could be behind a Filter menu).

Can we try that, and make the right-side of the menu narrower and slightly blue, to match Firefox?

As for tracking TB-17, let's see how the patch progresses before we do that, but I'm hopeful.

Thanks,
Blake.

Comment 59

[Richard Marti (:Paenglab)]

Attachment: WIP v4

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #58)
> Comment on attachment 648358 [details] [diff] [review]
> WIP v3
> 
> Things I think we can make less visible:
> Offline,
> Layout, (in Options, perhaps?)
> Compact Folders,

done

> All of the Filter stuff (could be behind a Filter menu).

done, put in one splitmenu below Activity Manager.
 
> Can we try that, and make the right-side of the menu narrower and slightly
> blue, to match Firefox?

Hmm, OSX FX has no AppMenu. But it is now blue. On OSX I had to set -moz-appearance: none in normal state because they where white.

Now the patch is also fully localized. When the attachment menu problem is solved, I think it's ready for review.

Comment 60

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 648695 [details] [diff] [review]
WIP v4

When I talked about the AppMenu, I meant
https://dl.dropbox.com/u/2301433/Screenshots/MenuButtonFFWin.png
vs.
http://dl.dropbox.com/u/2301433/Screenshots/MenuButtonTBMac.png

So, I think we're close.  It looks like we need a little styling for the top and bottom of the right part of the menu, and I'm still thinking that we have too much there…

Some thoughts on how to make it feel lighter (without actually removing anything ;) -

Could we remove some of the padding from the end of the menu items (maybe only on the right half, maybe on both halves)?

Can we make the top-level "Find in this message" into just "Find"?

(Also, Mike's gone this week, so let's continue moving ahead on this, and he can check it out when he gets back.)

Thanks,
Blake.

Comment 61

[Blake Winton (:bwinton) (:☕️)]

(Also, on the Mac, I think using the font from the buttons, instead of the font from the menu, might help.  On Windows 7, there seems to be less padding, and a thinner font, and the right part of the menu doesn't have the odd artifact.)

Comment 62

[Richard Marti (:Paenglab)]

Attachment: WIP v5 (review candidate)

This patch has a slimmer AppMenu on OSX and is using font-size: 12px on every AppMenu level.

Now also tested with my first self-built TB on Linux :)

If everything would be okay I'll use this patch for review.

Comment 63

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 650110 [details] [diff] [review]
WIP v5 (review candidate)

Kairo said "It feels awkward to have the main row of entries not open directly below the button".  Which leads me to wonder "Hey, why is that?  Can't we let the menu hang off the edge, if there's room, and put the main (left) section directly beneath the button?"

Other than that, I like it.

http://dl.dropbox.com/u/2301433/Screenshots/MenuButtonTBMacSmaller.png

Comment 64

[Richard Marti (:Paenglab)]

Attachment: patch

I removed the menupopup's position="after_end". If it has on the right enough place the menu is opened normally left aligned.

I'm asking mconley for review because I added (or better duplicated) a lot of JS and I want to be sure I have nothing made wrong.

The attachment menupopup is still not working, but this is handled in bug 780200.

Does this need tests? If yes, can this made in a new bug because I'm a noob in JS and it would be better if a expert could do this.

Comment 65

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 650233 [details] [diff] [review]
patch

Sometimes I can get more than one sub-menu showing at the same time.
But other than that, I like it.  ui-r=me with that fixed.

Here's what it looks like to me:
https://dl.dropbox.com/u/2301433/Screenshots/MenuButtonTBLinux.png
https://dl.dropbox.com/u/2301433/Screenshots/MenuButtonTBWindows.png
https://dl.dropbox.com/u/2301433/Screenshots/MenuButtonTBMacSmaller.png

And hopefully Mike will be able to help you write some tests, and fix the remaining issue(s) when he gets back.

Thanks,
Blake.

Comment 66

[Blake Winton (:bwinton) (:☕️)]

(The two of you might want to play around with this patch, too.)

Comment 67

[Mihovil Stanic [:Mikeyy - L10n HR]]

Why is menu button located on the right part of screen in all pictures?
Isn't FF look and feel with tabs next to it (like second attachment in this bug) better?

Comment 68

[Jim Porter (:squib)]

(In reply to Mihovil Stanic from comment #67)
> Why is menu button located on the right part of screen in all pictures?
> Isn't FF look and feel with tabs next to it (like second attachment in this
> bug) better?

This is what Firefox is moving towards as well with the Australis theme.

Comment 69

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 650233 [details] [diff] [review]
patch

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

Hey Richard,

This looks really good! Just a few style nits and a few questions and suggestions here or there, but nothing too major.

Thanks,

-Mike

::: mail/base/content/mailWidgets.xml
@@ +2687,4 @@
>        </handler>
>      </handlers>
>    </binding>
> +  <binding id="splitmenu">

I haven't done a line-by-line comparison, but can I assume this is more or less copied verbatim from Firefox's urlbarBindings.xml?

::: mail/base/content/mailWindowOverlay.js
@@ +134,5 @@
> +  goSetAccessKey('cmd_delete', 'valueDefaultAccessKey');
> +  document.commandDispatcher.updateCommands('create-menu-edit');
> +
> +  // initialize the favorite Folder checkbox in the appmenu menu
> +  var favoriteAppFolderMenu = document.getElementById('appmenu_favoriteFolder');

We prefer let over var. On line 141 too.

@@ +198,4 @@
>  
>    // Initialize the Show Feed Summary menu
>    var viewFeedSummary = document.getElementById('viewFeedSummary');
> +  var appmenuviewFeedSummary = document.getElementById('appmenu_viewFeedSummary');

We prefer let over var. We try not to introduce new var's.

Nit - I'd prefer this variable to be named "appmenuViewFeedSummary".

@@ +308,5 @@
>  }
>  
> +function InitAppViewSortByMenu()
> +{
> +  var sortType = gFolderDisplay.view.primarySortType;

let instead of var. I'll discontinue mentioning this point now.

@@ +326,5 @@
> +  setSortByMenuItemCheckState("appmenu_sortByRecipientMenuitem", (sortType == nsMsgViewSortType.byRecipient));
> +  setSortByMenuItemCheckState("appmenu_sortByAttachmentsMenuitem", (sortType == nsMsgViewSortType.byAttachments));
> +
> +  var sortOrder = gFolderDisplay.view.primarySortOrder;
> +  var sortTypeSupportsGrouping = (sortType == nsMsgViewSortType.byAuthor ||

Style nit - I think I'd prefer this to be formatted like:

let sortTypeSupportsGrouping = (sortType == nsMsgViewSortType.byAuthor ||
                                sortType == nsMsgViewSortType.byDate ||
                                sortType == nsMsgViewSortType.byReceived ||
                                ...
                                sortType == nsMsgViewSortType.byAttachments);

@@ +471,5 @@
> +  document.getElementById("appmenu_killThread").hidden = !isNews;
> +  document.getElementById("appmenu_killSubthread").hidden = !isNews;
> +  document.getElementById("appmenu_watchThread").hidden = !isNews;
> +  document.getElementById("appmenu_cancel").hidden = !isNews;
> +

Remove extraneous newline.

@@ +503,5 @@
> +
> +  // Initialize the Open Feed Message handler menu
> +  var index = GetFeedOpenHandler();
> +  document.getElementById("appmenu_openFeedMessage")
> +          .childNodes[index].setAttribute("checked", true);

If you break before a period, you should do it for each chunk of a statement - so this should be:

document.getElementById("appmenu_openFeedMessage")
        .childNodes[index]
        .setAttribute("checked", true);

@@ +511,5 @@
> +    openRssMenu.hidden = true;
> +
> +  // Disable mark menu when we're not in a folder.
> +  document.getElementById("appmenu_markMenu").disabled = gMessageDisplay.isDummy;
> +

Nit: I don't think this newline is necessary.

@@ +634,5 @@
> +  var html_as = 0;
> +  var prefer_plaintext = false;
> +  var disallow_classes = 0;
> +  var isFeed = gFolderDisplay.selectedMessageIsFeed;
> +  const defaultIDs = ["appmenu_bodyAllowHTML",

Nit: according to the Mozilla style guide, constants should start with a k, so these should be:

kDefaultIDs and kRssIDs

@@ +642,5 @@
> +  const rssIDs = ["appmenu_bodyFeedSummaryAllowHTML",
> +                  "appmenu_bodyFeedSummarySanitized",
> +                  "appmenu_bodyFeedSummaryAsPlaintext"];
> +  var menuIDs = isFeed ? rssIDs : defaultIDs;
> +  try

I'm seeing inconsistent bracing in here...sometimes the open brace is on the same line, and sometimes on the next line.

I personally prefer same line, so I think this should be:

try {
...
} catch(e) {
...
}

@@ +671,5 @@
> +  var AsPlaintext_menuitem = document.getElementById(menuIDs[2]);
> +  var AllBodyParts_menuitem = menuIDs[3] ? document.getElementById(menuIDs[3])
> +        : null;
> +
> +  document.getElementById("appmenu_bodyAllParts").hidden = 

Trailing whitespace on this line and 677.

@@ +672,5 @@
> +  var AllBodyParts_menuitem = menuIDs[3] ? document.getElementById(menuIDs[3])
> +        : null;
> +
> +  document.getElementById("appmenu_bodyAllParts").hidden = 
> +    ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu");

No space after !

@@ +678,5 @@
> +  if (!prefer_plaintext && !html_as && !disallow_classes &&
> +      AllowHTML_menuitem)
> +    AllowHTML_menuitem.setAttribute("checked", true);
> +  else if (!prefer_plaintext && html_as == 3 && disallow_classes > 0 &&
> +      Sanitized_menuitem)

Sanitized_menuitem should be indented so that its first character is lined up with the ! in !prefer_plaintext on the line above.

@@ +681,5 @@
> +  else if (!prefer_plaintext && html_as == 3 && disallow_classes > 0 &&
> +      Sanitized_menuitem)
> +    Sanitized_menuitem.setAttribute("checked", true);
> +  else if (prefer_plaintext && html_as == 1 && disallow_classes > 0 &&
> +      AsPlaintext_menuitem)

Same as above wrt indenting - AsPlaintext_menuitem should be indented so that its first character is lined up with the first p in prefer_plaintext on the line above.

@@ +684,5 @@
> +  else if (prefer_plaintext && html_as == 1 && disallow_classes > 0 &&
> +      AsPlaintext_menuitem)
> +    AsPlaintext_menuitem.setAttribute("checked", true);
> +  else if (!prefer_plaintext && html_as == 4 && !disallow_classes &&
> +      AllBodyParts_menuitem)

Same as above.

::: mail/base/content/mailWindowOverlay.xul
@@ +1702,5 @@
>                    accesskey="&importCmd.accesskey;"
>                    oncommand="toImport();"/>
>          <menuitem id="javascriptConsole" label="&errorConsoleCmd.label;" accesskey="&errorConsoleCmd.accesskey;" key="key_errorConsole" oncommand="toJavaScriptConsole();"/>
> +        <menuitem id="sanitizeHistory"
> +                  label="&clearRecentHistory.label;"                       

While you're here, can you remove the trailing whitespace?

@@ +2029,5 @@
> +                   class="toolbarbutton-1"
> +                   label="&appmenuButton.label;"
> +                   tooltiptext="&appmenuButton.tooltip;">
> +      <menupopup id="appmenu-popup"
> +                 onpopupshowing="file_init();

Hm - we should write a new function that calls all of these, and then have onpopupshowing call that function, as opposed to doing them all inline like this.

@@ +2160,5 @@
> +          <splitmenu id="appmenu_print"
> +                     iconic="true"
> +                     label="&printCmd.label;"
> +                     key="printKb"
> +                     command="PrintUtils.print();">

Why is this not using cmd_print?

@@ +2165,5 @@
> +              <menupopup>
> +                <menuitem id="appmenu_print_popup"
> +                          class="menuitem-iconic"
> +                          label="&printCmd.label;"
> +                          command="PrintUtils.print();"

Same here - cmd_print?

@@ +2172,5 @@
> +                          label="&printPreviewCmd.label;"
> +                          command="cmd_printpreview"/>
> +                <menuitem id="appmenu_printSetup"
> +                          label="&printSetupCmd.label;"
> +                          oncommand="PrintUtils.showPageSetup();"/>

Shouldn't this be command="cmd_printSetup" ?

@@ +2348,5 @@
> +              <menuseparator id="appmenu_fileMenuAfterCloseSeparator"/>
> +              <menu id="appmenu_getNewMsgFor"
> +                    label="&getNewMsgForCmd.label;"
> +                    oncommand="MsgGetMessagesForAccount(event.target._folder)">
> +                <menupopup type="folder" 

Trailing whitespace.

@@ +2361,5 @@
> +                            command="cmd_getNewMessages"/>
> +                  <menuseparator/>
> +                </menupopup>
> +              </menu>
> +              <menuitem id="appmenu_getnextnmsg"

I think appmenu_getnextnmsg should be:

appmenu_getNextNMsgs

and appmenu_sendunsentmsgs should be:

appmenu_sendUnsentMsgs

@@ +2378,5 @@
> +              <menuitem id="appmenu_renameFolder"
> +                        label="&renameFolder.label;"
> +                        command="cmd_renameFolder"/>
> +              <menuseparator id="appmenu_fileMenuAfterRenameSeparator"/>
> +              <menuitem id="appmenu_compactFolder" 

Trailing whitespace

@@ +2517,5 @@
> +              <menu id="appmenu_viewMessageViewMenu"
> +                    label="&msgsMenu.label;"
> +                    command="mailHideMenus"
> +                    oncommand="ViewChangeByMenuitem(event.target);">
> +              <menupopup id="appmenu_viewMessagePopup"

The contents of the menu tag should be indented 2 spaces.

@@ +2640,5 @@
> +              </menupopup>
> +            </menu>
> +            <menu id="appmenu_viewFeedSummary"
> +                  label="&bodyMenuFeed.label;">
> +              <menupopup id="appmenu_viewFeedSummaryPopupMenu" 

Trailing whitespace on a bunch of these lines.

@@ +2724,5 @@
> +                  </menupopup>
> +                </rule>
> +              </template>
> +              <menupopup>
> +              <menu label="&charsetMenuAutodet.label;"

The contents of this menupopup should be indented 2 spaces.

@@ +2733,5 @@
> +                    <menuitem type="radio" name="detectorGroup" checked="rdf:http://home.netscape.com/NC-rdf#Checked" uri="..." label="rdf:http://home.netscape.com/NC-rdf#Name"/>
> +                    </menupopup>
> +                  </rule>
> +                </template>
> +                <menupopup>

Instead of <menupopup></menupopup>, you can just use <menupopup/>

@@ +2741,5 @@
> +                    datasources="rdf:charset-menu" ref="NC:BrowserMoreCharsetMenuRoot">
> +                <template>
> +                  <rule>
> +                    <menupopup>
> +                    <menuitem uri="..." label="rdf:http://home.netscape.com/NC-rdf#Name"/>

This node should be indented 2 spaces.

@@ +2751,5 @@
> +                        datasources="rdf:charset-menu" ref="NC:BrowserMore1CharsetMenuRoot">
> +                    <template>
> +                      <rule>
> +                        <menupopup>
> +                        <menuitem uri="..." label="rdf:http://home.netscape.com/NC-rdf#Name"/>

This node should be indented 2 spaces.

@@ +2755,5 @@
> +                        <menuitem uri="..." label="rdf:http://home.netscape.com/NC-rdf#Name"/>
> +                        </menupopup>
> +                      </rule>
> +                    </template>
> +                    <menupopup>

Same as above - this empty menupopup can be written as <menupopup/>

Applies to all below as well.

@@ +2763,5 @@
> +                        datasources="rdf:charset-menu" ref="NC:BrowserMore2CharsetMenuRoot">
> +                    <template>
> +                      <rule>
> +                        <menupopup>
> +                        <menuitem uri="..." label="rdf:http://home.netscape.com/NC-rdf#Name"/>

This node should be indented 2 spaces. Applies to all below as well.

@@ +2899,5 @@
> +          <menuseparator id="appmenu_goRecentlyClosedTabsSeparator"/>
> +          <menuitem id="appmenu_goStartPage"
> +                    label="&startPageCmd.label;"
> +                    command="cmd_goStartPage"/>
> +        </menupopup>

If you see two closing nodes like this lined up, we know that the indentation screwed up somewhere.

@@ +2948,5 @@
> +                      command="cmd_openMessage"/>
> +            <menuitem id="appmenu_openConversationMenuitem"
> +                      label="&openConversationCmd.label;"
> +                      command="cmd_openConversation"/>
> +            <menu id="appmenu_openFeedMessage" 

Trailing whitespace on the next bit of code.

@@ +3158,5 @@
> +              </menupopup>
> +          </splitmenu>
> +        </vbox>
> +      </hbox>
> +      </menupopup>

Indentation must have screwed up if these two closing nodes are aligned.

@@ +3182,3 @@
>  #else
> +           defaultset="button-getmsg,button-newmsg,button-chat,button-address,separator,
> +                       button-tag,qfb-show-filter-bar,spring,gloda-search,button-appmenu">

We're going to need migration code as well to append button-appmenu to the end of customized mail-bar3's.

::: mail/base/content/messageWindow.js
@@ +515,4 @@
>    var openMail3Pane_menuitem = document.getElementById('tasksMenuMail');
>    if (openMail3Pane_menuitem)
>      openMail3Pane_menuitem.removeAttribute("hidden");
> +  var openMail3Pane_appmenuitem = document.getElementById('appmenu_tasksMenuMail');

Should introduce let instead of var through this file.

@@ +525,4 @@
>    var message_menuitem=document.getElementById('menu_showMessage');
>    if (message_menuitem)
>      message_menuitem.setAttribute("hidden", "true");
> +  

Whitespace

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +584,5 @@
> +<!ENTITY appmenuButton.tooltip "Displays the Application Menu">
> +
> +<!-- AppMenu Popup -->
> +<!ENTITY appmenuNewMsgCmd.label "New Message">
> +<!ENTITY appmenuNewContactCmd.label "Address Book Contact…">

Why are we unable to re-use the strings for the original menu items?

Comment 70

[Richard Marti (:Paenglab)]

Attachment: patch v2

Patch addressing all comments except:

> 
> We prefer let over var. On line 141 too.
> 

I've done this except on msgHdrViewOverlay.js line 621. When I change this I end in a not working interface. Error in console:
Error: TypeError: redeclaration of variable node
Source file: chrome://messenger/content/msgHdrViewOverlay.js
Line: 621, Column: 12
Source code:
        let node = document.getElementById("appmenu_fileAttachmentMenu")


> 
> Instead of <menupopup></menupopup>, you can just use <menupopup/>
> 

This doesn't work -> completely messed interface. Maybe because it's a RDF syntax. I also tried <menupopup /> without luck.

Now to your questions:

> 
> ::: mail/base/content/mailWidgets.xml
> @@ +2687,4 @@
> >        </handler>
> >      </handlers>
> >    </binding>
> > +  <binding id="splitmenu">
> 
> I haven't done a line-by-line comparison, but can I assume this is more or
> less copied verbatim from Firefox's urlbarBindings.xml?
> 

Correct, this was 1:1 copied from Firefox.

> 
> ::: mail/locales/en-US/chrome/messenger/messenger.dtd
> @@ +584,5 @@
> > +<!ENTITY appmenuButton.tooltip "Displays the Application Menu">
> > +
> > +<!-- AppMenu Popup -->
> > +<!ENTITY appmenuNewMsgCmd.label "New Message">
> > +<!ENTITY appmenuNewContactCmd.label "Address Book Contact…">
> 
> Why are we unable to re-use the strings for the original menu items?

"New Message" is new. It exists only "New" or "Message"
I've added "Address Book Contact…" to not include the whole abMainWindow.dtd for only this entity. If this doesn't matter I can add the dtd.

I've added to this patch the migration code in mailMigrator.js.
I also found a missing initialization in chat-messenger-overlay.js (@@ -330,6 +330,11 @@).

What would be good, but I didn't found how to do it, is to hide the menubar by default (only on main window). This should only be on Windows and Linux as OSX has his menu bar not in the window.

Comment 71

[Mike Conley (:mconley) (:⚙️)]

(In reply to Richard Marti [:paenglab] from comment #70)
> Created attachment 652367 [details] [diff] [review]
> patch v2
> 
> Patch addressing all comments except:
> 
> > 
> > We prefer let over var. On line 141 too.
> > 
> 
> I've done this except on msgHdrViewOverlay.js line 621. When I change this I
> end in a not working interface. Error in console:
> Error: TypeError: redeclaration of variable node
> Source file: chrome://messenger/content/msgHdrViewOverlay.js
> Line: 621, Column: 12
> Source code:
>         let node = document.getElementById("appmenu_fileAttachmentMenu")
> 

Ah, that's because you're redeclaring a node that was declared earlier in that function. Just re-use the same node, without let or var. Same on line 2607.

> > 
> > Instead of <menupopup></menupopup>, you can just use <menupopup/>
> > 
> 
> This doesn't work -> completely messed interface. Maybe because it's a RDF
> syntax. I also tried <menupopup /> without luck.

That's so weird. There are plenty of examples around in that file where that style of tagging is used.  :/

> "New Message" is new. It exists only "New" or "Message"
> I've added "Address Book Contact…" to not include the whole abMainWindow.dtd
> for only this entity. If this doesn't matter I can add the dtd.
> 

Naw, it's fine, just curious. Leave it as it is.

Comment 72

[Richard Marti (:Paenglab)]

Attachment: patch v3

(In reply to Mike Conley (:mconley) from comment #71)
> (In reply to Richard Marti [:paenglab] from comment #70)
> > I've done this except on msgHdrViewOverlay.js line 621. When I change this I
> > end in a not working interface. Error in console:
> > Error: TypeError: redeclaration of variable node
> > Source file: chrome://messenger/content/msgHdrViewOverlay.js
> > Line: 621, Column: 12
> > Source code:
> >         let node = document.getElementById("appmenu_fileAttachmentMenu")
> > 
> 
> Ah, that's because you're redeclaring a node that was declared earlier in
> that function. Just re-use the same node, without let or var. Same on line
> 2607.

I'm using now appmenunode instead of node. I hope this is also okay.

> > > 
> > > Instead of <menupopup></menupopup>, you can just use <menupopup/>
> > > 
> > 
> > This doesn't work -> completely messed interface. Maybe because it's a RDF
> > syntax. I also tried <menupopup /> without luck.
> 
> That's so weird. There are plenty of examples around in that file where that
> style of tagging is used.  :/

This is the only area where this is like this. I had for this also to add the rdf-syntax. This part is also copied from Firefox like it is.

Comment 73

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 652367 [details] [diff] [review]
patch v2

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

You uploaded a new version before I finished my review, but I think these comments should still apply. :)

::: mail/base/content/hiddenWindow.js
@@ +34,5 @@
> +      'zoomWindow', 'appmenu_replyMainMenu', 'appmenu_replyNewsgroupMainMenu',
> +      'appmenu_newFolder', 'appmenu_newMailAccountMenuItem', 'appmenu_close',
> +      'appmenu_newAccountMenuItem', 'appmenu_saveAs', 'appmenu_saveAsFile',
> +      'appmenu_newVirtualFolder', 'appmenu_viewBodyMenu', 'appmenu_goNextMenu',
> +      'appmenu_findAgainCmd', 'appmenu_sendunsentmsgs', 'appmenu_charsetMenu',

This needs to be updated to appmenu_sendUnsetMsgs.

Why isn't appmenu_getNextNMsgs in here?

::: mail/base/content/mailWindowOverlay.js
@@ +645,5 @@
> +  const kRssIDs = ["appmenu_bodyFeedSummaryAllowHTML",
> +                   "appmenu_bodyFeedSummarySanitized",
> +                   "appmenu_bodyFeedSummaryAsPlaintext"];
> +  let menuIDs = isFeed ? kRssIDs : kDefaultIDs;
> +  try

try {

@@ +663,5 @@
> +    if (disallow_classes > 0)
> +      gDisallow_classes_no_html = disallow_classes;
> +    // else gDisallow_classes_no_html keeps its inital value (see top)
> +  } catch (ex) {
> +    dump("failed to get the body plaintext vs. HTML prefs\n");

We shouldn't dump - use Components.utils.reportError instead.

@@ +670,5 @@
> +  let AllowHTML_menuitem = document.getElementById(menuIDs[0]);
> +  let Sanitized_menuitem = document.getElementById(menuIDs[1]);
> +  let AsPlaintext_menuitem = document.getElementById(menuIDs[2]);
> +  let AllBodyParts_menuitem = menuIDs[3] ? document.getElementById(menuIDs[3])
> +        : null;

Please indent this line to put the : underneath the ?

@@ +673,5 @@
> +  let AllBodyParts_menuitem = menuIDs[3] ? document.getElementById(menuIDs[3])
> +        : null;
> +
> +  document.getElementById("appmenu_bodyAllParts").hidden =
> +    ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu");

No space after !

::: mail/base/content/messageWindow.js
@@ +526,4 @@
>    if (message_menuitem)
>      message_menuitem.setAttribute("hidden", "true");
>  
> +  let message_menuitem=document.getElementById('appmenu_showMessage');

Space on either side of the =

@@ +533,5 @@
>    var folderPane_menuitem=document.getElementById('menu_showFolderPane');
>    if (folderPane_menuitem)
>      folderPane_menuitem.setAttribute("hidden", "true");
>  
> +  let folderPane_menuitem=document.getElementById('appmenu_showFolderPane');

Space on either side of the =.

You're also redefining vars that have already been defined. Just do:

folderPane_menuitem = document.getElementById('appmenu_showFolderPane');

Same goes for the code below.

::: mail/base/modules/mailMigrator.js
@@ +237,5 @@
> +          if (currentSet
> +              && currentSet.indexOf("button-appmenu") == -1) {
> +
> +            dirty = true;
> +            // Otherwise, just put the chat button at the end.

This comment no longer applies, and can be removed.

@@ +240,5 @@
> +            dirty = true;
> +            // Otherwise, just put the chat button at the end.
> +            currentSet = currentSet + ",button-appmenu";
> +            this._setPersist(barResource, currentSetResource, currentSet);
> +          }

We're going to have to add some utility code to make the menubar autohide by default. Come to think of it, maybe we should deal with this migration stuff in a separate bug.

Comment 74

[Richard Marti (:Paenglab)]

Attachment: patch v4

(In reply to Mike Conley (:mconley) from comment #73)
> Comment on attachment 652367 [details] [diff] [review]
> patch v2
> 
> Review of attachment 652367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You uploaded a new version before I finished my review, but I think these
> comments should still apply. :)
> 
> ::: mail/base/content/hiddenWindow.js
> @@ +34,5 @@
> > +      'zoomWindow', 'appmenu_replyMainMenu', 'appmenu_replyNewsgroupMainMenu',
> > +      'appmenu_newFolder', 'appmenu_newMailAccountMenuItem', 'appmenu_close',
> > +      'appmenu_newAccountMenuItem', 'appmenu_saveAs', 'appmenu_saveAsFile',
> > +      'appmenu_newVirtualFolder', 'appmenu_viewBodyMenu', 'appmenu_goNextMenu',
> > +      'appmenu_findAgainCmd', 'appmenu_sendunsentmsgs', 'appmenu_charsetMenu',
> 
> This needs to be updated to appmenu_sendUnsetMsgs.

Done

> Why isn't appmenu_getNextNMsgs in here?

getNextNMsgs from main menu is also not in this list.

> ::: mail/base/content/mailWindowOverlay.js
> @@ +645,5 @@
> > +  const kRssIDs = ["appmenu_bodyFeedSummaryAllowHTML",
> > +                   "appmenu_bodyFeedSummarySanitized",
> > +                   "appmenu_bodyFeedSummaryAsPlaintext"];
> > +  let menuIDs = isFeed ? kRssIDs : kDefaultIDs;
> > +  try
> 
> try {

Done

> @@ +663,5 @@
> > +    if (disallow_classes > 0)
> > +      gDisallow_classes_no_html = disallow_classes;
> > +    // else gDisallow_classes_no_html keeps its inital value (see top)
> > +  } catch (ex) {
> > +    dump("failed to get the body plaintext vs. HTML prefs\n");
> 
> We shouldn't dump - use Components.utils.reportError instead.

Done. I removed the newline. Is this okay or should this stay?

> @@ +670,5 @@
> > +  let AllowHTML_menuitem = document.getElementById(menuIDs[0]);
> > +  let Sanitized_menuitem = document.getElementById(menuIDs[1]);
> > +  let AsPlaintext_menuitem = document.getElementById(menuIDs[2]);
> > +  let AllBodyParts_menuitem = menuIDs[3] ? document.getElementById(menuIDs[3])
> > +        : null;
> 
> Please indent this line to put the : underneath the ?

Done

> @@ +673,5 @@
> > +  let AllBodyParts_menuitem = menuIDs[3] ? document.getElementById(menuIDs[3])
> > +        : null;
> > +
> > +  document.getElementById("appmenu_bodyAllParts").hidden =
> > +    ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu");
> 
> No space after !

Done

> ::: mail/base/content/messageWindow.js
> @@ +526,4 @@
> >    if (message_menuitem)
> >      message_menuitem.setAttribute("hidden", "true");
> >  
> > +  let message_menuitem=document.getElementById('appmenu_showMessage');
> 
> Space on either side of the =
> 
> @@ +533,5 @@
> >    var folderPane_menuitem=document.getElementById('menu_showFolderPane');
> >    if (folderPane_menuitem)
> >      folderPane_menuitem.setAttribute("hidden", "true");
> >  
> > +  let folderPane_menuitem=document.getElementById('appmenu_showFolderPane');
> 
> Space on either side of the =.

Done

> You're also redefining vars that have already been defined. Just do:
> 
> folderPane_menuitem = document.getElementById('appmenu_showFolderPane');
> 
> Same goes for the code below.

Done on all where the vars where the same.

> ::: mail/base/modules/mailMigrator.js
> @@ +237,5 @@
> > +          if (currentSet
> > +              && currentSet.indexOf("button-appmenu") == -1) {
> > +
> > +            dirty = true;
> > +            // Otherwise, just put the chat button at the end.
> 
> This comment no longer applies, and can be removed.

I changed to // Put the AppMenu button at the end.

> @@ +240,5 @@
> > +            dirty = true;
> > +            // Otherwise, just put the chat button at the end.
> > +            currentSet = currentSet + ",button-appmenu";
> > +            this._setPersist(barResource, currentSetResource, currentSet);
> > +          }
> 
> We're going to have to add some utility code to make the menubar autohide by
> default. Come to think of it, maybe we should deal with this migration stuff
> in a separate bug.

I'll let it in until you say remove it. If the button appears immediately after check-in this could help find bugs faster.

Comment 75

[Magnus Melin [:mkmelin]]

Comment on attachment 652586 [details] [diff] [review]
patch v4

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

::: mail/base/content/mailWindowOverlay.js
@@ +658,5 @@
> +      gDisallow_classes_no_html = disallow_classes;
> +    // else gDisallow_classes_no_html keeps its inital value (see top)
> +  } catch (ex) {
> +    Components.utils.reportError("failed to get the body plaintext vs. HTML prefs");
> +  }

Actually, why is this in try/catch to being with? Those prefs all have default values so if getting the pref doesn't work you hardly get a working app anyways.
For logging like this it helps to diagnose later if you log the ex object too ( + ex).

Comment 76

[Richard Marti (:Paenglab)]

I'm sorry, I'm a cargo-cult programmer for JS. I've copied this code some lines above. For this I wrote, mconley check good what I wrote (or better copied).

If this catch isn't needed, I can remove it.

Comment 77

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 652586 [details] [diff] [review]
patch v4

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

::: mail/base/content/mailWindowOverlay.js
@@ +658,5 @@
> +      gDisallow_classes_no_html = disallow_classes;
> +    // else gDisallow_classes_no_html keeps its inital value (see top)
> +  } catch (ex) {
> +    Components.utils.reportError("failed to get the body plaintext vs. HTML prefs");
> +  }

Actually, yeah - Magnus has a good point here. The whole try/catch thing isn't really necessary, now that I take a second look at it.

Comment 78

[Richard Marti (:Paenglab)]

Can I remove this lines?

+  try {

and

+    if (disallow_classes > 0)
+      gDisallow_classes_no_html = disallow_classes;
+    // else gDisallow_classes_no_html keeps its inital value (see top)
+  } catch (ex) {
+    Components.utils.reportError("failed to get the body plaintext vs. HTML prefs");

or do I need some of this lines?

Comment 79

[Mike Conley (:mconley) (:⚙️)]

Attachment: Menubar autohide migration - apply on top of patch v4

This patch should be applied on top of patch v4 (and then merged in to the next version of Richard's patch).

This patch simply adds autohide="true" to the main menubar, and then adds some Mozmill tests to make sure our migrations stick.

Comment 80

[Mike Conley (:mconley) (:⚙️)]

(In reply to Richard Marti [:paenglab] from comment #78)
> Can I remove this lines?
> 
> +  try {
> 
> and
> 
> +    if (disallow_classes > 0)
> +      gDisallow_classes_no_html = disallow_classes;
> +    // else gDisallow_classes_no_html keeps its inital value (see top)
> +  } catch (ex) {
> +    Components.utils.reportError("failed to get the body plaintext vs. HTML
> prefs");
> 
> or do I need some of this lines?

Remove these:

try {

and

} catch(ex) {
  Components.utils.reportError("failed to get the body plaintext vs. HTML prefs");
}

Everything else should stay.

Comment 81

[Richard Marti (:Paenglab)]

Attachment: patch v5

Patch addressing the comments and Menubar autohide migration with tests from mconley addded.

Comment 82

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 653793 [details] [diff] [review]
patch v5

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

Just two nits, and then I'm happy.

Great work, Richard!

::: mail/base/content/hiddenWindow.js
@@ +40,5 @@
> +      'appmenu_properties', 'appmenu_MessagePaneLayout', 'appmenu_showMessage',
> +      'appmenu_showFolderPane', 'appmenu_FolderViews', 'appmenu_viewSortMenu',
> +      'appmenu_groupBySort', 'appmenu_viewMessageViewMenu', 'appmenu_subscribe',
> +      'appmenu_viewMessagesMenu', 'appmenu_expandAllThreads', 'appmenu_findCmd',
> +      'appmenu_collapseAllThreads', 'appmenu_viewheadersmenu', 'appmenu_find',

viewHeadersMenu

::: mail/base/content/mailWindowOverlay.js
@@ +641,5 @@
> +  const kRssIDs = ["appmenu_bodyFeedSummaryAllowHTML",
> +                   "appmenu_bodyFeedSummarySanitized",
> +                   "appmenu_bodyFeedSummaryAsPlaintext"];
> +  let menuIDs = isFeed ? kRssIDs : kDefaultIDs;
> +    // Get prefs

The indentation for lines 645 - 658 needs to be corrected.

::: mail/base/content/mailWindowOverlay.xul
@@ +2590,5 @@
> +                              command="cmd_collapseAllThreads"/>
> +                  </menupopup>
> +                </menu>
> +                <menuseparator id="appmenu_viewAfterThreadsSeparator"/>
> +                <menu id="appmenu_viewheadersmenu"

This should be appmenu_viewHeadersMenu

Comment 83

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Patch for check-in.

Comment 84

[Mike Conley (:mconley) (:⚙️)]

Running it through try, along with the patch for bug 780200: https://tbpl.mozilla.org/?tree=Try&rev=0e41187ee9d5

Comment 85

[Mike Conley (:mconley) (:⚙️)]

Ok, we caught 1 failure - glad we ran it through the tests!

This failure is caused by a regression with the defaultset of mail-bar3 - the tag button is missing when resetting to the default set.

This is because the defaultset attribute in mailWindowOverlay.xul was broken up over two lines. That shouldn't happen, it should be a single line.

Richard, can you update the patch? I think then we can safely commit.

Comment 86

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 653848 [details] [diff] [review]
patch for check-in

Hindsight r- until mail-bar3's defaultset is changed to one line so that the tag button goes back.

Comment 87

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Patch which should pass the tests

Comment 88

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/af7232ffcac7

Comment 89

[:Irving Reid (No longer working on Firefox)]

This checkin causes a JavaScript error message (and breaks the hidden window on Mac) 

JavaScript error: chrome://messenger/content/hiddenWindow.js, line 34: missing ] after element list


due to a missing comma at the end of hiddenWindow.js line 33. Filed bug 785081

Comment 90

[:aceman]

This broke the display of the "Previous Msg" button icon. When in the disabled state, it shows this new TB button icon, instead of the proper icon (blue up arrow). Can be seen on Linux (gnomestripe theme). Is there a bug for that?

Comment 91

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #90)
> This broke the display of the "Previous Msg" button icon. When in the
> disabled state, it shows this new TB button icon, instead of the proper icon
> (blue up arrow). Can be seen on Linux (gnomestripe theme). Is there a bug
> for that?

Not that I know. Please file a bug and CC me to it.

Comment 92

[Mihovil Stanic [:Mikeyy - L10n HR]]

New menu doesn't show keyboard ACCESS KEYS next to buttons.

Comment 93

[Richard Marti (:Paenglab)]

The Firefox ButtonMenu is also not showing them.

Comment 94

[Mihovil Stanic [:Mikeyy - L10n HR]]

Well, I guess that isn't good also.
It's harder for new users to even know about them since they must turn on old menu to see access keys.

Comment 95

[rsx11m]

Is it possible to actually navigate through the application-button menu with access keys? Clicking on the button with the mouse then releasing it keeps the menu open, but it will close again when hitting the ALT key afterwards (talking Windows).

Comment 96

[Richard Marti (:Paenglab)]

Not as I know. But when you hit the ALT button then the main menu appears and you can navigate as before. The AppMenu is more for mouse users.

Comment 97

[rsx11m]

Makes sense, in that case access keys would be redundant as they can't be used anyway.

(In reply to Richard Marti [:Paenglab] from comment #96)
> But when you hit the ALT button then the main menu appears
> and you can navigate as before.

Yes on Windows, but not on Linux (that's covered in bug 790713 already).

Comment 98

[Serg Iv]

I set mail.chat.enabled to false and still can see menu entries for chats. Is that by design?

Comment 99

[Blake Winton (:bwinton) (:☕️)]

Hmm.  That doesn't sound quite right.  Florian?  Richard?  Anything we can do here to fix this?

Comment 100

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton) from comment #99)
> Hmm.  That doesn't sound quite right.  Florian?  Richard?  Anything we can
> do here to fix this?

The list at http://hg.mozilla.org/comm-central/annotate/ef4833cf43da/mail/components/im/content/chat-messenger-overlay.js#l977 should also contain "appmenu_goChat" and "appmenu_goChatSeparator"

And as I looked at the patch to find that, I also saw that http://hg.mozilla.org/comm-central/annotate/ef4833cf43da/mail/components/im/content/chat-messenger-overlay.js#l333 should instead just have added an || to the test at http://hg.mozilla.org/comm-central/annotate/ef4833cf43da/mail/components/im/content/chat-messenger-overlay.js#l328

Comment 101

[Serg Iv]

Just to make it clear I meant "Go -> Chat" and "Tools -> Chat status"

Comment 102

[Florian Quèze [:florian]]

appmenu_imAccountsStatus and appmenu_afterChatSeparator are also missing in the list.

Comment 103

[Serg Iv]

Should this bug be reopened or should we file another one?

Comment 104

[Blake Winton (:bwinton) (:☕️)]

File another one, please!

(And cc Florian, Paenglab, and I… :)

Thanks,
Blake.

Comment 105

[Serg Iv]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=814414

Comment 106

[ricuccimichele]

i think that the appmenu button in windows OS should be like firefox's button (of course with blue colour instead of orange). I think it would be more user-friendly and could possibly become a sort of signature for mozilla applications.

Comment 107

[Jim Porter (:squib)]

(In reply to ricuccimichele from comment #106)
> i think that the appmenu button in windows OS should be like firefox's
> button (of course with blue colour instead of orange). I think it would be
> more user-friendly and could possibly become a sort of signature for mozilla
> applications.

Firefox is going to be moving towards what Thunderbird has now, not the other way around. Thunderbird just managed to finish the UI changes first.

Comment 108

[ricuccimichele]

(In reply to Jim Porter (:squib) from comment #107)
> Firefox is going to be moving towards what Thunderbird has now, not the
> other way around. Thunderbird just managed to finish the UI changes first.

Is it a good idea? I don't know, personally I think that windows users used to find menus on the top left, not on the right. I still can't get used to actual thunderbird menu, and you gave me a terrible news telling me that firefox will be using that menu too. Can you link me a discussion or something about this decision? Because I really can't explain the reason why to do that

Comment 109

[Jim Porter (:squib)]

Here is the feature page for Firefox's appmenu button: https://wiki.mozilla.org/Features/Desktop/Panel_Menu

If you'd rather the button be on the left, you can change it. Just right-click the toolbar and then drag the appmenu button wherever you like.

Comment 110

[Richard Marti (:Paenglab)]

If you want a blue AppButton, you can install https://addons.mozilla.org/thunderbird/addon/before-tabs-toolbar/ and move the button to the left of the tabs.