583386 - Implement latest Firefox Menu design

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: i5 mockup of the Firefox menu

This bug covers all of the work to implement the Firefox menu detailed in the i5 mockup.  By tracking this specific set of changes with a single bug, we can have a centralized place for discussion, and it can be set to blog a particular beta release.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Three caveats to the attachment and the scope of this bug:

1) In the send menu, the hypothetical social services don't need to be added in this bug
2) The Sync item and menu will likely be added in a separate bug
3) The single menu item "Options > Interface..." can be replaced with the current set of commands (everything in the current customize menu, minus the add-ons manager).

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Beltzner: I would like this bug to block Beta 4

Comment 6

[Peter Henkel [:Terepin]]

Alex, what is the relation between this bug and bug 571784? Should I change the dependancy list?

Comment 8

[Peter Henkel [:Terepin]]

Question: Looking on mockup, specificaly on Edit menu, does this mean that edit tray was abandoned?

Comment 9

[Tiago Sá]

Where should the "work offline" go? It's still not reliable enough, I believe, that it can be disabled completely. And there's no other way to browse offline when there's an internet connection.

Comment 11

[Peter Henkel [:Terepin]]

(In reply to comment #9)
> Where should the "work offline" go? It's still not reliable enough, I believe,
> that it can be disabled completely. And there's no other way to browse offline
> when there's an internet connection.

Work offline will be removed.

Comment 12

[imradyurrad]

(In reply to comment #9)
> Where should the "work offline" go? It's still not reliable enough, I believe,
> that it can be disabled completely. And there's no other way to browse offline
> when there's an internet connection.

Disconnect internet?

Comment 13

[imradyurrad]

Shouldn't "exit" be the last menu item?

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

Looking at the plan in comment #0 unless I've missed it, been over it several times, and I don't see anyplace yet where menu item:  troubleshooting information is going to be implemented/placed.  

Dropping it already or oversight ?

Comment 16

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex, what is the relation between this bug and bug 571784? Should I change the
>dependancy list?

I set some of those to depend on the project bug itself, and duped the windows 7 visual style one over to this bug.

>Question: Looking on mockup, specificaly on Edit menu, does this mean that edit
>tray was abandoned?

yeah, it turns out the data in the test pilot study was including context menu actions for edit commands, which caused their numbers to be so high.  Once we looked at actual edit menu interactions, it became clear that we didn't really need to use primary chrome.  Also, with tabs on the screen edge for maximized mode, the edit tray was going to take up too much of the tab strip.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

>Looking at the plan in comment #0 unless I've missed it, been over it several
>times, and I don't see anyplace yet where menu item:  troubleshooting
>information is going to be implemented/placed.  

Moved to the bottom of the home tab, here is a full list of what's happening with each menu item:

http://people.mozilla.com/~faaborg/files/20100718-firefoxButtonDetails/menuMigration-i1.png

from http://blog.mozilla.com/faaborg/2010/07/18/details-about-the-firefox-button/
(now slightly out of date)

Comment 19

[Luke Iliffe (Harlequin99)]

Have heard in several bugs and blog posts a push to eliminate all redundancy in the UI for FF4 (just one example e.g. bug 575487 comment 19). This is something I am looking forward to. However in i5 of the firefox menu the clear recent history appears under both 'Start private browsing' and the history sub menus. 

Intended? 

It appears to me to fit better semantically under the History menu. This would remove all sub menus from the Start Private Browsing menu, something that seems to fit better with a very specific command. With i5 clear recent history would 
be available in 3 places counting the traditional tools menu if alt+T was pressed.

Comment 20

[Jonathan Haas]

I suggest making either a real/traditional sub menu out of the "Edit" command, or just removing it.

Issues with the mockup:
- This list of (toolbar) icons in a menu looks unusual and weird. 
- Especially new users who aren't familiar with the context menu or keyboard shortcuts will probably be afraid of using it or just don't identify these icons as their usual command.
- Localization issue: "Edit" is short, but in German for example it would be "Bearbeiten", breaking the width of the menu.
- As you already said, menu item usage of these items is very low. We shouldn't add "clutter icons" to the menu only too add some mostly unused items.

Comment 21

[imradyurrad]

(In reply to comment #20)

Why remove it if it'll just be replaced with a submenu? That adds an extra click.
The icons though do need some work.

Comment 22

[Peter Henkel [:Terepin]]

(In reply to comment #13)
> Shouldn't "exit" be the last menu item?

No, because Developer Tools and Extension Menu Item are in different section.

Comment 23

[imradyurrad]

(In reply to comment #22)
> (In reply to comment #13)
> > Shouldn't "exit" be the last menu item?
> 
> No, because Developer Tools and Extension Menu Item are in different section.

They can be positioned on top of the exit button. Consistency>aesthetics

Comment 24

[Peter Henkel [:Terepin]]

Great idea. And then by adding extension items one by one, the Exit button will be lower and lower. Fantastic consistency.

Comment 25

[imradyurrad]

(In reply to comment #24)
> Great idea. And then by adding extension items one by one, the Exit button will
> be lower and lower. Fantastic consistency.

Oh please, Henkel. Bugzilla could do without your sarcasm. AFAIK, the extension portion of the FF menu's gonna have a scrollable element if needed.

As for consistency, I can't name a single OS that doesn't have the exit button at the very buttom.

Comment 26

[Peter Henkel [:Terepin]]

They puted that section to the end of menu for a reason. And that reason is it will be changing it's size. You can't put something different in the midle of something else, consistency would be broken. You can't put it above, because the entire menu would be shifting up and down, consistency would be broken again. And you certainly can't add scroller to the menu, because aside the fact it will decrease accesabilty, it woul be completely inconsistent (and from the subjective point of view, ugly). I can't name any OS that have scrollable area in side nonscrollable area.

Comment 27

[Paul [pwd]]

Maybe there's an alternative. I agree that exit should be at the bottom of the menu. Perhaps the idea of making said area scrollable is what should be questioned and as such should a large number of menu items be created, it should fall back to a secondary menu. Oh perhaps, only showing the most frequently used one, as per the Office menu system.

Comment 28

[u88484]

Can we please force extensions to put their menu entries under the Add-ons submenu?  The main firefox button is going to be inundated with pretty much every single extension that believes it is important enough to require a menu entry.

Comment 29

[Tiago Sá]

I don't think we can force extensions to do anything. Extensions do whatever they want. If we try to force them to put their entries on a sub-menu, they'll just alter that "force" and get on with it. All we can do, as far as I know, is recommend extension makers to put their entries in the appropriate place. But in the end, they WILL be able, ALWAYS, to put stuff wherever.

I also think we should recommend extensions makers NOT to create irrelevant menu entries, like so many do. Like OptimizeGoogle does, completely unneeded, TabRenamizer, AdBlock Plus, Google Redesigned, so many extensions that don't NEED a menu entry, but they do, and it's bad. They should stick to their status bar icon by default, and if the user doesn't want it, he can customize which one appears and so on, like sometimes happens, but extension authors should be recommended to limit the amount of interface clutter they create.

Comment 30

[[not reading bugmail]]

(In reply to comment #29)
I think that is also why there is talk and a bug about the addon bar instead of status bar.  

Correct me if I'm wrong, but I'm trying to recall the technical side of the menu code: extensions in general cannot add entries onto a menu that has no menu id I think if I remember a comment or a bug fix that was made once already to add the ability for menu entries to show up, maybe it was for the customize menu.  If that's the case, then having an extensions sub menu for this makes more sense and don't allow it for the top level menu.

Comment 31

[u88484]

I started a newsgroup discussion about the extensions adding menu entries to the Firefox menu.  Please use this instead of this bug.  Sorry Alex for starting up the discussion here.

http://groups.google.ca/group/mozilla.dev.planning/browse_thread/thread/75c394069413974e#

(In reply to comment #30)
> (In reply to comment #29)
> Correct me if I'm wrong, but I'm trying to recall the technical side of the
> menu code: extensions in general cannot add entries onto a menu that has no
> menu id I think if I remember a comment or a bug fix that was made once already
> to add the ability for menu entries to show up, maybe it was for the customize
> menu.  If that's the case, then having an extensions sub menu for this makes
> more sense and don't allow it for the top level menu.
Extensions can currently add menu-entries is they want to.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#444

Comment 32

[imradyurrad]

(In reply to comment #26)

Read what's written to the left of the extension menu in the mockup.

Comment 33

[lukaswrites.bugzilla]

the exit button should be at the very bottom of the menu , I think...

Comment 34

[Tan Wei Lin Jason [:Ryuji]]

Alex, is there any intention to land the feature of allowing private browsing in a new tab or a new window in Firefox 4?  I previously saw some Firefox Button Mock-up of New Private Tab and New Private Window. I wonder if it is intended for Mozilla developers to create that feature for FF 4.

Comment 35

[Tiago Sá]

(In reply to comment #34)
> Alex, is there any intention to land the feature of allowing private browsing
> in a new tab or a new window in Firefox 4?  I previously saw some Firefox
> Button Mock-up of New Private Tab and New Private Window. I wonder if it is
> intended for Mozilla developers to create that feature for FF 4.

I have never seen such mockups myself, but I did make one mockup with that. I'm not a developer, by the way. I did that because I didn't know, at the time, how private browsing works (I never use it because it's completely pointless), so I did that. But since when you start private browser, your whole session is replaced by a new private session, it wouldn't work.

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #34)
> Alex, is there any intention to land the feature of allowing private browsing
> in a new tab or a new window in Firefox 4?  I previously saw some Firefox
> Button Mock-up of New Private Tab and New Private Window. I wonder if it is
> intended for Mozilla developers to create that feature for FF 4.

I'm pretty sure something like that would depend on full process separation for tabs, which I'm almost positive (as an outsider to Mozilla) won't be in the 4.0 timeframe.

Comment 37

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug is going to need an owner soon if we are hoping to make beta 4.

Comment 38

[u88484]

Alex, I'm going to work on some of this and attach a patch for someone else to complete.  Basically all I'm going to do is add all the menu entries, separators, split-menus, keyboard shortcuts, iconic images?.  I've already tried to add the start private browsing but I couldn't get the toggling code to update the menu entry once PB was started.

Someone else feel free to jump on this though if I don't assign myself to this before tomorrow.

Comment 39

[Peter Henkel [:Terepin]]

Since we are getting proper Firefox Menu for beta4, can we get proper Firefox Button (bug 574681) for beta4 as well?

Comment 40

[lukaswrites.bugzilla]

(In reply to comment #37)
> This bug is going to need an owner soon if we are hoping to make beta 4.

if this thing can't be done in beta 4 , will it be within firefox 4 or , will it be suspended till the next firefox final release? (e.g Fx 4.5 or 4.6)

Comment 41

[imradyurrad]

(In reply to comment #40)
> (In reply to comment #37)
> > This bug is going to need an owner soon if we are hoping to make beta 4.
> 
> if this thing can't be done in beta 4 , will it be within firefox 4 or , will
> it be suspended till the next firefox final release? (e.g Fx 4.5 or 4.6)

I'm not sure if that's sarcasm or impatience. This'll definitely be in 4.0.
Even if for some reason it wasn't, 4.5 and 4.6 are very far away. A  year at the least.

Not making beta4 isn't that big a deal. We still have about 3 betas left. That's 6 weeks.

Comment 42

[Tan Wei Lin Jason [:Ryuji]]

I don't think a 2 week beta cycle is quite possible. I mean there is a lot of feature and bugs that need to be landed in order to release the next beta. I don't think they can do it that quickly.

Comment 43

[Tiago Sá]

This is kind of a blocker, so no Firefox will no Firefox Menu that works properly...

I say.

Comment 44

[u88484]

I'll be upload an almost complete patch here in a little while.  Hold your horses.

Comment 45

[u88484]

Attachment: Patch v1

Patch v1 (not complete)

What I did not do and still needs to be done:
* Make the menu purdy
* Add iconic images to whatever still doesn't have them
* Add keyboard shortcuts (to submenus) for whatever doesn't have them
* Clicking 'Open' in the new tab menu opens the open file dialog...not sure if this is correct.
* The command for private browsing doesn't fully work.  Private browsing mode is started but the menu item isn't changed to 'Stop Private Browsing
* Everything to do with 'Edit'
* The 'Send Link' split-menu and its contents
* I can't figure out why the "Bookmarks Sidebar" shows as just Bookmarks.  I added the entity but for some reason only the bookmarks portion of it shows even though it works when used elsewhere in a menu.
* Test the clear recent history because I don't remember if that automatically clears the history and I don't want to lose mine
* Make populating the 'Recently Closed Tabs' (and windows) popup menus work 
* Did not remove 'Customize...'.  Not sure if that is really wanted and I didn't completely understand what is going on with the 'Options...' menu
* 'Interface...' does not have a command as I don't understand what that should open.
* Add a command for 'Getting Started' because I wasn't sure what that should open
* Did not add 'Sync' as I believe the sync crew would have that in their patch that implements the UI since that hasn't landed yet.
* Make populating the 'Character Encoding' popup menu work

Someone else can take over the bug because I'm done with all that I can do.

Comment 46

[u88484]

Attachment: Screenshot of patch 1 on Windows 7

Comment 47

[u88484]

Alex, if you can answer some of the points above I can create a new patch then someone that actually knows how to code can takeover the patch to fix a few small things that shouldn't take more than 10 minutes.

Comment 48

[lukaswrites.bugzilla]

(In reply to comment #45)
> Created attachment 463830 [details] [diff] [review]
> Patch v1
> 
> Patch v1 (not complete)
> 
> What I did not do and still needs to be done:
> * Make the menu purdy
> * Add iconic images to whatever still doesn't have them
> * Add keyboard shortcuts (to submenus) for whatever doesn't have them
> * Clicking 'Open' in the new tab menu opens the open file dialog...not sure if
> this is correct.
> * The command for private browsing doesn't fully work.  Private browsing mode
> is started but the menu item isn't changed to 'Stop Private Browsing
> * Everything to do with 'Edit'
> * The 'Send Link' split-menu and its contents
> * I can't figure out why the "Bookmarks Sidebar" shows as just Bookmarks.  I
> added the entity but for some reason only the bookmarks portion of it shows
> even though it works when used elsewhere in a menu.
> * Test the clear recent history because I don't remember if that automatically
> clears the history and I don't want to lose mine
> * Make populating the 'Recently Closed Tabs' (and windows) popup menus work 
> * Did not remove 'Customize...'.  Not sure if that is really wanted and I
> didn't completely understand what is going on with the 'Options...' menu
> * 'Interface...' does not have a command as I don't understand what that should
> open.
> * Add a command for 'Getting Started' because I wasn't sure what that should
> open
> * Did not add 'Sync' as I believe the sync crew would have that in their patch
> that implements the UI since that hasn't landed yet.
> * Make populating the 'Character Encoding' popup menu work
> 
> Someone else can take over the bug because I'm done with all that I can do.

that's good, but, I think it's little bit different from the mockup right?

Comment 49

[Peter Henkel [:Terepin]]

(In reply to comment #45)
> Patch v1 (not complete)

That pretty much says everything.

Comment 50

[Dão Gottwald [:dao]]

(In reply to comment #49)
> (In reply to comment #45)
> > Patch v1 (not complete)
> 
> That pretty much says everything.

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 51

[Peter Henkel [:Terepin]]

Which aspect of my reply is against the rules? I have the feeling that even if I wrote simple dot it would be still against rules...

Comment 52

[lukaswrites.bugzilla]

(In reply to comment #51)
> Which aspect of my reply is against the rules? I have the feeling that even if
> I wrote simple dot it would be still against rules...

agree with you, there are so many sensitive persons in bugzilla these days....

Comment 53

[Tiago Sá]

It was a pointless comment. Unless you have something constructive to say, don't post. I hope this is constructive enough.

Comment 54

[Peter Henkel [:Terepin]]

(In reply to comment #45)
> * I can't figure out why the "Bookmarks Sidebar" shows as just Bookmarks.  I
> added the entity but for some reason only the bookmarks portion of it shows
> even though it works when used elsewhere in a menu.

The same thing happens to StrataBuddy. The dev eventually fixed it, but don't know how. I might ask him if you want.

Comment 55

[Mike Beltzner [:beltzner, not reading bugmail]]

Congrats, Kurt - you own a blocker! :)

Comment 56

[u88484]

(In reply to comment #55)
> Congrats, Kurt - you own a blocker! :)

Sorry Mike but I really can't code.  I was just trying to help someone else out with the tedious part.

Comment 57

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex, if you can answer some of the points above I can create a new patch

Sorry about the lag, here's answers to all of the questions


>* Clicking 'Open' in the new tab menu opens the open file dialog...not sure if
>this is correct.

correct, same as File > Open File...


==Items that are ok to leave for now==

>* Make the menu purdy

Totally fine to hold this work for a later beta (we also might be doing some repositioning of a few of the top level items then as well)

>* Add iconic images to whatever still doesn't have them

Also fine to hold for a later beta, but I can attach the icons if someone wants to land them with this patch.

>* Did not add 'Sync' as I believe the sync crew would have that in their patch
>that implements the UI since that hasn't landed yet.

>* 'Interface...' does not have a command as I don't understand what that should
>open.

>* The 'Send Link' split-menu and its contents

It's ok for send link to just default to email for now

==Items we should fix for this patch==
>* Everything to do with 'Edit'

>* Add keyboard shortcuts (to submenus) for whatever doesn't have them

>* I can't figure out why the "Bookmarks Sidebar" shows as just Bookmarks.  I
>added the entity but for some reason only the bookmarks portion of it shows
>even though it works when used elsewhere in a menu.

>* The command for private browsing doesn't fully work.  Private browsing mode
>is started but the menu item isn't changed to 'Stop Private Browsing

>* Test the clear recent history because I don't remember if that automatically
>clears the history and I don't want to lose mine

>* Make populating the 'Recently Closed Tabs' (and windows) popup menus work 

>* Make populating the 'Character Encoding' popup menu work


==Items we should fix for this patch (with comments)==

>* Did not remove 'Customize...'.  Not sure if that is really wanted and I
>didn't completely understand what is going on with the 'Options...' menu

I'll attach a new mockup to detail that for now (current design assumes that we are able to move interface customization to an in content page instead of a group of menu check items)

>* Add a command for 'Getting Started' because I wasn't sure what that should
>open

Same URL as the pre-populated bookmark would be fine for now

Comment 59

[Shawn Wilsher :sdwilsh]

So, we have no assignee on this bug, and it doesn't have an owner.  Why do we need this for beta4?  Comment 2 says it would be nice to block beta4 but no reasoning was given.

Comment 60

[Mike Beltzner [:beltzner, not reading bugmail]]

I'd marked it b4 when it had an owner; now that it doesn't, agree that it can't make that milestone.

Comment 61

[[:Cww]]

Is there a bug for getting the troubleshooting information links into the about: screen?

Comment 62

[Alex Faaborg [:faaborg] (Firefox UX)]

>Comment 2 says it would be nice to block beta4 but no reasoning was given.

Sorry, the reason was that we are currently missing access to some very major menu commands.  Curiously people don't seem to be freaking out as much as I would have expected though, so a later beta is probably fine.

Comment 63

[Alex Faaborg [:faaborg] (Firefox UX)]

Thanks Frank!

A couple of additional notes:

==Keyboard access==

alt-f should open the new menu (and up-clicking the alt key without a modifier should continue to temporarily display the traditional menu).  This is somewhat morphing the behavior of ribbon apps like Wordpad, Paint and Office, with the behavior of menu apps like Explorer and IE, but seems the best way to maintain consistency with both.

After being invoked with alt-f, give the bookmarks command in the right pane the focus, so the user can hit left to access the left pane, and right to access the submenu of bookmarks (this will make sense to everyone else after I post i6).

==Color==

Menu commands should be text color on window color (which normally works out to black on white) with Aero and Luna.  The recessed light blue area can be hard coded to #F1F5FB if we detect the use has a "default theme" https://developer.mozilla.org/en/CSS/%3A-moz-system-metric%28windows-default-theme%29

However, this is one of the colors that we would in the future modify based on the ability to detect the user's exact windows theme (bug 543910)

Comment 64

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: i6 mockup of the Firefox Menu

This iteration includes several changes from i5:

-Two pane view that moves all of the different locations that the user can go to over to the right pane.  At the moment a "location" usually means opening a pop-up window, but eventually these will all be in-content UI.  An detailed discussion of this change can be found in mozilla.dev.usability.

-Sync item removed for now as we discuss possible primary UI options

-Options > Interface temporarily expanded out to the current set of toolbar commands

-Work Offline temporarily added under developer tools

Comment 65

[Mike Beltzner [:beltzner, not reading bugmail]]

I like the new mockup a lot; not sure if it's doable in XUL, but fryn's a bit of a wizard!

One note: I think we should keep "Troubleshooting" in the top-level Help menu, not the About Firefox box. It's proven to be an incredibly valuable tool for our support team, and I think we want to make it a little easier to get to.

Comment 66

[patrickjdempsey]

Any plans for a visual indicator like a vertical separator to make it more clear that some of these new items are both menus and menuitems?  That would give us three basic kinds of menu items with clear distinction:

icon menuitem 
icon menu >
icon menuitem separator >

Comment 67

[Alex Faaborg [:faaborg] (Firefox UX)]

>One note: I think we should keep "Troubleshooting" in the top-level Help menu,
>not the About Firefox box. It's proven to be an incredibly valuable tool for
>our support team, and I think we want to make it a little easier to get to.

What they need is a mouse driven path, I figured placing a link at the bottom of the Home Tab would be easier place to direct people to (over the phone, etc.) than in a sub-menu.  This sort of puts it in primary UI (assuming the home tab has focus).

Comment 68

[Joshua M [:soapy]]

I have created a menu which is almost identical to the mockup Alex has made.

http://dl.dropbox.com/u/3769988/Firefox/mockupmenu.png

I know I'm missing redo/undo,but other than that it's pretty close. Any opinions would be great.

Comment 69

[Paul [pwd]]

I like the new mockup, but Help should definitely be on the left above the Exit item.

Comment 70

[imradyurrad]

(In reply to comment #69)
> I like the new mockup, but Help should definitely be on the left above the Exit
> item.

Why not just make it a secondary or tertiary menu item? I don't think it's important enough to warrant cluttering the menu.

Comment 71

[Jonathan Haas]

(In reply to comment #70)
> Why not just make [help] a secondary or tertiary menu item? I don't think it's
> important enough to warrant cluttering the menu.

1) There is no option to attach it to. Would you put it under Options? Under Developer Tools? Doesnt make sense at all.

2) Users who actually need help should be able to find this option easily.

On a side note, I'm still concerned about the edit icons. They're really unintuitive and ugly and will cause more issues und usability problems than a simple submenu would. See comment 20 for details. For example even I, as a experienced Windows user, need a while to decide which icon means paste and which means copy. New users who are afraid of shortcuts and therefore are the only ones who require those menu items will be hopelessy lost. Do some useability experiments if you don't believe me. Please.

Comment 72

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #67)
> What they need is a mouse driven path, I figured placing a link at the bottom
> of the Home Tab would be easier place to direct people to (over the phone,

What happens if the user changes their home tab to be a different page?

Comment 73

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #72)
> What happens if the user changes their home tab to be a different page?

Isn't there a planned difference between "home tab" and "homepage"? I know right now, it'd just be a tab that takes you to your homepage, but I thought the "home tab" had much bigger plans.

Comment 74

[[:Cww]]

(In reply to comment #72)
(Man, this bug moves fast!)

Troublehshooting info should be a help menu item at least.  Minimally, it's consistent with the behavior with the old menubar view where it's under Help.  That way, we don't have to know which menu they're using, we can say it's in the menu, help, troubleshooting info.  We'd rather not chase it down with "if it's not there, try the home tab, if it's not there, try the about screen."

Comment 75

[Tiago Sá]

(In reply to comment #73)
> (In reply to comment #72)
> > What happens if the user changes their home tab to be a different page?
> 
> Isn't there a planned difference between "home tab" and "homepage"? I know
> right now, it'd just be a tab that takes you to your homepage, but I thought
> the "home tab" had much bigger plans.

No, the home tab will not be customizable (i.e. it won't load your home page, particularly because it's just one and you can have multiple home pages). The plan is to make home pages (the current ones anyway) load in app tabs, but that is not optimal, in my opinion, as I've argued here:

http://groups.google.com/group/mozilla.dev.usability/browse_thread/thread/f1833c54aa6d64f9

Still, that's the plan, as far as I know

Comment 76

[u88484]

Reassigning to Joshua M.  He has an almost complete patch based on iteration 6 that I'm helping him get ready for review (almost there!).  Hope you don't mind Frank!  I talked to khuey before doing this and he said he'd take the flak if you already started working on this.

Alex, any update on those images for Edit?

Comment 77

[Alex Faaborg [:faaborg] (Firefox UX)]

>Troublehshooting info should be a help menu item at least.  Minimally, it's
>consistent with the behavior with the old menubar view where it's under Help. 

The Home Tab placement would give us cross platform consistency, so that we could always consistently tell users "click home, scroll to the bottom and click the link trouble shooting information" every time, instead of saying "which platform are you on, ok, well in that case look to the right side of the firefox menu, under options, yeah so then expand that help sub menu, then click troubleshooting information"

For consistency we would want to actually remove this command from the help menu on all other platforms (and the traditional menu accessible with the alt key on modern versions of windows).

Comment 78

[Alex Faaborg [:faaborg] (Firefox UX)]

>What happens if the user changes their home tab to be a different page?

This problem isn't unique to a troubleshooting information hyperlink in the
footer, there is a lot of Firefox functionality planned for the Home Tab that
we can't really make optional (and force the duplication of in other parts of
the UI).  For instance, a number of asynchronous communications like update
available (after we give them amble time to restart on their own) are being
moved to Home Tab snippets.  The introduction of one of these push snippets
will cause the home tab to glow similar to how a Gmail appt ab will glow when
you get an incoming chat.

I think the cleanest way to deal with this is to allow users to specify their
start page (out of the list of existing app tabs, or adding a new one that will
generate an app tab).  This app tab is then shown on launch, so the startup
experience is nearly exactly the same.  The only difference is that their
startup page is going to be using the favicon for that particular site.  Under
this model going forward the house icon is now a controlled metaphor set to
mean the specific functionality of Firefox Home.  This strategy also ties
strongly into the naming of the Firefox Home iPhone app (which contains the
same set of data and functionality).

Comment 79

[Joshua M [:soapy]]

Attachment: Patch v1 based on i6

Css needs to be cleaned up and redone with proper images, encoding menu needs to be populated.

Comment 80

[Joshua M [:soapy]]

Attachment: Screenshot of Patch v1 based on i6

Comment 81

[u88484]

Comment on attachment 465760 [details] [diff] [review]
Patch v1 based on i6

Requesting a quick review on what the user has uploaded so far.  Once the final icons are provided and someone helps with populating the character encoding submenu then a complete final patch will be submitted.

I also tried figure out the character encoding menu to no avail.

Comment 82

[Frank Yan (:fryn)]

(In reply to comment #79)
> Created attachment 465760 [details] [diff] [review]
> Patch v1 based on i6
> 
> Css needs to be cleaned up and redone with proper images, encoding menu needs
> to be populated.

Excellent work! Thanks for taking the initiative to do this :)
There are a couple subtleties about the menu that Faaborg noted to me in a conversation yesterday.
For example, all the menuitems need accesskeys.
I'll build with the patch and show it to Faaborg for feedback :)

Comment 83

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #78)
> >What happens if the user changes their home tab to be a different page?
> 
> This problem isn't unique to a troubleshooting information hyperlink in the
> footer, there is a lot of Firefox functionality planned for the Home Tab that
> we can't really make optional (and force the duplication of in other parts of

We're off topic for this bug talking about the Home Tab, other than to say that we can not and should not depend on the Troubleshooting link being in the locally hosted Firefox Start UI. (None of that functionality is planned to make it in Firefox 4, or at least none of it has been cleared with me, or resourced.)

From a user experience perspective, I do not think that it's a reasonable expectation that one would have to go to the "home" tab to get to troubleshooting information. Especially when there's a menu called "Help" which users have been trained (by thousands of applications) to investigate for their help and support needs.

The Troubleshooting menuItem should be restored in the "Help" section.

Comment 84

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 465761 [details]
Screenshot of Patch v1 based on i6

awesome work, we can deal with an subtle issues in follow up bugs if we need to, but overall ui-r+

Comment 85

[Alex Faaborg [:faaborg] (Firefox UX)]

>For example, all the menuitems need accesskeys.

the two pane mode makes keyboard arrow navigation somewhat difficult, since hitting right will expand the sub-menu of an item on the left pane (and not take you to the right pane).  Our somewhat hacky solution is to start with the focus on bookmarks, that way the user can choose which pane they want to before they expand any sub-menus.

Comment 86

[Alex Faaborg [:faaborg] (Firefox UX)]

Stephen: could we get 16x16 icons for undo and redo in both the normal and disabled state?

Comment 87

[Alex Faaborg [:faaborg] (Firefox UX)]

Filed follow up bug 587178 for the undo and redo icons

Comment 88

[Alex Faaborg [:faaborg] (Firefox UX)]

Tried out the patch applied, everything is definitely good enough to land now for user feedback.  Here are the notes I had if you have time to make any additional changes:

-Bookmarks should be a combo menu, with the main action opening the library window and navigating to All Bookmarks

-History should be a combo menu, with the main action opening the library window and navigating to History

-Help should be a combo menu, with the main action opening Help (same as first item in the menu)

-Edit icons still look inactive when they are actually active

-Edit switches from inactive text to black (but still italics) on hover

-Need to register alt-f to open

-Need to set the focus after alt-f to bookmarks, and get the combo menus to be accessible with arrow keys (they seem to currently be skipped)

-Need to get the windows system menu accessible with a right click on the Firefox button

Thanks for getting this done so quickly! Dolske may be able to give you an expedited review so that we can try to get this into Beta 4.

Comment 89

[Justin Dolske [:Dolske]]

Comment on attachment 465760 [details] [diff] [review]
Patch v1 based on i6

Hey, this looks really fantastic overall! Just a few things that need to be tweaked...

[Looks like this is your first patch (congrats!), if there's anything we can do to help explain the process feel free to ask/email -- or irc://irc.mozilla.org in #fx-team or #developers if you'd like to chat with us live.]


>+++ b/browser/base/content/browser.js
...
>-  var firstMenuItem = popup.firstChild;
>+  var firstMenuItem = popup.firstChild.nextSibling.nextSibling;

popup.querySelectorAll("menuitem") should be more robust to future changes in the menu DOM structure.


>+<hbox id="appmenu-button-container">
>+   <button id="appmenu-button" type="menu" label="&brandShortName;" style="-moz-user-focus: ignore;">
>+     <menupopup id="appmenu-popup">
>+       <grid>
>+         <rows>
>+           <row>

A grid feels a bit like overkill here, <hbox><vbox/><vbox/></hbox> seems like ought to be enough, but maybe I've missed a subtle issue?

>+               <menuitem id="privateBrowsingItem" class="menuitem-iconic" image="chrome://browser/skin/Privacy-16.png" label="&privateBrowsingCmd.start.label;" accesskey="&privateBrowsingCmd.start.accesskey;" startlabel="&privateBrowsingCmd.start.label;" stoplabel="&privateBrowsingCmd.stop.label;" command="Tools:PrivateBrowsing"/>

I hate to nitpick on line lengths, but ginormous lines like this are really hard to read (both in the source, and especially Bugzilla). Please reformat to match the existing style in this file (roughly, one attribute per line, vertically aligned).

The image= should be set in browser/themes/*/browser/browser.css via an ID selector and list-style-image, take a look at how other menuitems do this. [This allows 3rd party themes to change the style, if they want to.]

>+             <vbox width="150" id="appmenuRightPane">

The width= probably should be in browser.css too, instead of being specified inline.

>+               <menu id="appmenuBookmarkMenu" label="&bookmarksMenu.label;" class="menu-iconic">
>+                 <menupopup id="appmenuBookmarkMenu_popup" placespopup="true" context="placesContext" openInTabs="children" oncommand="BookmarksEventHandler.onCommand(event);" onclick="BookmarksEventHandler.onClick(event);" onpopupshowing="BookmarksMenuButton.onPopupShowing(event);if (!this.parentNode._placesView)new PlacesMenu(event, 'place:folder=BOOKMARKS_MENU');this.appendChild(document.getElementById('appmenuBookmarkMenu_unsorted_seperator'));this.appendChild(document.getElementById('appmenuBookmarkMenu_unsorted-menuitem'));" tooltip="bhTooltip" popupsinherittooltip="true">

Hmm, that's kind of a lot of JS to dump inline. Not sure if Places offers a helper to make this smaller, or maybe we should just modify Places / add a helper to browser.js. Lemme ask around.

>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
...
>+<!ENTITY appmenuSaveAs.label "Save As...">

The "..." should be a UTF-8 ellipsis, as in "Toolbar Layout…" above it.

>+++ b/browser/themes/winstripe/browser/browser.css
...
>+#appmenu-button #appmenu_print,
>+#appmenu-button toolbarbutton {
>+  list-style-image: url(data:image/png;base64,iVBORw0KGgoAA

The data: URIs really need to be standalone files in the source tree. Basic steps are:

1) Put the PNG in the desired location in your mozilla-central tree
2) "hg add path/to/it.png" so Hg knows you're adding it
3) Add entries to browser/themes/*/browser/jar.mn
4) Add CSS list-style-image that refers to it.

If this is too much magic Hg mumbo-jumbo, I can help update the patch to do all this once the other stuff is addressed.

(I assume these are the icons faaborg was asking shorlander about in previous comments?)

>+#appmenu-button toolbarbutton:not([disabled]):hover {
>+  border: 1px solid #b8d6fb !important;
>+  -moz-box-shadow: inset 0 0 1px white !important;
>+  background: -moz-linear-gradient(#fafbfd, #ebf3fd) !important;
>+}

Might need to tweak these to use the special CS colornames that inherit from the current OS theme, but I didn't really check to see what the colors specified here are for or if that's actually for. Faaborg or shorlander might have a handy reference table for these color names?

Comment 90

[Alex Faaborg [:faaborg] (Firefox UX)]

>>+#appmenu-button toolbarbutton:not([disabled]):hover {
>>+  border: 1px solid #b8d6fb !important;
>>+  -moz-box-shadow: inset 0 0 1px white !important;
>>+  background: -moz-linear-gradient(#fafbfd, #ebf3fd) !important;
>>+}
>
>Might need to tweak these to use the special CS colornames that inherit from
>the current OS theme,

we should only be using colors light #F1F5Fb (the light blue background of the right pane) if we detect the user has a default theme (Aero, Luna):

https://developer.mozilla.org/en/CSS/%3A-moz-system-metric%28windows-default-theme%29

All of the other colors in the menu can be based on extracted system colors

Comment 91

[Alex Faaborg [:faaborg] (Firefox UX)]

sorry "colors meant like #F1F5Fb"

Comment 92

[Joshua M [:soapy]]

Thanks for the feedback guys. This is my first patch and I'll do what I can to implement the suggestions. It was a very rough cut to get this out and get some general feedback.

Comment 93

[Joshua M [:soapy]]

Attachment: Patch v2 based on i6

Added split-menus to history/bookmarks/help
Moved all 'image' attributes from xul into browser.css
Moved 'width' attribute into browser.css
Added image to theme
Fixed edit label hover
Fixed "Save As…" label in browser.dtd
Cleaned up xul

Comment 94

[Dão Gottwald [:dao]]

Comment on attachment 465931 [details] [diff] [review]
Patch v2 based on i6

a few drive-by comments...

>+                 <menuitem id="appmenu_editLabel" class="split-menuitem-item" flex="1" label="&editMenu.label;" disabled="true"/>
>+                 <toolbarbutton id="cut-button" class="toolbarbutton-1 chromeclass-toolbar-additional" command="cmd_cut" onclick="hidePopup();" tooltiptext="&cutButton.tooltip;"/>
>+                 <toolbarbutton id="copy-button" class="toolbarbutton-1 chromeclass-toolbar-additional" command="cmd_copy" onclick="hidePopup();" tooltiptext="&copyButton.tooltip;"/>
>+                 <toolbarbutton id="paste-button" class="toolbarbutton-1 chromeclass-toolbar-additional" command="cmd_paste" onclick="hidePopup();" tooltiptext="&pasteButton.tooltip;"/>

Neither toolbarbutton-1 nor chromeclass-toolbar-additional are appropriate for these. You should add a new class like appmenu-edit-button, though, and use that instead of "#appmenu-button toolbarbutton" in the CSS.

Also prepend "cut-button", "copy-button" and "paste-button" with "appmenu-". Or maybe name them "appmenu_cut" etc.

>+#appmenu-button > menupopup {

#appmenu-popup

>+  -moz-appearance: none !important;
>+  background: white !important;
>+  border: 1px solid rgba(0,0,0,.5) !important;
>+}

Do this only if the default OS theme is detected, as Alex noted.

>+#appmenu-button #appmenu_print,

Remove "#appmenu-button" here and in similar id selectors.

Many of the !important flags look like they shouldn't be needed.

Comment 95

[Tiago Sá]

This is for Alex.

By the way, where do Sync and Tab Candy go? Also, where does the recently closed tabs list go?

Maybe we could create, at least for Tab Candy and recently closed (and a few others), a "tabs" entry with a sub menu that defauts to tab candy. The submenu would have recently closed, make app tab and maybe bookmark this tab or something like that, if the redundancy doesn't outweight the fact that many people, I think, use the menu entry rather than the star.

Comment 96

[Peter Henkel [:Terepin]]

TabSets icon is in tab bar and Weave icon is in status bar (add-ons bar). No need to clutter the menu.

Comment 97

[Joshua M [:soapy]]

Attachment: Patch v3 based on i6

-Edit,copy,paste have their own ids and class. Updated browser.js so they get disabled when necessary.

-Cleaned up CSS. Menu is only skinned on the default Windows theme.

-Clicking on a disabled copy,cut, or paste button in the menu no longer closes it.

Comment 98

[Justin Dolske [:Dolske]]

Comment on attachment 465984 [details] [diff] [review]
Patch v3 based on i6

Ok, I did a detailed review so I have more comments. But let me say this is really great work (especially for a first patch!), and is coming along fantastically!

>+++ b/browser/base/content/browser.js
...
>                    placesContextMenuPopupState == "open" ||
>                    document.getElementById("cut-button") ||
>                    document.getElementById("copy-button") ||
>+                   document.getElementById("appmenu-cut") ||
>+                   document.getElementById("appmenu-copy") ||
>+                   document.getElementById("appmenu-paste") ||

You don't want this. These menu elements are always in the browser's DOM, so these always evaluate to true.

Rather, this should check to see if the Firefox menu is currently showing or not (ie, this code is trying to avoid doing expensive work if the UI it would change isn't even showing).

So, instead:

  let appMenuPopupState = document.getElementById("appmenu-popup").state;
  ...
  appMenuPopupState == "showing" ||
  appMenuPopupState == "open" ||


>-  var firstMenuItem = popup.firstChild;
>+  var firstMenuItem = popup.firstChild.nextSibling.nextSibling;

Looking at this closer now, I see my earlier querySelectorAll comment was misplaced... This code wants to know where it should insert the new menuitem entries it's generating, normally that's before the first thing in the menupopup (which is actually normally a menuseperator, not a menuitem), but for the app menu there's an existing "Options..." entry that we want to stay at the top. For the other menupopups this is invoked upon, this change makes it insert items at the wrong point.

I think the safeset way to handle this is to allow the caller of onViewToolbarsPopupShowing() to optionally specify an insertion point. So, down in the appmenu you'd have:

  onpopupshowing="onViewToolbarsPopupShowing(event, this.firstChild.nextSibling.nextSibling);"

and then something like:

function onViewToolbarsPopupShowing(aEvent, aInsertPoint) {
...
  var firstMenuItem = popup.firstChild;
  if (aInsertPoint)
    firstMenuItem = aInsertPoint;


>@@ -4702,6 +4705,7 @@
>       menuItem.setAttribute("label", toolbarName);
>       menuItem.setAttribute("checked", toolbar.getAttribute(hidingAttribute) != "true");
>       if (popup.id != "appmenu_customizeMenu")
>+      var firstMenuItem = popup.firstChild;
>         menuItem.setAttribute("accesskey", toolbar.getAttribute("accesskey"));
>       popup.insertBefore(menuItem, firstMenuItem);

This change looks like an accidental cut'n'paste? It breaks the |if| because it's changing the (unbraced) first-and-only statement of the |if|. Remove.


>+++ b/browser/base/content/browser.xul
>-#filter substitution
>+w#filter substitution

Ooops, this change looks like a accidental change. Remove.

>+   <button id="appmenu-button" type="menu" label="&brandShortName;" style="-moz-user-focus: ignore;">

Nit: attributes on separate lines (looks like you did update the <toolbarbutton>s, those look fine now).

>+                 <menuitem id="appmenu_newTab" class="split-menuitem-item" flex="1" label="&tabCmd.label;" command="cmd_newNavigatorTab"/>

Nit: attributes on separate lines for this and the other <menuitems>.

>+               <menuitem id="appmenu_downloads" label="&downloads.label;" command="Tools:Downloads"/>
>+               <spacer height="20px"/>
>+               <menuitem id="appmenu_addons" label="&addons.label;" command="Tools:Addons" class="menuitem-iconic"/>

I think the intention of the spacer is to essentially be a blank <menuitem>, so it's height should be set in "em" units, perhaps fractional to get close to 20px for the default font. That way if the user is, say, using enormous system fonts, the spacer will not be stuck at a static 20 pixels in height. 

>+                   <menuitem id="appmenu_help_getting_started" label="&appmenuGettingStarted.label;" oncommand="gBrowser.selectedTab = gBrowser.addTab('http://www.mozilla.com/firefox/central/');" onclick="checkForMiddleClick(this, event);"/>

This URL shouldn't be hardcoded, but I think this is OK for now... We can fix this in a followup bug, and maybe use oncommand="openHelpLink('firefox-getstarted')".


>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
...
> <!ENTITY appMenuSidebars.label "Sidebars">
>+<!ENTITY appmenuOpen.label "Open">

The existing entries are all "appMenuFoo", you're adding "appmenuFoo", so the result is a confusing mix. Please change the ones you're adding to match the existing uppercase-M format.

+<!ENTITY appmenuOpen.label "Open">

Faaborg: I think this should be "Open File..."

+<!ENTITY appMenuPrintCmd.label "Print">

I believe we need two strings here: "Print..." and "Print".

Faaborg: It's kind of awkward to have these both visible close together, should one be "Print Now"?

+<!ENTITY appmenuCharacterEncoding.label "Character Encoding">

Remove, this can be added when someone gets around to adding the submenu.


>diff --git a/browser/themes/winstripe/browser/appmenu-icons.png b/browser/themes/winstripe/browser/appmenu-icons.png

Did you extract the icons from the mockups, or are they from somewhere else?

Just making sure we're not landing artwork someone else owns. :) If Horlander's going to provide new icons anyway, we can land with those and this won't matter.


>+++ b/browser/themes/winstripe/browser/browser.css

CSS basically looks fine, though Faaborg should probably sit down with a few flavors of Windows to make sure things look right on the various platforms. I can kick off a tryserver build when the patch is about done, or we can maybe just handle that in a separate followup bug.


>+++ b/browser/themes/winstripe/browser/jar.mn
...
>+        skin/classic/aero/browser/appmenu-icons.png

Also need to add this (but without the "aero") in the similar place up near the top of the file, so non-aero flavors of Windows will see the icon.

Comment 99

[Alex Faaborg [:faaborg] (Firefox UX)]

>Faaborg: I think this should be "Open File..."

yep, my mistake in the mockup

>Faaborg: It's kind of awkward to have these both visible close together, should
>one be "Print Now"?

Both should be "Print..." another mistake in the mockup.  The first command being redundant is to expose keyboard shortcuts, and to help users who haven't figured out that you can hit the main item to carry out the command.

>If Horlander's
>going to provide new icons anyway, we can land with those and this won't
>matter.

Landing them now is fine, we just need to be sure to run optipng and pngcrush later (not just on these but all of our new icon files), to check for rogue color profiles and reduce file size.

>Faaborg should probably sit down with a few
>flavors of Windows to make sure things look right on the various platforms.

We can take care of that later, Firefox menu is off by default on XP and 2000

Comment 100

[Alex Faaborg [:faaborg] (Firefox UX)]

>By the way, where do Sync and Tab Candy go? Also, where does the recently
>closed tabs list go?

At the moment Sync is in the status bar, but we are considering placing the user's short name (specified during sync set up) to the right of the Firefox button.  With the account manager feature, we are worried about mode errors, where you end up giving someone else access to your account (across all of their machines), if you don't realize that you are using an instance of Firefox that has been set up with a sync account.

TabSets will get a control attached to the tab overflow drop down.

Recently closed tabs is under History in i6

Comment 101

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Tweak to the bookmarks menu

Tweak to the bookmarks menu based on discussion with Limi and comments in bug 578967.

Removed:
-View bookmarks toolbar
-View bookmarks sidebar

Added:
-bookmark this page (discoverability of icon and keyboard shortcut, heavily used item)
-Subscribe to this page

If we can get these changes landed in this bug that would be ideal, otherwise we'll use a follow up.

Comment 102

[Dão Gottwald [:dao]]

(In reply to comment #98)
> I think the intention of the spacer is to essentially be a blank <menuitem>, so
> it's height should be set in "em" units, perhaps fractional to get close to
> 20px for the default font. That way if the user is, say, using enormous system
> fonts, the spacer will not be stuck at a static 20 pixels in height. 

It should be a menuseparator, styled accordingly.

Comment 103

[Dão Gottwald [:dao]]

Comment on attachment 465984 [details] [diff] [review]
Patch v3 based on i6

>+.appmenu-edit-button:not([disabled]):hover {
>+  border: 1px solid #b8d6fb;
>+  -moz-box-shadow: inset 0 0 1px white;
>+  background: -moz-linear-gradient(#fafbfd, #ebf3fd);
>+}

This should also be dependent on -moz-windows-default-theme.

Comment 104

[Joshua M [:soapy]]

Attachment: Patch v4 based on i6

Added 'checkbox' attribute to fullscreen,console, and inspector
Spacers now use 'em' measurement, code to change size is in browser.css
Appmenu-icons added to both themes
Fixed customize menu popup swaping items around
Reworked javascript related to edit buttons
Implemented newest bookmark menu mockup
Updated strings in browser.dtd to be more consistent
Updated strings in browser.dtd based on comments
Edit-buttons hover is only active on default Windows theme
All attributes in xul are now on their own line
Removed "w" at the start of browser.xul, was a mistake

Comment 105

[Dão Gottwald [:dao]]

Comment on attachment 466107 [details] [diff] [review]
Patch v4 based on i6

Can you please adjust the diff context as per <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3f>?

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -3587,6 +3587,7 @@
>   let editMenuPopupState = document.getElementById("menu_EditPopup").state;
>   let contextMenuPopupState = document.getElementById("contentAreaContextMenu").state;
>   let placesContextMenuPopupState = document.getElementById("placesContext").state;
>+  let appMenuPopupState = document.getElementById("appmenu-popup").state;

This will fail on platforms where appmenu-popup doesn't exist.

>   var firstMenuItem = popup.firstChild;
>+    if (aInsertPoint)
>+      firstMenuItem = aInsertPoint;

var firstMenuItem = aInsertPoint || popup.firstChild;

>+<hbox id="appmenu-button-container">
>+   <button id="appmenu-button" 
>+           type="menu" 
>+           label="&brandShortName;" 
>+           style="-moz-user-focus: ignore;">
>+     <menupopup id="appmenu-popup"
>+                onpopupshowing="updateEditUIVisibility();">
>+        <hbox>
>+             <vbox id="appmenuLeftPane">

Restore two-space indentation for each element nesting level.

"Left" and "right" should be avoided, as it's reverse in right-to-left mode.

>+               <hbox flex="0" 

Is flex="0" doing anything useful?

+                     class="split-menuitem">
+                 <menuitem id="appmenu_editLabel" 
+                           class="split-menuitem-item" 
+                           flex="1" 
+                           label="&editMenu.label;" 
+                           disabled="true"/>

split-menuitem doesn't really fit here. Edit should be a label, not a menuitem.

+                 <spacer id="appmenuLeftPane-spacer"/>

This id doesn't seem specific enough. The whole element should probably be removed in favor of CSS margin or padding.

>+                 <menuitem id="appmenu_editLabel" 
>+                           class="split-menuitem-item" 
>+                           flex="1" 
>+                           label="&editMenu.label;" 
>+                           disabled="true"/>

Lots of trailing spaces here and elsewhere.

>+                                onclick="if (! this.disabled) hidePopup();" 

remove the space after !

>+                            onpopupshowing="BookmarksMenuButton.onPopupShowing(event);if (!this.parentNode._placesView)new PlacesMenu(event, 'place:folder=BOOKMARKS_MENU');this.appendChild(document.getElementById('appmenuBookmarkMenu_unsorted_seperator'));this.appendChild(document.getElementById('appmenuBookmarkMenu_unsorted-menuitem'));" 

This looks pretty messy... Insert some line breaks at least.

>+    -moz-box-shadow: 1px 0 3px 0 rgba(204,214,234,1) inset;

the second 0 is redundant

rgba(...,1) -> rgb()

>+#appmenu_quit  {

remove one space

>+#appmenu_addons > .menu-iconic-left{

add a space before {

>+  margin-right: -5px;
>+  margin-left: 5px;

use -moz-margin-start and -moz-margin-end

>+#appmenu-button > menuitem,
>+#appmenu-button > menu {

This doesn't match any nodes.

>+#appmenu-button #privateBrowsingItem {

remove #appmenu-button

#privateBrowsingItem needs a better name

>+#appmenu-button #subscribeToPageMenuitem:not([disabled]),
>+#appmenu-button #subscribeToPageMenupopup:not([hidden]) {

same as above

>+#appmenuRightPane {
>+  width: 150px;
>+}

This seems bogus, doesn't take varying font sizes and localization into account.

You're adding a bunch of entities for strings that already exist and are being used in a similar context. Can you reuse these strings?

Comment 106

[Joshua M [:soapy]]

(In reply to comment #105)
> Comment on attachment 466107 [details] [diff] [review]
> Patch v4 based on i6
> 
> Can you please adjust the diff context as per
> <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3f>?
> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> >@@ -3587,6 +3587,7 @@
> >   let editMenuPopupState = document.getElementById("menu_EditPopup").state;
> >   let contextMenuPopupState = document.getElementById("contentAreaContextMenu").state;
> >   let placesContextMenuPopupState = document.getElementById("placesContext").state;
> >+  let appMenuPopupState = document.getElementById("appmenu-popup").state;
> 
> This will fail on platforms where appmenu-popup doesn't exist.
> 
> >   var firstMenuItem = popup.firstChild;
> >+    if (aInsertPoint)
> >+      firstMenuItem = aInsertPoint;
> 
> var firstMenuItem = aInsertPoint || popup.firstChild;
> 
> >+<hbox id="appmenu-button-container">
> >+   <button id="appmenu-button" 
> >+           type="menu" 
> >+           label="&brandShortName;" 
> >+           style="-moz-user-focus: ignore;">
> >+     <menupopup id="appmenu-popup"
> >+                onpopupshowing="updateEditUIVisibility();">
> >+        <hbox>
> >+             <vbox id="appmenuLeftPane">
> 
> Restore two-space indentation for each element nesting level.
> 
> "Left" and "right" should be avoided, as it's reverse in right-to-left mode.
> 
> >+               <hbox flex="0" 
> 
> Is flex="0" doing anything useful?
> 
> +                     class="split-menuitem">
> +                 <menuitem id="appmenu_editLabel" 
> +                           class="split-menuitem-item" 
> +                           flex="1" 
> +                           label="&editMenu.label;" 
> +                           disabled="true"/>
> 
> split-menuitem doesn't really fit here. Edit should be a label, not a menuitem.
> 
> +                 <spacer id="appmenuLeftPane-spacer"/>
> 
> This id doesn't seem specific enough. The whole element should probably be
> removed in favor of CSS margin or padding.
> 
> >+                 <menuitem id="appmenu_editLabel" 
> >+                           class="split-menuitem-item" 
> >+                           flex="1" 
> >+                           label="&editMenu.label;" 
> >+                           disabled="true"/>
> 
> Lots of trailing spaces here and elsewhere.
> 
> >+                                onclick="if (! this.disabled) hidePopup();" 
> 
> remove the space after !
> 
> >+                            onpopupshowing="BookmarksMenuButton.onPopupShowing(event);if (!this.parentNode._placesView)new PlacesMenu(event, 'place:folder=BOOKMARKS_MENU');this.appendChild(document.getElementById('appmenuBookmarkMenu_unsorted_seperator'));this.appendChild(document.getElementById('appmenuBookmarkMenu_unsorted-menuitem'));" 
> 
> This looks pretty messy... Insert some line breaks at least.
> 
> >+    -moz-box-shadow: 1px 0 3px 0 rgba(204,214,234,1) inset;
> 
> the second 0 is redundant
> 
> rgba(...,1) -> rgb()
> 
> >+#appmenu_quit  {
> 
> remove one space
> 
> >+#appmenu_addons > .menu-iconic-left{
> 
> add a space before {
> 
> >+  margin-right: -5px;
> >+  margin-left: 5px;
> 
> use -moz-margin-start and -moz-margin-end
> 
> >+#appmenu-button > menuitem,
> >+#appmenu-button > menu {
> 
> This doesn't match any nodes.
> 
> >+#appmenu-button #privateBrowsingItem {
> 
> remove #appmenu-button
> 
> #privateBrowsingItem needs a better name
> 
> >+#appmenu-button #subscribeToPageMenuitem:not([disabled]),
> >+#appmenu-button #subscribeToPageMenupopup:not([hidden]) {
> 
> same as above
> 
> >+#appmenuRightPane {
> >+  width: 150px;
> >+}
> 
> This seems bogus, doesn't take varying font sizes and localization into
> account.
> 
> You're adding a bunch of entities for strings that already exist and are being
> used in a similar context. Can you reuse these strings?

I used the same ids for privateBrowsingItem,subscribeToPageMenuitem, and subscribeToPageMenupopup so the states could be controlled by the javascript already in place. If needed I can use unique ids and edit browser.js to reflect that.

The strings I added to browser.dtd are based on the Mockups, If we want to use entries that already exist I can remove them.

Comment 107

[Alex Faaborg [:faaborg] (Firefox UX)]

>The strings I added to browser.dtd are based on the Mockups

+<!ENTITY appMenuOpen.label "Open File…">

we can use openFileCmd.label ("Open File…")

>+<!ENTITY appMenuFind.label "Find">

This should just be "Find..." (adding the missing ellipsis).  In findOnCmd.label, the "in this page..." is now implied (although findOnCmd.label should stay the same for context menus).

>+<!ENTITY appMenuSaveAs.label "Save As…">

Same thing, in savePageCmd.label the "this page as..." is now implied (savePageCmd.label should stay the same for context menus).

>+<!ENTITY appMenuPrintCmd.label "Print…">

we can use printCmd.label ("Print...")

+<!ENTITY appMenuDeveloperTools.label "Developer Tools">

perhaps change developerMenu.label ("Developer")? I'm not sure how that is currently used.

+<!ENTITY appMenuConsole.label "Console">

mistake in the mockup, use webConsoleCmd.label "Web Console" instead (so users don't think that we might be opening a system console)

+<!ENTITY appMenuInspector.label "Inspector">

This appears to be new, inspectMenu.label ("Inspect") is probably used on a selection or context menu. 

+<!ENTITY appMenuViewPageSource.label "View Page Source">

We can use viewPageSourceCmd.label

+<!ENTITY appMenuViewBookmarks.label "View All Bookmarks">

This is new, and in the traditional menu bar should be used in place of organizeBookmarks.label ("Organize Bookmarks…")

+<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks">

This clearly exists (appears in the library window) but couldn't find the existing entity in browser.dtd

+<!ENTITY appMenuViewHistory.label "View All History">

This is new, changing the text of showAllHistoryCmd2.label ("Show All History").  The new text should also be used in the traditional menu bar

+<!ENTITY appMenuGettingStarted.label "Getting Started">

New to browser.dtd, since the existing string was used for a pre-populated bookmark

+<!ENTITY appMenuAboutProduct.label "About &brandShortName;">

Clearly exists, but I couldn't find the existing entity in browser.dtd

Comment 108

[Joshua M [:soapy]]

(In reply to comment #107)
> >The strings I added to browser.dtd are based on the Mockups
> 
> +<!ENTITY appMenuOpen.label "Open File…">
> 
> we can use openFileCmd.label ("Open File…")
> 
> >+<!ENTITY appMenuFind.label "Find">
> 
> This should just be "Find..." (adding the missing ellipsis).  In
> findOnCmd.label, the "in this page..." is now implied (although findOnCmd.label
> should stay the same for context menus).
> 
> >+<!ENTITY appMenuSaveAs.label "Save As…">
> 
> Same thing, in savePageCmd.label the "this page as..." is now implied
> (savePageCmd.label should stay the same for context menus).
> 
> >+<!ENTITY appMenuPrintCmd.label "Print…">
> 
> we can use printCmd.label ("Print...")
> 
> +<!ENTITY appMenuDeveloperTools.label "Developer Tools">
> 
> perhaps change developerMenu.label ("Developer")? I'm not sure how that is
> currently used.
> 
> +<!ENTITY appMenuConsole.label "Console">
> 
> mistake in the mockup, use webConsoleCmd.label "Web Console" instead (so users
> don't think that we might be opening a system console)
> 
> +<!ENTITY appMenuInspector.label "Inspector">
> 
> This appears to be new, inspectMenu.label ("Inspect") is probably used on a
> selection or context menu. 
> 
> +<!ENTITY appMenuViewPageSource.label "View Page Source">
> 
> We can use viewPageSourceCmd.label
> 
> +<!ENTITY appMenuViewBookmarks.label "View All Bookmarks">
> 
> This is new, and in the traditional menu bar should be used in place of
> organizeBookmarks.label ("Organize Bookmarks…")
> 
> +<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks">
> 
> This clearly exists (appears in the library window) but couldn't find the
> existing entity in browser.dtd
> 
> +<!ENTITY appMenuViewHistory.label "View All History">
> 
> This is new, changing the text of showAllHistoryCmd2.label ("Show All
> History").  The new text should also be used in the traditional menu bar
> 
> +<!ENTITY appMenuGettingStarted.label "Getting Started">
> 
> New to browser.dtd, since the existing string was used for a pre-populated
> bookmark
> 
> +<!ENTITY appMenuAboutProduct.label "About &brandShortName;">
> 
> Clearly exists, but I couldn't find the existing entity in browser.dtd

My mistake, I will remove duplicate entries.

Comment 109

[Justin Dolske [:Dolske]]

Comment on attachment 466107 [details] [diff] [review]
Patch v4 based on i6

This, plus the comments by Dao/Faaborg above, is looking pretty close. Could you get an updated patch up?

Code freeze for Beta 4 is today at noon PST (~11 hours from now). I don't know if we'll be able to squeeze this in, but I'll see if we can try. Obviously UX really wants this in for feedback on the planned (and now 99% implemented!) design.

Comment 110

[Tommy XP]

I noticed that there is no zoom in the menu. Google Chrome has a zoom. also, the menu stays active so you can tap zoom a couple times. Very intuitive design that firefox should copy.

Comment 111

[Joshua M [:soapy]]

Attachment: Patch v5 based on i6

Comment 112

[Vlado Valastiak [:wladow]]

Comment on attachment 466430 [details] [diff] [review]
Patch v5 based on i6

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd
>--- a/browser/locales/en-US/chrome/browser/browser.dtd
>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
>@@ -101,7 +101,7 @@
> <!ENTITY subscribeToPageMenupopup.label "Subscribe to This Page">
> <!ENTITY subscribeToPageMenuitem.label "Subscribe to This Page…">
> <!ENTITY addCurPagesCmd.label "Bookmark All Tabs…">
>-<!ENTITY organizeBookmarks.label "Organize Bookmarks…">
>+<!ENTITY organizeBookmarks.label "View All Bookmarks">
You need to rename the entity here, otherwise localizers won't get notified about this change. When doing so be sure not to break anything.

> <!ENTITY historyUndoWindowMenu.label "Recently Closed Windows">
> 
> <!ENTITY historyHomeCmd.label "Home">
>-<!ENTITY showAllHistoryCmd2.label "Show All History">
>+<!ENTITY showAllHistoryCmd2.label "View All History">
The same here.

>+<!ENTITY appMenuFind.label "Find…">
>+<!ENTITY appMenuDeveloperTools.label "Developer Tools">
There already is developerMenu.label for this, so either use this one or remove it in case you decide to go with new one.

>+<!ENTITY appMenuInspector.label "Inspector">
The same here, we already have inspectMenu.label. If we decide to go with appMenuInspector.label, we need a follow-up to make the item in Tools menu consistent with this one.

>+<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks">
This one already exists in toolkit's places.properties, but probably ok to have it duplicated here.

>+<!ENTITY appMenuAboutProduct.label "About &brandShortName;">
baseMenuOverlay.dtd -> aboutProduct.label

Comment 113

[Joshua M [:soapy]]

Attachment: Patch v6 based on i6

I have taken what I hope is the safe approach. I've removed all the extra strings from browser.dtd, reverted back to the default strings for a few, and added baseMenuOverlay to browser.xul.

Comment 114

[Dão Gottwald [:dao]]

Joshua, the patch will be significantly easier to review if you do this:

> Can you please adjust the diff context as per
> <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3f>?

Comment 115

[Joshua M [:soapy]]

(In reply to comment #114)
> Joshua, the patch will be significantly easier to review if you do this:
> 
> > Can you please adjust the diff context as per
> > <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3f>?

I'm really new to this, sorry. Can I contact you on irc?

Comment 116

[Joshua M [:soapy]]

Attachment: Patch v7 based on i6

I've edited my Mercurial config file and I hope this will provide better information to review this patch.

Comment 117

[Joshua M [:soapy]]

Attachment: Patch v7 based on i6

Comment 118

[Dão Gottwald [:dao]]

Comment on attachment 466634 [details] [diff] [review]
Patch v7 based on i6

Suggestions for better ids:

>       document.getElementById("privateBrowsingItem")
>               .setAttribute("disabled", "true");
>+#ifdef MENUBAR_CAN_AUTOHIDE
>+      document.getElementById("appmenu-PrivateItem")
>+              .setAttribute("disabled", "true");
>+#endif
>       document.getElementById("Tools:PrivateBrowsing")
>               .setAttribute("disabled", "true");

appmenu_privateBrowsing

>+        <vbox id="appmenuStartPane">

appmenuPrimaryPane

>+        <vbox id="appmenuEndPane">

appmenuSecondaryPane

Comment 119

[Joshua M [:soapy]]

Attachment: Patch v8 based on i6

Updated ids on a few items based on feedback from Dão.

Comment 120

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Tweak to the help menu

This returns troubleshooting information to the help menu based on comment #83.  The downside of placing this here is that now there is a different path to describe based on the user's version of Windows.  However, it does group nicely with a new command to start Firefox in safe mode (bug 542122).

If we can't add troubleshooting information for this patch, that's fine, we'll just file a follow up later.

Comment 121

[Dão Gottwald [:dao]]

The latest patch doesn't seem to apply on mozilla-central tip.

Comment 122

[Joshua M [:soapy]]

Attachment: Patch v8 based on i6

Fixed to work with latest trunk.

Comment 123

[Justin Dolske [:Dolske]]

Comment on attachment 467565 [details] [diff] [review]
Patch v8 based on i6

>+++ b/browser/base/content/browser.xul
...
>     <panel id="inspector-panel"
>            orient="vertical"
>            hidden="true"
>            ignorekeys="true"
>            noautofocus="true"
>            noautohide="true"
>+           level="top"

Oops, stray change, I believe?

Otherwise, this is fine and we should get this landed ASAP. Further tweaking and nitpicks can happen in followup bugs.

I'll make this fix when landing.

Comment 124

[Justin Dolske [:Dolske]]

Attachment: Nitpick update of Patch v.8

Comment 125

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/1504917ed42e

But backed out due what dao assures me is a 4% twinopen talos regression on Linux:
http://hg.mozilla.org/mozilla-central/rev/adbc1d5888f4

Comment 126

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #125)
> Pushed:
> http://hg.mozilla.org/mozilla-central/rev/1504917ed42e
> 
> But backed out due what dao assures me is a 4% twinopen talos regression on
> Linux:
> http://hg.mozilla.org/mozilla-central/rev/adbc1d5888f4

Sorry! I backed out the wrong patch, relanded:
http://hg.mozilla.org/mozilla-central/rev/c8c886655ea1

Comment 127

[pal-moz]

no icon/favicon for "History","Download","Options","Help"... ?

Comment 128

[Jim Jeffery not reading bug-mail 1/2/11]

Under Bookmarks the path to the Bookmarks Toolbar is missing. 

Unsorted should be near the top someplace not at the bottom of a lengthy list of bookmarks.

Comment 129

[Pavel Cvrcek [:JasnaPaka]]

This bug is marked as fixed. Please fill new bug if you think that something is wrong or you want to improve something.

Comment 130

[Dão Gottwald [:dao]]

I was close to backing this out because of the mess in bug 589163, which broke at least the Recently Closed Tabs menu...

Comment 131

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #130)
> I was close to backing this out because of the mess in bug 589163, which broke
> at least the Recently Closed Tabs menu...

We should either get immediate fixes in, or back this out and address that issue.

Comment 132

[Peter Henkel [:Terepin]]

What's the point using two "Options..." with the same functionality?

Comment 133

[Darren Kalck [:D-Kalck]]

The same as Print, New Tab and Help.

Comment 134

[u310142]

The white section of the menu needs some extra top and bottom margin like in the mockup.

Comment 135

[patrickjdempsey]

Just reiterating that I would like someone to consider comment #66.  The current UI approach is visually and functionally confusing, and a tiny change can make all the difference between a UI that makes some kind of sense and one that's confusing or at least unpredictable.

Comment 136

[u88484]

(In reply to comment #134)
> The white section of the menu needs some extra top and bottom margin like in
> the mockup.

File a new bug.

(In reply to comment #135)
> Just reiterating that I would like someone to consider comment #66.  The
> current UI approach is visually and functionally confusing, and a tiny change
> can make all the difference between a UI that makes some kind of sense and one
> that's confusing or at least unpredictable.

File a new bug.

Also, verifying this fixed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre

Comment 137

[patrickjdempsey]

(In reply to comment #136)
> (In reply to comment #135)
> > Just reiterating that I would like someone to consider comment #66.  The
> > current UI approach is visually and functionally confusing, and a tiny change
> > can make all the difference between a UI that makes some kind of sense and one
> > that's confusing or at least unpredictable.
> 
> File a new bug.

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

Comment 138

[u310142]

(In reply to comment #136)
> (In reply to comment #134)
> > The white section of the menu needs some extra top and bottom margin like in
> > the mockup.
> 
> File a new bug.

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

Comment 139

[Paul McAuley]

> On a side note, I'm still concerned about the edit icons. They're really
> unintuitive and ugly and will cause more issues und usability problems than a
> simple submenu would. See comment 20 for details. For example even I, as a
> experienced Windows user, need a while to decide which icon means paste and
> which means copy. New users who are afraid of shortcuts and therefore are the
> only ones who require those menu items will be hopelessy lost. Do some
> useability experiments if you don't believe me. Please.

I agree. The only Edit options that are actually required in the menu are undo/redo and find. Everything else is better in the superior right-click context menu. I did a detailed analysis of this here:
https://wiki.mozilla.org/User:Broccauley/Firefox_Menu_Explorations#Placement_of_Contents_of_the_Edit_Menu

Comment 140

[Paul McAuley]

Following on from creating some of the original inspiration mock-ups for this, I've made new mock-ups with my updated take on the 2-pane concept and feedback on this "i6" iteration at the bottom:
https://wiki.mozilla.org/User:Broccauley/Firefox_Menu_Explorations

Comment 141

[ruiderson]

Friends, I don't know where is the best place to give sugestions, but i have an idea for Private Browsing in Firefox Menu. The menu should have an option to start a New Private Browsing, a New Private Windows ( like in Google Chrome ), and a New Private Tab ( like in Opera ). I think Private tabs can be a good idea because we can use Panorama to create private and non-private groups, or a private tab and a non-private tab together in the same group, and easy change browsing modes in the same window, increasing the acessibility.

Sorry if I'm writing in the wrong place, I don't want to waste your time.
Thanks.

Comment 142

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #141)
> Friends, I don't know where is the best place to give sugestions, but i have an
> idea for Private Browsing in Firefox Menu. The menu should have an option to
> start a New Private Browsing, a New Private Windows ( like in Google Chrome ),
> and a New Private Tab ( like in Opera ). I think Private tabs can be a good
> idea because we can use Panorama to create private and non-private groups, or a
> private tab and a non-private tab together in the same group, and easy change
> browsing modes in the same window, increasing the acessibility.
> 
> Sorry if I'm writing in the wrong place, I don't want to waste your time.
> Thanks.

You're going to have to wait for electrolysis to progress further before that can happen, if I'm not mistaken.

Comment 143

[Tan Wei Lin Jason [:Ryuji]]

When is the target Milestone of electrolysis? is it Firefox 5?

Comment 144

[u88484]

(In reply to comment #140)
> Following on from creating some of the original inspiration mock-ups for this,
> I've made new mock-ups with my updated take on the 2-pane concept and feedback
> on this "i6" iteration at the bottom:
> https://wiki.mozilla.org/User:Broccauley/Firefox_Menu_Explorations

Great concept.  You should file specific bugs for some of those suggestions.

Comment 145

[imradyurrad]

Sorry for the spam folks, but is there a bug for addon/extension menus not being in the FF button? 

https://bug583386.bugzilla.mozilla.org/attachment.cgi?id=465479

Comment 146

[Alex Faaborg [:faaborg] (Firefox UX)]

yikes, I may have dropped the ball on that.  I've filed bug 599788 and requested blocking.

>Sorry for the spam folks

Catching a possible omission like that is the opposite of spam, thanks for pointing it out!

Comment 147

[Dave Buco]

Am I correct in assuming that addon/extension menus in the Firefox button will not happen?

http://oi52.tinypic.com/1qihef.jpg

Comment 148

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #147)
> Am I correct in assuming that addon/extension menus in the Firefox button will
> not happen?
> 
> http://oi52.tinypic.com/1qihef.jpg

See the bug in comment #146 above...