Closed Bug 585370 Opened 14 years ago Closed 14 years ago

Implement the Firefox button on Linux

Categories

(Firefox :: General, defect)

All
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: wgianopoulos, Assigned: wgianopoulos)

References

(Depends on 1 open bug)

Details

(Whiteboard: [see comment 69])

Attachments

(5 files, 52 obsolete files)

1006 bytes, patch
dao
: review+
dietrich
: approval2.0+
Details | Diff | Splinter Review
27.48 KB, image/png
Details
14.90 KB, image/png
Details
11.17 KB, patch
Dolske
: approval2.0+
Details | Diff | Splinter Review
28.91 KB, image/png
faaborg
: ui-review+
Details
It seems that the entire look and feel of Firefox is being changed under Windows for Firefox 4, but Linux will be left with the Firefox 3 look.

The reason for this seems to be that there seems to be some unnecessary tie-in in developers minds between implementing the Firefox button and enabling drawing in the titlebar.

It seems to me that implementing making the menubar be hide-able and having the Firefox button at the left of the tabbar as Firefox Beta3 is going to work under Windows/XP should be easy to implement under Linux to give people the option to use the old UI or chose a more Firefox 4 Windows-like UI by choosing to hide the menubar under Linux.
blocking2.0: --- → ?
Blocks: 572482
It would also be great to be able to hide (not draw in) the titlebar when maximized to get the full intended effect when maximized, if not windowed.
Component: Theme → General
QA Contact: theme → general
Blocks: 580741
Stephen has more information here, but the Linux distributions suggested that we not do this due to upcoming platform changes on their side.
blocking2.0: ? → -
I would suggest that Firefox builds from Mozilla simply aim to be as updated and streamlined as possible, and if any Linux distributions wish to do something differently they can just customize their build as desired. In other words, don't penalize everyone else because of what any individual distos may want. It's not like we can get everyone to agree on everything.
(In reply to comment #3)
> I would suggest that Firefox builds from Mozilla simply aim to be as updated
> and streamlined as possible, and if any Linux distributions wish to do
> something differently they can just customize their build as desired. In other
> words, don't penalize everyone else because of what any individual distos may
> want. It's not like we can get everyone to agree on everything.

The ability to natively draw widgets in the titelbar should come with gtk3. This will give us the ability to do what we want the right way. Doing this entirely on the Firefox side would be a lot of work for a possibly degraded experience that would soon become redundant. 

I think what is work doing is implementing the Firefox button on Linux as an option in much the same way it is presently working on Windows XP.
(In reply to comment #4)
> The ability to natively draw widgets in the titelbar should come with gtk3.
> This will give us the ability to do what we want the right way. Doing this
> entirely on the Firefox side would be a lot of work for a possibly degraded
> experience that would soon become redundant. 
> 
> I think what is work doing is implementing the Firefox button on Linux as an
> option in much the same way it is presently working on Windows XP.

I am wondering if there is objection from some of the distros to trying to make the ALT key do a function when pressed alone?  After all it is supposed to be a modifier key which by definition performs no action on its own but merely modifies some other keyboard or mouse action that is performed in conjunction with pressing the key.

The reason I ask is that I have a patch that I have been running on my own system a few days that gets around the issue of the ALT key not being easily made to make the Menubar appear under Linus by using a trick of adding an empty menu to the menubar with an access key of "M" so that you make the menubar appear by pressing ALT+M.

What I like about this approach is:

1.  It is a minimal patch
2.  It does not pervert the definition of ALT as a modifier key.

What I don't like is:

1.  It is not exactly the way it works under windows.
2.  Although ALT+M makes sense in an en-US build, I have no idea about other localizations.  IT might not be particularly mnemonic, or worse yet might already be used as an access key for one of the other menubar choices.  I think perhaps the key sequence used here should be more universal.
(In reply to comment #5)

Not sure that this is on-topic for this bug.
(In reply to comment #6)
> (In reply to comment #5)
> 
> Not sure that this is on-topic for this bug.

Well, the current situation is that there is no code that needs to be written to implement the Firefox Button under Linux (except that it is currently entirely unthemed and is really just a text button with a dropmarker).  All the code exists in the tree but is non-functional because the menubar autohide function has been disabled under Linux becuase of the issue of not being able to do the unhide with the press of the ALT key alone.
Depends on: 552302
(In reply to comment #4)
> The ability to natively draw widgets in the titelbar should come with gtk3.
> This will give us the ability to do what we want the right way. Doing this
> entirely on the Firefox side would be a lot of work for a possibly degraded
> experience that would soon become redundant. 

As far as I can tell, GTK+3 won't be released for quite some time.  It appears that the API is still under heavy development, and the latest Gnome snapshot (2.31.6) just released a few days ago is still built on GTK+2.  I think it would be a mistake to wait on GTK3, unless we are talking about Firefox 5 or 6.
Sorry to post twice in a row, but I wanted to make a suggestion.

Why not consider moving the firefox button into the navigation bar / tab bar bookmarks bar?  In addition to API compatibility problems, drawing in the title bar area might be a larger issue for some non-GTK based Window managers.
In reply to comment 7: To repeat something from bug 552302 here: addons have existed for quite a while that can hide/show the menu bar under Linux just fine. There's no reason to not add the ability by default to match other platforms. Again, if a distro disagrees let them deal with their build themselves.

In reply to comment 9: comment 0 already suggests the tab bar, which is done on Windows XP and is the most logical and simple thing to do here.
(In reply to comment #8)
> As far as I can tell, GTK+3 won't be released for quite some time.  It appears
> that the API is still under heavy development, and the latest Gnome snapshot
> (2.31.6) just released a few days ago is still built on GTK+2.  I think it
> would be a mistake to wait on GTK3, unless we are talking about Firefox 5 or 6.

I believe the concern here is not that we should wait for GTK3 before doing anything, just try not to do a kludge that works with GTK2 that is likely to make Firefox incompatible with GTK3.

I don't think that having everyone working on GTK3 development switch to no longer using Firefox as their default browser because it is incompatible, is a desirable outcome here.
One thing that has yet to be mentioned: If the Firefox button isn't implemented under Linux, this effectively drops Linux support for any addons that intend to add entries to the menu. No button -> errors overlaying + developer who doesn't touch Linux = no Linux support => not really fully supported OS anymore.
Requesting blocking for at least optional support as per Stephen in comment 4.
blocking2.0: - → ?
(In reply to comment #12)
> One thing that has yet to be mentioned: If the Firefox button isn't implemented
> under Linux, this effectively drops Linux support for any addons that intend to
> add entries to the menu. No button -> errors overlaying + developer who doesn't
> touch Linux = no Linux support => not really fully supported OS anymore.

We'll have no Firefox button on WinXP or Win2K, either. The idea is that add-on authors will overlay a hook which puts the menuItems in the appopriate place, I believe.
(In reply to comment #14)
> We'll have no Firefox button on WinXP or Win2K, either.

WinXP has the button, it's just hidden by default. (I'm not sure about Win2K, but I think it's the same as WinXP; correct me if not) Linux doesn't have it at all.
(In reply to comment #14)
> The idea is that add-on authors will overlay a hook which puts the 
> menuItems in the appopriate place, I believe.

Is that possible atm? It would be great if we could provide a semi-official extension for this. Just a toolbar button which calls up the menu. So, all we really need is a way to access this menu from extensions.
(In reply to comment #16)
> (In reply to comment #14)
> > The idea is that add-on authors will overlay a hook which puts the 
> > menuItems in the appopriate place, I believe.
> 
> Is that possible atm? It would be great if we could provide a semi-official
> extension for this. Just a toolbar button which calls up the menu. So, all we
> really need is a way to access this menu from extensions.

I have an extension that I support that adds menu item to either the classic file menu or the Firefox menu or both depending on shat is available.  It is relatively easy to do.
Attached patch possible solution (obsolete) — Splinter Review
This is the patch I alluded to in comment #5.

I suspect that due to the lack of anyone who says there is this code that works perfectly had provided a reviewable patch to date.  This is the best path going forward,.  If someone comes along with an approved patch to allow the ALT key alone to un-hide the menubar then that is great, but since that does not appear to be forthcoming, I am hoping that perhaps this will be accepted as the best way to get some support for the Firefox button under Linux in time for the Firefox 4 release.

The patch has 3 parts.

1.  The code in browser/base/content/browser-menubar.inc and browser/locales/en-US/chrome/browser/browser.dtd implements the ATL-M to un-hide the Menubar.

2.  The code in toolkit/content/xul.css reverts the code from bug 498628 which disabled autohide under Linux because there was no key combination to unhide the menubar without opening a menu.   This limitation is fixed by part 1 of this patch.

3.  The code in browser/base/Makefile.in defines MENUBAR_CAN_AUTOHIDE for Linux which enables the Firefox button menu.

Builds containing this patch are available at my usual test location:

http://www.wg9s.com/mozilla/firefox/
Alt-M is not a good way to go, I think.

Again, the Hide Menubar extension shows with just a press of alt and works just fine. I'm using it right now, and have been since the Personal Menu extension switched to using it instead of it doing the exact same thing itself for a very long time. There have been extensions that have allowed hiding of the menu bar and reshowing it by pressing alt for years, and Linux is supported just fine. I see no reason to not just use alt like elsewhere, yanking any code or ideas from one of the many extensions that have implemented this if needed.
(In reply to comment #19)
> Alt-M is not a good way to go, I think.
> 
> Again, the Hide Menubar extension shows with just a press of alt and works just
> fine. I'm using it right now, and have been since the Personal Menu extension
> switched to using it instead of it doing the exact same thing itself for a very
> long time. There have been extensions that have allowed hiding of the menu bar
> and reshowing it by pressing alt for years, and Linux is supported just fine. I
> see no reason to not just use alt like elsewhere, yanking any code or ideas
> from one of the many extensions that have implemented this if needed.

The hide menubar extension works by putting a lot of raw event handling code in the XUL level that is required for Linux only.  Code at the XUL level should be platform agnostic.  Therefore, this approach is really a non-starter as a solution.

Linux only code would really need to be in the back-end someplace and not a kludge adde at the XUL layer.  Just my opinion.  I could be wrong.
(In reply to comment #20)
> The hide menubar extension works by putting a lot of raw event handling code in
> the XUL level that is required for Linux only.  Code at the XUL level should be
> platform agnostic.  Therefore, this approach is really a non-starter as a
> solution.
> 
> Linux only code would really need to be in the back-end someplace and not a
> kludge adde at the XUL layer.  Just my opinion.  I could be wrong.

Then either some other better implementation can be done or the same hacks can be done better. One way or another, it's quite doable. We may have to simply accept that the Right Way™ may not be an option and the hacky way is best for now.

(In reply to comment #17)
> I have an extension that I support that adds menu item to either the classic
> file menu or the Firefox menu or both depending on shat is available.  It is
> relatively easy to do.

This requires thought; some developers don't make with the thinking as much as they should. The most common way I've seen people not support Linux is just because they never thought to. If everything the developer has seen has the Firefox button, I see no reason why some wouldn't rely on its existence. (or shouldn't for that matter) One way or another, any big platform inconsistencies like this are problems for extension and also theme support.
Well, one of the issues is also that one of the major complaints about Firefox is that compared to chrome, it is a bit slow and klunky.  Trying to process more raw keyboard events at the XUL level, as these extensions do, is probably not step in the direction toward making things faster.
Bill, I just tested your latest build and while it does show a firefox button, it does so in a separate row and still shows the old menubar.

The only real place for the firefox button to live would be in front of the first tab or as configurable toolbar button.
(In reply to comment #23)
> Bill, I just tested your latest build and while it does show a firefox button,
> it does so in a separate row and still shows the old menubar.
> 
> The only real place for the firefox button to live would be in front of the
> first tab or as configurable toolbar button.

Yes, my plan was that if the approach in this patch were accepted then I would get it to work similar to the way it does under windows XP in the beta3 build.  The button would be to the left of the tabs if tabs-on-top is selected and on a separate row otherwise.
(In reply to comment #24)
> The button would be to the left of the tabs if tabs-on-top is selected and on a
> separate row otherwise.

We really need some UI decisions here... but having the button on a separate row doesn't make a lot of sense, after all the point is to reduce chrome.
On Linux, it's supposed to be on the same line as the tab bar, but not in the window chrome for technical reasons (until GNOME adds this capability), so like this:

===== Window title ======================[_][x]
[Firefox v] [ tab 1 ] [ tab 2 ][+] 
[<] [>]  [http://example.com]
@limi, what happens if the option "Always show the tab bar" is not ticked for Linux?
(In reply to comment #27)
> @limi, what happens if the option "Always show the tab bar" is not ticked for
> Linux?

I would say we don't care and just display

===== Window title ======================[_][x]
[<] [>]  [http://example.com]
[Firefox v] [ tab 1 ] [ tab 2 ][+]

Easiest to implement and probably not widely used.

NOTE: there is a problem with tabs-on-top and tabs dragging when there is not menu. Try it. I think we need to increase top padding in that case and also use a smaller tab drop indicator.
I looked into how the hack for this on Windows XP works in the beta, and unfortunately it is just that.  It is roughly the equivalent of this code I just added to my userChrome.css file.

#navigator-toolbox[tabsontop="true"] > #toolbar-menubar[autohide="true"] ~ #TabsToolbar:not([inFullscreen]) {
  -moz-padding-start: 7.5em !important;
  margin-top: -1.7em !important;
}

As you can see the spacing here is dependent on the length of brandShortname and fonts being used.
I should have mentioned that I shared that css just to provide kind of a starting point to talk about the possible layout.

It really does not work correctly.  The button ends up not being clickable except along the top and at the area where the brandShortName appeared, the drop marker and right margin as well as the area under the label are not clickable.

It also does not work correctly with tabs-on-top in full screen mode.  The label appears over the navigation bar.

It also does not display correctly when in toolbar customization mode if you have the menubar hidden and tabs-on-top selected.
A better UserChrome.css hack:

#navigator-toolbox[tabsontop="true"] > #toolbar-menubar[autohide="true"] ~ #TabsToolbar:not([inFullscreen]) {
  margin-left: 7.3em !important;
  margin-top: -2.375em !important;
}
#titlebar-spacer, #titlebar-buttonbox {
  display: none !important;
}

This works correctly with the tabcandy and display all tabs icon and tweaks the margins to make things actually align correctly.  It still is a bad hack that depends on the length of brandShortName and also does not have all of the appbutton being click-able as it should be.
I should have mentioned that my builds at http://www.wg9s.com/mozilla/firefox/  now include both the ATL+M hack, and this CSS hack, built into the default Linux theme.
Comment on attachment 465030 [details] [diff] [review]
possible solution

Asking for review on this.  Not because I think this is the absolute correct solution, but because with the current situation of the application button being dependent on autohide, and autohide being disabled because of a lack of a way under Linux to un0autohide without opening a men u pop-up, this is a way to permit people to work on how to do the theme work on the application button under Linux in parallel with someone smarter than me figuring out a way to get this to work with just the ALT key.

I think this is a way to move forward here by providing a method to un-hide under Linux that does not open a pop-up, although it is via a different key-sequence than what works under Windows.
Attachment #465030 - Flags: review?(dao)
There is a third option, you know: no auto-unhide. Just let the menu bar be toggled via the menu and not by alt or a hotkey. Just put the toggle menu entry in the Firefox button menu somewhere and have that keyboard accessible as would be the menu entry in the regular menu.
(In reply to comment #34)
> There is a third option, you know: no auto-unhide. Just let the menu bar be
> toggled via the menu and not by alt or a hotkey. Just put the toggle menu entry
> in the Firefox button menu somewhere and have that keyboard accessible as would
> be the menu entry in the regular menu.

Well, you know, that is kind of my feeling as well.  The only reason we currently need the hot-key unhide is because the check for updates is not an option under the application button and because of extensions that need to make the options they add under tools in the classic menu be somehow available under the new applications menu.
So having the applications button appear should be dependent upon the menubar not being displayed.  Rather than on it's ability to autohide, as it is now.
(In reply to comment #34)
> There is a third option, you know: no auto-unhide. Just let the menu bar be
> toggled via the menu and not by alt or a hotkey. Just put the toggle menu entry
> in the Firefox button menu somewhere and have that keyboard accessible as would
> be the menu entry in the regular menu.

See bug 575279 comment 6...
Also, somebody would actually have to make all App button menu contents keyboard accessible, not just the button itself.
This implements the same theme code as in Winstripe for the Windows/XP aero basic case.
Attachment #467085 - Attachment is obsolete: true
Assignee: nobody → bill
Status: NEW → ASSIGNED
Comment on attachment 465030 [details] [diff] [review]
possible solution

Realized Dao did not seem to be a peer for this module. Requesting a review from Gavin.
Attachment #465030 - Flags: review?(dao) → review?(gavin.bugzilla)
Comment on attachment 467143 [details] [diff] [review]
theme code for application button with tabs-on-top v2

This is essentially the same fix checked in for Windows in bug 574434.
Attachment #467143 - Flags: review?(gavin.sharp)
Attachment #465030 - Flags: review?(gavin.bugzilla) → review?(gavin.sharp)
Dao can review this (and is probably a better choice).
(In reply to comment #42)
> Dao can review this (and is probably a better choice).

OK.  That is what I thought originally as much of the first patch is just re-instating code he had removed in the first place because he did not like the way hiding the menubar worked under Linux.
Attachment #465030 - Flags: review?(gavin.sharp) → review?(dao)
Attachment #467143 - Flags: review?(gavin.sharp) → review?(dao)
Comment on attachment 465030 [details] [diff] [review]
possible solution

>--- a/browser/base/content/browser-menubar.inc
>+++ b/browser/base/content/browser-menubar.inc
>@@ -683,9 +683,14 @@
> #endif
>               </menupopup>
>             </menu>
> 
> #ifdef XP_MACOSX
>           <menu id="windowMenu" />
> #endif
>           <menu id="helpMenu" />
>+#ifdef MOZ_WIDGET_GTK2
>+          <menu id="menu-menu" label=" "
>+            accesskey="&menuBar.accesskey;">
>+          </menu>
>+#endif
>         </menubar>

This seems entirely non-discoverable. At that point we could as well have no shortcut for showing the menu bar without opening a menu -- which might be okay as long as we don't hide the menu bar by default.
Attachment #465030 - Flags: review?(dao) → review-
Comment on attachment 467143 [details] [diff] [review]
theme code for application button with tabs-on-top v2

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css
>@@ -1190,16 +1190,35 @@ statusbarpanel#statusbar-display {
> /* Tabs */
> #TabsToolbar {
>   -moz-appearance: none;
>   min-height: 0;
>   padding: 0;
>   -moz-box-shadow: ThreeDShadow 0 -1px inset;
> }
> 
>+/* XXX: stop-gap until the Application button can be drawn in the title bar */

Unlike on Windows, we're not going to be able to do this on Linux soon enough. It's a hack and only acceptable on Windows because we know it's going away. We should just make it a toolbar item on Linux.
Attachment #467143 - Flags: review?(dao) → review-
(In reply to comment #44)
> This seems entirely non-discoverable. At that point we could as well have no
> shortcut for showing the menu bar without opening a menu -- which might be okay
> as long as we don't hide the menu bar by default.

I kind of thought it a bit kludgy myself.  The main reason I offered it was if there was a plan to get this to work with just the ALT button eventually this could be landed first to give theme people a chance to work on the theme in relation to the app button using this code.

I still think the entire idea of the application button being dependent on the ability to hide the menubar is just wrong.

What about a preference for menu styhle with 3 values:

Application button
Classic manubar
both

With the both choice only being offered on platfroms where the auto-hide is supported?
(In reply to comment #45)
> Unlike on Windows, we're not going to be able to do this on Linux soon enough.
> It's a hack and only acceptable on Windows because we know it's going away. We
> should just make it a toolbar item on Linux.

I love the idea of this being a toolbar button, so presumably this could be placed where desired, including on the left side of the bookmarks toolbar.
(In reply to comment #45)
>> We should just make it a toolbar item on Linux

There should really be UX input for this but personally I think showing a
toolbar item next to the bookmarks button only when the menubar is hidden would
work best and require the least amount of hacks. Making the button placable by the user is not what we want IMHO.

We could style this with some orange icon to match the firefox button on windows.

CONS:
- menu access on the right instead of the left

PROS:
- choice for the user to get rid of the menu bar
- easy to implement
- losing about 30 pixels of the awesomebar is cheap compared to losing quite
some space in the tab bar
- similar to google chrome
- similar to how many GNOME 3 apps will handle this (I can provide current mockups if anyone is interested)

In the end let's not forget that this will not be the default
Placement as a toolbar icon is by far the best proposed solution, IMHO.  This likely wins in the long-term too.  Even if GTK-3 allows the placement of such buttons in the titlebar, that doesn't mean that all window managers will support this -- especially the more obscure WM's.
(In reply to comment #48)
> (In reply to comment #45)
> >> We should just make it a toolbar item on Linux
> 
> There should really be UX input for this but personally I think showing a
> toolbar item next to the bookmarks button only when the menubar is hidden would
> work best and require the least amount of hacks. Making the button placable by
> the user is not what we want IMHO.
> 
> We could style this with some orange icon to match the firefox button on
> windows.
> 
> CONS:
> - menu access on the right instead of the left
> 
> PROS:
> - choice for the user to get rid of the menu bar
> - easy to implement
> - losing about 30 pixels of the awesomebar is cheap compared to losing quite
> some space in the tab bar
> - similar to google chrome
> - similar to how many GNOME 3 apps will handle this (I can provide current
> mockups if anyone is interested)
> 
> In the end let's not forget that this will not be the default

(In reply to comment #48)
> (In reply to comment #45)
> >> We should just make it a toolbar item on Linux
> 
> There should really be UX input for this but personally I think showing a
> toolbar item next to the bookmarks button only when the menubar is hidden would
> work best and require the least amount of hacks. Making the button placable by
> the user is not what we want IMHO.
> 
> We could style this with some orange icon to match the firefox button on
> windows.
> 
> CONS:
> - menu access on the right instead of the left
> 
> PROS:
> - choice for the user to get rid of the menu bar
> - easy to implement
> - losing about 30 pixels of the awesomebar is cheap compared to losing quite
> some space in the tab bar
> - similar to google chrome
> - similar to how many GNOME 3 apps will handle this (I can provide current
> mockups if anyone is interested)
> 
> In the end let's not forget that this will not be the default

The whole point of making it placeable by the user is that the user can make the choice on the trade-off between losing 30 pixels on the awesomebar and "losing quite some space in the tab bar".

Users who do not typically have more that 4 or 5 tabs open, would probably prefer this on the tabbar.
(In reply to comment #48)
> - losing about 30 pixels of the awesomebar is cheap compared to losing quite
> some space in the tab bar

Oh I really don't like the way this is done under windows.  I think this belongs on the left ot the tabs with tabs-on top and instead of being brandShortname with a dropmarker, it should merely be the Firefox icon, which would not require a great deal of horizontal real-estate.
> The whole point of making it placeable by the user is that the user can make
> the choice on the trade-off between losing 30 pixels on the awesomebar and
> "losing quite some space in the tab bar".
> 
> Users who do not typically have more that 4 or 5 tabs open, would probably
> prefer this on the tabbar.

This is a good point, but simplicity of development should also be considered. 
It seems to me that such functionality is already built into the toolbar, but
the tabbar doesn't really support adding buttons -- at least not in Firefox
3.6.
(In reply to comment #52)
> > The whole point of making it placeable by the user is that the user can make
> > the choice on the trade-off between losing 30 pixels on the awesomebar and
> > "losing quite some space in the tab bar".
> > 
> > Users who do not typically have more that 4 or 5 tabs open, would probably
> > prefer this on the tabbar.
> 
> This is a good point, but simplicity of development should also be considered. 
> It seems to me that such functionality is already built into the toolbar, but
> the tabbar doesn't really support adding buttons -- at least not in Firefox
> 3.6.

Sigh!  You are correct.  I should have checked that first.
Assignee: bill → nobody
Status: ASSIGNED → NEW
(In reply to comment #52)
> It seems to me that such functionality is already built into the toolbar, but
> the tabbar doesn't really support adding buttons -- at least not in Firefox
> 3.6.

That is possible right now (see new tab and tabcandy buttons... tabs basicly live in a toolbar now). But the discussion is not really going anywhere. The UX team needs to make a decision.
Attached patch suggestion from comment 51 (obsolete) — Splinter Review
Posting for reference only.
Minusing; the UX team can renominate if they think that this blocks the release.
blocking2.0: ? → -
I personally don't think this should be blocking 4.0 if we can't make it, agreed.

As for the intended behavior: Ideally, we'd have the option to show the Firefox menu on the same level as the tab strip, as earlier specified:

===== Window title ======================[_][x]
[Firefox v] [ tab 1 ] [ tab 2 ][+] 
[<] [>]  [http://example.com]

If people turn off the tab bar, when they have added the Firefox menu button to it, it goes away with the tab bar. It should behave like (and I assume, *be*) a normal toolbar button. (not sure what the correct terminology here is).
(In reply to comment #57)
> I personally don't think this should be blocking 4.0 if we can't make it,
> agreed.
> 
> 
I think this definitely should block the release. It'll be a shame if Linux version releases with the old interface and will lower the popularity of Firefox on Linux.
(In reply to comment #58)
> I think this definitely should block the release. It'll be a shame if Linux
> version releases with the old interface and will lower the popularity of
> Firefox on Linux.

That's not why the menu button exists; Linux doesn't have this kind of UI, whereas Windows 7 & Vista do.

Linux is like Mac OS X in this respect, where the menu bar is the established way to interact with an application. We'd like to offer the option to use the new-style menu button on platforms that don't normally use this as an option (see: OS X, XP, Linux), but it's not going to block the release in itself.
>Linux doesn't have this kind of UI

Although ironically the Firefox button may have influenced some of the decisions being made for Gnome's UI, in which case we may eventually get a native control to use :) (talk about coming full circle).

But yeah, in the meantime our overall goal is to create an interface that is native and familiar given the interactive design of the surrounding operating system.
(In reply to comment #60)
> >Linux doesn't have this kind of UI
> 
> Although ironically the Firefox button may have influenced some of the
> decisions being made for Gnome's UI, in which case we may eventually get a
> native control to use :) (talk about coming full circle).
> 
> But yeah, in the meantime our overall goal is to create an interface that is
> native and familiar given the interactive design of the surrounding operating
> system.

Well the problem with that approach is that I assume I am not the only one who develops Firefox extensions under Linux that add items to the Menu.  Without having the New Application Button menu in some way usable under Linux this severely impacts my ability to properly support my extensions.

Not having some way to have this work at least with a hidden preference is a hinderence to extension developers.
When fun things like this are being considered:
https://bug592147.bugzilla.mozilla.org/attachment.cgi?id=470654

Not having the Firefox button, at least optionally, means not being real full Firefox. People will extend this. It must be in.

(In reply to comment #59)
> We'd like to offer the option to use the
> new-style menu button on platforms that don't normally use this as an option
> (see: OS X, XP, Linux), but it's not going to block the release in itself.

It will be an awful mess in addon land, both extensions and themes, if Firefox starts dropping cross-platform support in any part. Not having the button on certain platforms does just that. I don't see how this can't block.
Attachment #468043 - Attachment is obsolete: true
Attachment #465030 - Attachment is obsolete: true
Attachment #467143 - Attachment is obsolete: true
I obsoleted all the patches here as they no longer apply cleanly and also did not work correctly if your OS theme was a dark background with white text.

If anyone is still interested in this approach, my builds and patches are available at http://www.wg9s.com/mozilla/firefox/
I agree with comment #62.

In my opinion, this issue should block release, definitely.

Waiting for GTK+ 3 is not a way IMO, ie. Debian's next release (Squeeze, frozen
now) will contain GTK+ 2 - GTK+ 3 will be (hopefully) present in next stable
release (2014/2015 - just guessing). So Firefox 4/5/6 will look like Firefox 3
until then? Strange.

I don't like platform specific UI changes, this is like saying "If you want
pretty Firefox, don't use Linux" or "Linux is not supported".
Google chrome seems to be able to do this just fine without waiting for gtk3, so although the GTK developers might prefer not to have a lot of applications doing this in a way that might not be compatible with what they plan for GTK3, the genie is already out of the bottle.  It would seem at this point that waiting is just letting the competition get way ahead.
Attached image Chrome vs Firefox screenshot (obsolete) —
Don't let us start this discussion again. Doing what Chrome does is not really an option because it doesn't integrate nicely with window management (no context menu) and also does not allow to preserve the style of window deco. See the attached screenshot, it shows a Chrome window (active) as well as a terminal window (inactive) and Firefox (active). As you see, Chrome looks totally out of place.

Doing the "implement as button and place before first tab" is still the way to go.
What about the way it is done in Opera?
Proposal to lower the chatter in here:
* Restrict this bug to the simple proposal (comment 26)
* File a new bug for debating fancier routes after that
The desired approach for a Linux Firefox Button would be:

- Firefox button as a customizable toolbar item
- Menubar ON by default
- Add option to hide the menubar manually on Linux

 View > Toolbars > √ Menu Bar
                   √ Navigation Toolbar
                   √ Bookmarks Toolbar
                   --------------------
                   √ Tabs on top
                   --------------------
                     Customize…

- If the menubar is hidden manually the Firefox button appears next to the tabs by default but can be moved anywhere
- No menubar access key (the Firefox menu should be self sufficient and the alt key convention is Windows specific)
(In reply to comment #69)
Sounds perfect.

Any suggestion to allow other OSes to move the Firefox button around too?
Whiteboard: [see comment 69]
(In reply to comment #69)
> The desired approach for a Linux Firefox Button would be:
> 
> - Firefox button as a customizable toolbar item
> - Menubar ON by default
> - Add option to hide the menubar manually on Linux
> 
>  View > Toolbars > √ Menu Bar
>                    √ Navigation Toolbar
>                    √ Bookmarks Toolbar
>                    --------------------
>                    √ Tabs on top
>                    --------------------
>                      Customize…
> 
> - If the menubar is hidden manually the Firefox button appears next to the tabs
> by default but can be moved anywhere
> - No menubar access key (the Firefox menu should be self sufficient and the alt
> key convention is Windows specific)

There are issues with this approach.  Most notably that it requires supporting the placement of toolbar icons on the bookmarks toolbar which is not currently supported and last I knew feature freeze was scheduled for Friday.

Also making this a toolbar butt that can be places anywhere via customize toolbars requires that the button have a text label that displays if the user selects display icons and text or text only.  As far as I know string freeze for such a label is also scheduled for Friday.

Toolbar icons currently have the option to display large icons or small icons bot with or without the text label and also have the option to display the text label only.

For this purpose I really don;t think any option makes sense except displaying small icon only without text.  So, there are a lot of UI issues to be determined none of which will be solved by Friday.
(In reply to comment #70)
> (In reply to comment #69)
> Sounds perfect.
> 
> Any suggestion to allow other OSes to move the Firefox button around too?

Which other OSes are you referring to?  The only other primary supported OS is for OSX and Apple is very draconian about their HID which sort of requires the traditional menubar.
(In reply to comment #71)
> There are issues with this approach.  Most notably that it requires supporting
> the placement of toolbar icons on the bookmarks toolbar which is not currently
> supported and last I knew feature freeze was scheduled for Friday.

I don't understand what you mean by this. You can currently place toolbar icons on the bookmarks toolbar.


> Also making this a toolbar butt that can be places anywhere via customize
> toolbars requires that the button have a text label that displays if the user
> selects display icons and text or text only.  As far as I know string freeze
> for such a label is also scheduled for Friday.

Wouldn't the label be whatever is being displayed on the button? "Minefield" or "Firefox". Technically it could go entirely label-less since the button is self describing.


> Toolbar icons currently have the option to display large icons or small icons
> bot with or without the text label and also have the option to display the text
> label only.

They do have that ability. I don't think this is something we should worry about though. The button should be fixed as a button with the word "Firefox" on it regardless of the state of the the other toolbar items. The only thing it probably should do is respect height of large vs. small icons.
(In reply to comment #72)
> (In reply to comment #70)
> > Any suggestion to allow other OSes to move the Firefox button around too?
> 
> Which other OSes are you referring to?

I meant primarily letting the various versions of Windows move the button.

> The only other primary supported OS is for OSX and Apple is very draconian
> about their HID which sort of requires the traditional menubar.

I'm talking about an option, not anything by default. (and I don't really care about Apple's rules too much, personally ;)
(In reply to comment #73)
> They do have that ability. I don't think this is something we should worry
> about though. The button should be fixed as a button with the word "Firefox" on
> it regardless of the state of the the other toolbar items.

+1... this is what the official beta feedback extension does btw... adds a
button which always shows the label "Feedback" (and a drop marker) and no icon
at all. Having the firefox button look and work like that would be great imho
(In reply to comment #74)
> (In reply to comment #72)
> > (In reply to comment #70)
> > > Any suggestion to allow other OSes to move the Firefox button around too?
> > 
> > Which other OSes are you referring to?
> 
> I meant primarily letting the various versions of Windows move the button.

so, now besides asking for adding support to customize toolbar icons on the tabbar, which has never been supported before, you also want to support this on the titlebar as well?
(In reply to comment #76)
> (In reply to comment #74)
> > (In reply to comment #72)
> > > (In reply to comment #70)
> > > > Any suggestion to allow other OSes to move the Firefox button around too?
> > > 
> > > Which other OSes are you referring to?
> > 
> > I meant primarily letting the various versions of Windows move the button.
> 
> so, now besides asking for adding support to customize toolbar icons on the
> tabbar, which has never been supported before, you also want to support this on
> the titlebar as well?

This seems off-topic for this bug :)
Attached image My idea of button with tabs-on-top (obsolete) —
I kind of had a different idea.  This is based on comments form other Linux users in various forums.  Just use the application icon as an icon with no text on the left of the tab toolbar.  This takes up zero vertical space and a minimal amount of horizontal space which is what I gathered was what the users seemed to be asking for.
Attachment #473235 - Attachment description: My idea of button with tabsw-on-top → My idea of button with tabs-on-top
(In reply to comment #77)
> (In reply to comment #76)
> > (In reply to comment #74)
> > > (In reply to comment #72)
> > > > (In reply to comment #70)
> > > > > Any suggestion to allow other OSes to move the Firefox button around too?

> This seems off-topic for this bug :)

Agreed; sorry for starting the tangent. ;)
Comment on attachment 473235 [details]
My idea of button with tabs-on-top

That results in two identical icons directly next to eachother, unfortunately.
(In reply to comment #80)
> That results in two identical icons directly next to eachother, unfortunately.

Not with any window manager :) but even the real firefox logo there wouldn't really scream "click me I am a menu button" to me.

Anyway, UX has decided - see comment #26 and comment #69 which has all the info that is needed.
(In reply to comment #81)
> (In reply to comment #80)
> > That results in two identical icons directly next to eachother, unfortunately.
> 
> Not with any window manager :) but even the real firefox logo there wouldn't
> really scream "click me I am a menu button" to me.
> 
> Anyway, UX has decided - see comment #26 and comment #69 which has all the info
> that is needed.

Well I  will try one more time.

I can either do make it a toolbar icon that can be customized and put on any toolbar where customization is currently possible
  
  OR

I can put it on the left side of the tabbar in whatever form you want it to take.

I cannot do both because I do not have the necessary skills or knowledge of how all of this works to add a new toolbar to the customization scheme.  However, it is also my opinion that even if I did, such a patch would be rejected anyway for being too late for the feature freeze.

SO although UX may have spoken, I think we need a new answer.  If none is forthcoming then I guess I can't help here.
(In reply to comment #81)
> (In reply to comment #80)
> > That results in two identical icons directly next to eachother, unfortunately.
> 
> Not with any window manager :) but even the real firefox logo there wouldn't
> really scream "click me I am a menu button" to me.

Well if it were really being done as a customizable toolbar icon, I woudl do this with a dropmarker ant the text albe woudl be appShortname so for test only would be a textbutton with the application name and a dropmarker and for icon only would at least have a dropmarker.

I was thinking after posting my screenshot that this really at least needs a dropmarpker.
(In reply to comment #82)
> (In reply to comment #81)
> > Anyway, UX has decided - see comment #26 and comment #69 which has all the info
> > that is needed.
> 
> Well I  will try one more time.
> 
> I can either do make it a toolbar icon that can be customized and put on any
> toolbar where customization is currently possible
> 
>   OR
> 
> I can put it on the left side of the tabbar in whatever form you want it to
> take.
> 
> I cannot do both because I do not have the necessary skills or knowledge of how
> all of this works to add a new toolbar to the customization scheme.  However,
> it is also my opinion that even if I did, such a patch would be rejected anyway
> for being too late for the feature freeze.
> 
> SO although UX may have spoken, I think we need a new answer.  If none is
> forthcoming then I guess I can't help here.

I vote door #2: left side of tabbar when there's no normal menu with no placement customization. Customization is nifty, but I care way less about that then just getting the button in. Just make things act vaguely similar to Windows XP and I'm happy.
(In reply to comment #81)
> Not with any window manager :) but even the real firefox logo there wouldn't
> really scream "click me I am a menu button" to me.

OK, would it seem any better to you with a tooltip?

I am realy not trying to be agumentative here.  I just really don't know how to implement what is asked for in comment 69.  However I don't see anyone else stepping up trying to help here at all.
(In reply to comment #73)
> (In reply to comment #71)
> > There are issues with this approach.  Most notably that it requires supporting
> > the placement of toolbar icons on the bookmarks toolbar which is not currently
> > supported and last I knew feature freeze was scheduled for Friday.
> 
> I don't understand what you mean by this. You can currently place toolbar icons
> on the bookmarks toolbar.

Oops.  I meant to say on  the tabsbar, which is currently NOT supported.
The tabbar has been a normal toolbar for a while and accepts other toolbar items :)
(In reply to comment #87)
> The tabbar has been a normal toolbar for a while and accepts other toolbar
> items :)

It does but only to be added to the right of the normally defined items.  There is absolutely no way to us the customize toolbars menu to add anything to the left of the tabs, which is, i presume what you really want here.
(In reply to comment #88)
> (In reply to comment #87)
> > The tabbar has been a normal toolbar for a while and accepts other toolbar
> > items :)
> 
> It does but only to be added to the right of the normally defined items.  There
> is absolutely no way to us the customize toolbars menu to add anything to the
> left of the tabs, which is, i presume what you really want here.

Ah yes, that would be a problem.
(In reply to comment #89)
> (In reply to comment #88)
> > (In reply to comment #87)
> > > The tabbar has been a normal toolbar for a while and accepts other toolbar
> > > items :)
> > 
> > It does but only to be added to the right of the normally defined items.  There
> > is absolutely no way to us the customize toolbars menu to add anything to the
> > left of the tabs, which is, i presume what you really want here.
> 
> Ah yes, that would be a problem.

But then I guess you are correct.  Maybe I really just need to look into this farther to see why things I try to add on the left end up on the right.  This might be more easily doable than I thought.
(In reply to comment #88)
> (In reply to comment #87)
> > The tabbar has been a normal toolbar for a while and accepts other toolbar
> > items :)
> 
> It does but only to be added to the right of the normally defined items.  There
> is absolutely no way to us the customize toolbars menu to add anything to the
> left of the tabs, which is, i presume what you really want here.

Just to be clear here.  I think you idea of how this should really work is really right on.  My issue was just with trying to work more with the realities of what can be done in the Firefox timeframe.  My initial feeling was it might need to be a we get it close for 4.0, maybe mavbe an extensions to fix it better and can do what you suggested for 4.1, but I think I need ot take another look at this.  I think it should be doable within the Firefox 4 timeframe, living within the feature/string freeze constraints.  just I am just not sure I am smart enough to figure out how.  But I am going to give it another look.

I have some promising ideas, now that I think on it some more.
(In reply to comment #88)
> (In reply to comment #87)
> > The tabbar has been a normal toolbar for a while and accepts other toolbar
> > items :)
> 
> It does but only to be added to the right of the normally defined items.  There
> is absolutely no way to us the customize toolbars menu to add anything to the
> left of the tabs, which is, i presume what you really want here.

I don't know where you're getting that idea. Just today, on Minefield under Linux, I moved the home button up to the left of the tab bar, specifically to the left of an app tab. It works fine.

Just for completeness-sake, I tried a new profile with latest Minefield and it lets me use the customize toolbar dialog to put any button I want on the left or right of the tab bar. Just note that by "tab bar" I mean the toolbar which contains the tabs and by "tabs" here I include the empty space where more new tabs are added (meaning all buttons on the right are aligned to the far right after the tab space). Simply put, you can customize this like anywhere else just fine.
(In reply to comment #92)
> (In reply to comment #88)
> > (In reply to comment #87)
> > > The tabbar has been a normal toolbar for a while and accepts other toolbar
> > > items :)
> > 
> > It does but only to be added to the right of the normally defined items.  There
> > is absolutely no way to us the customize toolbars menu to add anything to the
> > left of the tabs, which is, i presume what you really want here.
> 
> I don't know where you're getting that idea. Just today, on Minefield under
> Linux, I moved the home button up to the left of the tab bar, specifically to
> the left of an app tab. It works fine.
> 
> Just for completeness-sake, I tried a new profile with latest Minefield and it
> lets me use the customize toolbar dialog to put any button I want on the left
> or right of the tab bar. Just note that by "tab bar" I mean the toolbar which
> contains the tabs and by "tabs" here I include the empty space where more new
> tabs are added (meaning all buttons on the right are aligned to the far right
> after the tab space). Simply put, you can customize this like anywhere else
> just fine.

Well that is even better news. if you can already do this as long as an app tab is present, then I can just look at how the app tabs work.  I cannot get this to work without app tabs already present but will look into that.  Sound like I am either doing something wrong.  App tabs being present makes a difference or perhaps it is extension related.  Anyway this all gives me hope of being able to make this work as specified without violating any of the feature freeze rules.
(In reply to comment #93)
> Well that is even better news. if you can already do this as long as an app tab
> is present, then I can just look at how the app tabs work.

I did it the first time with an app tab just happening to be there. (mentioning it was an accidental red herring on my part) In the second part of comment 92 I said I just tested in a new profile, no app tabs involved.

It all works naturally. Maybe you're just not dragging it to just the right spot and it's finicky?
(In reply to comment #94)
> (In reply to comment #93)
> > Well that is even better news. if you can already do this as long as an app tab
> > is present, then I can just look at how the app tabs work.
> 
> I did it the first time with an app tab just happening to be there. (mentioning
> it was an accidental red herring on my part) In the second part of comment 92 I
> said I just tested in a new profile, no app tabs involved.
> 
> It all works naturally. Maybe you're just not dragging it to just the right
> spot and it's finicky?

OIC, this is lame, but it does work.  With just tabs there you can not place something to the left of the leftmost tab by tying to place it to the left of it, but trying to place it on top of it kind of ends up placing it to the left.  OK.  I guess I can go with that if that is how it works.  I will try to come up with a patch as time permits.
"Firefox" as the label makes a lot of sense when it replaces the window caption, less so when the window caption already says "Firefox" and the button is somewhere on the toolbars. "Menu" would probably make more sense there. We might want to honor the icon/text/icon+text mode state as well.
(In reply to comment #96)
> "Firefox" as the label makes a lot of sense when it replaces the window
> caption, less so when the window caption already says "Firefox" and the button
> is somewhere on the toolbars. "Menu" would probably make more sense there. We
> might want to honor the icon/text/icon+text mode state as well.

That makes sense to me.  Should we do a patch to just define the "Menu" label in the appropriate properties file so that that part of this makes the localization freeze deadline?
Attached patch Just the l10n piece of this (obsolete) — Splinter Review
This is just the part of this that would need to land before localization freeze if we want to go with Menu as the label for the toolbar button.
Assignee: nobody → bill
Status: NEW → ASSIGNED
Attachment #473794 - Flags: review?
Attachment #473794 - Flags: review? → review?(dao)
Attachment #473794 - Flags: approval2.0?
Comment on attachment 473794 [details] [diff] [review]
Just the l10n piece of this

All other toolbar buttons have a tooltip, this should probably have one as well. That might actually be of use for Windows as well.
(In reply to comment #99)
> Comment on attachment 473794 [details] [diff] [review]
> Just the l10n piece of this
> 
> All other toolbar buttons have a tooltip, this should probably have one as
> well. That might actually be of use for Windows as well.

OK.  What should the tooltip say?

"Display appShortname menu" ?
I take it your point is to decide on the tooltip also so we can get all the l10n strings in before freeze.  Sounds like a plan.
(In reply to comment #100)
> (In reply to comment #99)
> > Comment on attachment 473794 [details] [diff] [review] [details]
> > Just the l10n piece of this
> > 
> > All other toolbar buttons have a tooltip, this should probably have one as
> > well. That might actually be of use for Windows as well.
> 
> OK.  What should the tooltip say?
> 
> "Display appShortname menu" ?
> I take it your point is to decide on the tooltip also so we can get all the
> l10n strings in before freeze.  Sounds like a plan.

I know sooner hit submit, than I realized it is not a good idea to try to make this a multi-segment thing with appShortname in the middle as you then run into difficulty with languages that have a completely different syntax as to where in a phrase different parts of speech go.
<!ENTITY appMenuButton.tooltip "Open &brandShortName; menu"> should work.
Comment on attachment 473794 [details] [diff] [review]
Just the l10n piece of this

I've included this patch in my patch in bug 593126.
Attachment #473794 - Attachment is obsolete: true
Attachment #473794 - Flags: review?(dao)
Attachment #473794 - Flags: approval2.0?
Depends on: 593126
(In reply to comment #103)
> Comment on attachment 473794 [details] [diff] [review]
> Just the l10n piece of this
> 
> I've included this patch in my patch in bug 593126.

OK, but you patch does not include the tooltip I was going to add to the patch here.
Assignee: bill → nobody
Status: ASSIGNED → NEW
(In reply to comment #96)
> "Firefox" as the label makes a lot of sense when it replaces the window
> caption, less so when the window caption already says "Firefox" and the button
> is somewhere on the toolbars. "Menu" would probably make more sense there. We
> might want to honor the icon/text/icon+text mode state as well.

That doesn't make sense to me. It's called the Firefox menu for a reason. :)

This is straying into Opera territory, where when they got horrible results from just having their menu be their "O" icon, they just added the word "Menu" and put the entire existing menu tree under it.

If you think there's any chance of confusing a menu with a window title, we should do the opposite — remove "Firefox" from the window title, like we do on other platforms. (I assume the application switcher icon can still show the application name on Linux?)
I agree with everything Limi said.  I would also add that this is called "the firefox menu" if it doesn't have a name you can't tell someone to go to it, or really explain to anyone that it exists. (or less commonly, file a bug on it).
(In reply to comment #105)
> (In reply to comment #96)
> > "Firefox" as the label makes a lot of sense when it replaces the window
> > caption, less so when the window caption already says "Firefox" and the button
> > is somewhere on the toolbars. "Menu" would probably make more sense there. We
> > might want to honor the icon/text/icon+text mode state as well.
> 
> That doesn't make sense to me. It's called the Firefox menu for a reason. :)
> 
> This is straying into Opera territory, where when they got horrible results
> from just having their menu be their "O" icon, they just added the word "Menu"
> and put the entire existing menu tree under it.

I'm not sure what all this really means. Are you saying that if we labeled it "Menu", we'd have to follow Opera with regards to the menu structure?

> If you think there's any chance of confusing a menu with a window title, we
> should do the opposite — remove "Firefox" from the window title, like we do on
> other platforms. (I assume the application switcher icon can still show the
> application name on Linux?)

"other platforms" would be Mac -- we actually have "Firefox" in the window title on Windows, like every other browser except Safari.
(In reply to comment #106)
> I agree with everything Limi said.  I would also add that this is called "the
> firefox menu" if it doesn't have a name you can't tell someone to go to it, or
> really explain to anyone that it exists. (or less commonly, file a bug on it).

"The Firefox menu" seems to apply just fine here:

,-------------- Firefox ---------------.
| [Menu v]                             |
|                                      |
...
(In reply to comment #104)
> (In reply to comment #103)
> > Comment on attachment 473794 [details] [diff] [review]
> > Just the l10n piece of this
> > 
> > I've included this patch in my patch in bug 593126.
> 
> OK, but you patch does not include the tooltip I was going to add to the patch
> here.

In that case please include the tooltip on a patch on this bug.
(In reply to comment #107)
> I'm not sure what all this really means. Are you saying that if we labeled it
> "Menu", we'd have to follow Opera with regards to the menu structure?

No. It's like labeling a window with "Window". It doesn't make sense. :)

> "other platforms" would be Mac -- we actually have "Firefox" in the window
> title on Windows, like every other browser except Safari.

Only on XP at this point. Vista/7 doesn't have it. In any case, I'm not arguing for doing this, I'm just saying that if you're concerned about having "Firefox" in two locations, we should prefer getting rid of it from the title bar over naming the menu "Menu".
(In reply to comment #110)
> (In reply to comment #107)
> > I'm not sure what all this really means. Are you saying that if we labeled it
> > "Menu", we'd have to follow Opera with regards to the menu structure?
> 
> No. It's like labeling a window with "Window". It doesn't make sense. :)

Well, things need to have distinct labels. In an environment with multiple windows, "window" obviously doesn't cut it. There's no such ambiguity in a window such as the one in comment 108.

> > "other platforms" would be Mac -- we actually have "Firefox" in the window
> > title on Windows, like every other browser except Safari.
> 
> Only on XP at this point. Vista/7 doesn't have it. In any case, I'm not arguing
> for doing this, I'm just saying that if you're concerned about having "Firefox"
> in two locations, we should prefer getting rid of it from the title bar over
> naming the menu "Menu".

Vista/7 do have it, it just not normally visible when we render the Firefox button instead of the stock title bar.
I would also add that removing "Firefox" from the window title seems to be beyond this bug's scope.
(In reply to comment #111)
> Vista/7 do have it, it just not normally visible when we render the Firefox
> button instead of the stock title bar.
> I would also add that removing "Firefox" from the window title seems to be
> beyond this bug's scope.

So is renaming the menu, to be honest. Let's get the basic implementation working the same as on the other platforms as far as capability and general layout goes (not placement, obviously).
(In reply to comment #112)
> So is renaming the menu, to be honest. Let's get the basic implementation
> working the same as on the other platforms as far as capability and general
> layout goes (not placement, obviously).

I didn't say anything about capability or the menu layout. Beyond that please don't assert consistency where it just doesn't exist. "Other platforms" means Windows in this case and we're replacing the title bar with the Firefox button there. It's a different setup.
One of our goals with providing users with this option is to allow them to have a firefox interface that has cross platform consistency.  This is also important in terms of our documentation.
Ease of documentation probably shouldn't be a driving factor here, we should just do what makes most sense. "Firefox" being duplicated and scattered over the UI surely doesn't make a lot of sense.

Cross-platform consistency is moot right now. The interface will in fact be different, given the title bar and keyboard access constraints. Neither will the Linux/OS X/Windows transition be seamless nor will we able to document it as such. The optional button on the toolbar for Linux is essentially a stopgap rather than an achieved goal. Long-term I'd even expect us to drop it if/when Gnome gets a global and mandatory menu bar like OS X.
(In reply to comment #114)
> One of our goals with providing users with this option is to allow them to have
> a firefox interface that has cross platform consistency.  This is also
> important in terms of our documentation.

Yes and yes.

(In reply to comment #115)
> Ease of documentation probably shouldn't be a driving factor here

The word "documentation" here could be swapped out for some form of "users figuring out how to use the thing". That includes docs, help, help relying on docs, and users just winging it. If Firefox varies massively in base (w/o addons) capability depending on where you go then it no longer becomes a stable and consistent single product. I'd go as far as to say that you shouldn't be using the same name if you don't have the same main base capabilities (regardless of the defaults and minor platform specific differences).

(In reply to comment #115)
> Long-term I'd even expect us to drop it if/when
> Gnome gets a global and mandatory menu bar like OS X.

And us KDE users are what, screwed? (not Qt; GTK on KDE) Not to mention the other DEs out there.

Gnome !== Linux

> The interface will in fact be different

Different, but with similar capabilities (one would hope), mostly just different defaults and hotkeys. Same stuff with a different style. We can debate about the style but I want to keep the same stuff, and to be explicit, the Firefox button is stuff not style.
> the Firefox button is stuff not style.

Its usage, including being on by default, is style.
The Firefox button originated on Windows since we don't have a menu bar there.  However, I expect it to show up on other platforms where the user may not be familiar with the surrounding OS, or they might not even have access to the surrounding OS.  For instance, I could imagine that Firefox running in full screen on larger format android tablets or netbooks might use a Firefox button. (the same could potentially be true for some linux distributions on netbooks, especially if they have a custom launcher interface).  So in this regard, cross platform consistency is actually an objective.
Assignee: nobody → bill
Status: NEW → ASSIGNED
My real job schedule seems to have permitted me once again to find time to work on this again.  I will probably just leave it assigned to me form now on as when work interrupts, no one else seems to step up to working this anyway.

I almost have the movable toolbar item working and will probably be posting a patch for that soon.  The menubar hiding is probably simpler so that patch should follow soon after.

However, I just realized I never asked to have this reviewed and checked-in and this actually need to land before the localization freeze which is imminent.
Attachment #476569 - Flags: review?
Attachment #476569 - Flags: review? → review+
Attachment #476569 - Flags: approval2.0?
Stephen Horlander said (c#4) "The ability to natively draw widgets in the titelbar should come with gtk3"
Aren't you talking about client-side windows which has been implemented in GTK+ 2.18 ?
http://blogs.gnome.org/gtk/2009/07/10/gtk-2173-unstable-release/
(In reply to comment #120)
> Aren't you talking about client-side windows which has been implemented in GTK+
> 2.18 ?

No, we were talking about client side window *decorations* which are even not yet ready for GTK 3.0... :(
(In reply to comment #118)
> The Firefox button originated on Windows since we don't have a menu bar there. 
> However, I expect it to show up on other platforms where the user may not be
> familiar with the surrounding OS, or they might not even have access to the
> surrounding OS. [...] So in this regard, cross
> platform consistency is actually an objective.

I would also add that the coolness factor of having a Firefox button instead of the traditional menu should not be underestimated. One of the most common complaints in Input on Linux has been the lack of a Firefox button, which makes me believe that having the button enabled by default would be preferred by most early adopters.
(In reply to comment #122)
> (In reply to comment #118)
> > The Firefox button originated on Windows since we don't have a menu bar there. 
> > However, I expect it to show up on other platforms where the user may not be
> > familiar with the surrounding OS, or they might not even have access to the
> > surrounding OS. [...] So in this regard, cross
> > platform consistency is actually an objective.
> 
> I would also add that the coolness factor of having a Firefox button instead of
> the traditional menu should not be underestimated. One of the most common
> complaints in Input on Linux has been the lack of a Firefox button, which makes
> me believe that having the button enabled by default would be preferred by most
> early adopters.

New bug for that : Bug 598293
Attachment #476569 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
I had been holding off on posting the patches for this because I don't have check-in privileges and was trying to make sure that the wrong patch did not get checked-in with my checkin-needed request.  As it now seems there is a less than zero chance of getting the l10n part of this checked-in before the beta7 localization string freeze, I will be posting the patches i have to fix this later tonight.
Attachment #473235 - Attachment is obsolete: true
This patch does the following:

1.  It has no effect whatsoever on Windows builds.

2.  It implements the Application Menu button as a customizable toolbar button that can be placed on any customizable toolbar in any position chosen by the user.

3.  In order to permit the menu to be used either in the titlebar, or as a customizable toolbar item, I moved the appmenu-popup definition to an include file much as the menubar currently is.  This allows the same menu to be used in both places while changes only need to be made in one place in the source.

4.  Under Linux, by default, the Application Menu button is placed at the left of the tabbar.

5.  Under Mac/OSX the button, by default is not displayed.

6.  The toolbar button icon is currently just a placeholder.  I fully expect that the Linux theme group will change this to a more appropriate icon after this is checked in.  I am not an artistic or theme person, I am just trying to get this working as desired.

7.  For menu icons, on the new application button menu, I use the same icons as those used for the equivalent choices on the traditional menubar menu.

8.  For now, since I have no access to a MAC and no way to test what this looks like, since I had no facilities to do any pinstripe theme changes, I have temporarily placed an #ifndef XP_MACOSX in the code to disable the toolbar item under MAC/OSX.  This is noted with an appropriate XXX comment.
Attachment #477786 - Flags: review?
Attachment #477786 - Flags: review? → review?(dao)
(In reply to comment #125)
> Created attachment 477786 [details] [diff] [review]
> Implement applications menu as a toolbar button

Cool, could you attach a screenshot for those of us that don't have a Linux box to test this? :)
With this I ran into an issue.

Perhaps I am stupid, but I was unable to find any way to not display the menubar that did not end up leaving the hotkeys ALT-F, ALT-E, ALT-V, ALT-H, ALT-B, ALT-T and ALT-H active and still displaying the menu popups without the menubar re-showing.  This seemed worse than just re-activating the autohide for Linux and having those keys both show the pop-up and unhide the menubar.

When this code was previously disabled it was because there was no reasonable way to un-hide the menubar.  Now that we are activating the Application menu button as a toollbar item under Linux, the reasonable way to un-hide the menubar is to use Options -> Menu Bar from that button menu.
Attachment #477793 - Flags: review?
Attached image screenshot showing button on the tabbar (obsolete) —
Screenshot showing the menu button on the tabbar.  I tried to show this with the menu open but stupid Linux won;t allow me to do a screenshot with a popup menu displayed.  Go figure.
There are also 32-bit and 64-bit builds available including all 3 patches on this bug at http://www.wg9s.com/mozilla/firefox/
Attached image screenshot with menu open (obsolete) —
I used a camera
(In reply to comment #128)
> stupid Linux won;t allow me to do a screenshot with a popup
> menu displayed.  Go figure.

Just use whatever screenshot application comes with your desktop environment. On KDE 4.4 here I get to KSnapshot by just pressing the print screen key. Just set it to a couple second delay, click "new snapshot", then click the menu within those couple seconds and wait. Poof, screenshot with menu open. (though, no cursor) I'm sure GNOME and others have something similar.

(In reply to comment #131)
> I used a camera

Hacky, but effective. :p
Attached image screenshot with menu open (obsolete) —
Attachment #477804 - Attachment is obsolete: true
Attachment #477797 - Attachment description: screenshot showin toolbar in customize toolbar window → screenshot showing toolbar in customize toolbar window
Comment on attachment 477810 [details]
screenshot with menu open

from terminal window:

gnome-screenshot --delay=5
Attached image screenshot showing button tooltip (obsolete) —
Attachment #477793 - Flags: review? → review?(dao)
Comment on attachment 477794 [details]
screenshot showing button on the tabbar

If we want to go ahead and get this landed, that's totally fine.  However, eventually we need to address the following things with the UI:

-The button should be called "Firefox" in the customization pallet, so that it can be consistently referred to as the Firefox button, which produces the Firefox menu.
-The button should either be a Firefox icon, or the text "Firefox" depending on placement, it shouldn't be a down arrow
-The menu should not contain keyboard shortcuts on the top level, these are already exposed on each of the sub-menus
Attachment #477794 - Flags: ui-review-
(In reply to comment #136)
> -The button should either be a Firefox icon, or the text "Firefox" depending on
> placement, it shouldn't be a down arrow

Won't this look bad in combination with the simple icon style which I think is still meant to make it into 4.0? Maybe we can have simplified glyph for this as well (which would probably work better at the small size, too)

Anyway, this should land ASAP, the look can still be polished later.
(In reply to comment #137)
> Anyway, this should land ASAP, the look can still be polished later.

It will land when it's ready.
Comment on attachment 476569 [details] [diff] [review]
l10n patch required for tooltip on toolbar button (checked in)

http://hg.mozilla.org/mozilla-central/rev/9631d2f6c6a0
Attachment #476569 - Attachment description: l10n patch required for tooltip on toolbar button → l10n patch required for tooltip on toolbar button (checked in)
(In reply to comment #136)
> -The button should either be a Firefox icon, or the text "Firefox" depending on
> placement, it shouldn't be a down arrow

The arrow doesn't make sense, but we certainly don't want a Firefox icon here either. A Firefox icon is right above this in the title bar (unless your Gtk theme removes it).
Keywords: checkin-needed
Comment on attachment 477786 [details] [diff] [review]
Implement applications menu as a toolbar button

The way MENUBAR_CAN_AUTOHIDE is handled here doesn't seem to make sense. Seems like it should be defined on Linux.

>+++ b/browser/base/content/browser-appmenu.inc

>+# The Initial Developer of the Original Code is
>+# Bill Gianopoulos <bill@wg9s.com>.

You're really just moving it. ;-)

>+#ifdef MOZ_WIDGET_GTK2
>+             defaultset="appmenu-button,tabbrowser-tabs,new-tab-button,alltabs-button,tabview-button,tabs-closebutton"
>+#else
>              defaultset="tabbrowser-tabs,new-tab-button,alltabs-button,tabview-button,tabs-closebutton"
>+#endif
>              collapsed="true">
> 
>+#ifndef MENUBAR_CAN_AUTOHIDE
>+#ifndef XP_MACOSX
>+# XXX: bug 585370 -  temporarily disable under MAC/OSX until pinstripe theme is
>+#                    updated to theme the appmenu-button

Make this ifdef MOZ_WIDGET_GTK2, remove the XXX comment. We can't hide the menu bar on OS X.

>+      <toolbarbutton id="appmenu-button"
>+                     class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                     type="menu"
>+                     label="&appMenuButton.label;"
>+                     tooltiptext="&appMenuButton.tooltip;"
>+                     removable="true">
>+#include browser-appmenu.inc
>+      </toolbarbutton>
>+#endif
>+#endif

We need to make sure the button shows up as the user hides the menu bar (but not otherwise, which your patch doesn't seem to handle?). Making it removable seems like a footgun we better shouldn't provide.
Attachment #477786 - Flags: review?(dao) → review-
Attachment #477797 - Attachment is obsolete: true
Comment on attachment 477793 [details] [diff] [review]
Implement menubar autohide under Linux

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -810,17 +471,17 @@
>            tabsontop="true"
> #endif
>            persist="tabsontop">
>     <!-- Menu -->
>     <toolbar type="menubar" id="toolbar-menubar" class="chromeclass-menubar" customizable="true"
>              defaultset="menubar-items"
>              mode="icons" iconsize="small" defaulticonsize="small"
>              lockiconsize="true"
>-#ifdef MENUBAR_CAN_AUTOHIDE
>+#ifndef XP_MACOSX
>              toolbarname="&menubarCmd.label;"
>              accesskey="&menubarCmd.accesskey;"
> #endif
>              context="toolbar-context-menu">

Again, update MENUBAR_CAN_AUTOHIDE to reflect the new reality.
Attachment #477793 - Flags: review?(dao) → review-
Maybe we should use an icon like this one:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/places/bookmarksMenu.png

In fact, for now you could start referencing this very icon. We don't need a large version as the tab bar's icon size is always small.

It also seems like you shouldn't hide the dropmarker.
(In reply to comment #136)
> Comment on attachment 477794 [details]
> screenshot showing button on the tabbar
> 
> If we want to go ahead and get this landed, that's totally fine.  However,
> eventually we need to address the following things with the UI:
> 
> -The button should be called "Firefox" in the customization pallet, so that it
> can be consistently referred to as the Firefox button, which produces the
> Firefox menu.
> -The button should either be a Firefox icon, or the text "Firefox" depending on

Hmm in my earlier mock-ups I had posted in this patch I was using the firefox icon and I thought that idea had been shot-down.

> placement, it shouldn't be a down arrow

As I said, this was just a placeholder and I was not really trying to come-up with the final icon in this bug.  If we want to do that, however, I am ok doing that, if that is what is needed to get this landed.

> -The menu should not contain keyboard shortcuts on the top level, these are
> already exposed on each of the sub-menus

Hmm.  Oddly, I did not notice it was doing that.  I will look at how Windows is hiding those and copy the code.  It did not occur to me to check since I am using the same popup definition that windows does.
(In reply to comment #141)
> Comment on attachment 477786 [details] [diff] [review]
> Implement applications menu as a toolbar button
> 
> The way MENUBAR_CAN_AUTOHIDE is handled here doesn't seem to make sense. Seems
> like it should be defined on Linux.

I had noticed that as well.  The problem is that after all of this lands, assuming it does.  This ifdef is still required but is really unrelated to if the menubar can autohide or not.  I think the real issue is that part of what it controlled originally had to do with autohide, and part of what it did had to do with having showing or hiding the menubar controlling the visibility of the Firefox button.  Which now only works under windows.  I had planned on filing a followup bug on renaming this DEFINE, but could do it as part of this bug also.

It should probably be called MENU_BUTTON_BAR_TOGGLE or something to that effect.

> 
> >+++ b/browser/base/content/browser-appmenu.inc
> 
> >+# The Initial Developer of the Original Code is
> >+# Bill Gianopoulos <bill@wg9s.com>.
> 
> You're really just moving it. ;-)

That is true.  Who did the original app button code?  should probably put them here.

> 
> >+#ifdef MOZ_WIDGET_GTK2
> >+             defaultset="appmenu-button,tabbrowser-tabs,new-tab-button,alltabs-button,tabview-button,tabs-closebutton"
> >+#else
> >              defaultset="tabbrowser-tabs,new-tab-button,alltabs-button,tabview-button,tabs-closebutton"
> >+#endif
> >              collapsed="true">
> > 
> >+#ifndef MENUBAR_CAN_AUTOHIDE
> >+#ifndef XP_MACOSX
> >+# XXX: bug 585370 -  temporarily disable under MAC/OSX until pinstripe theme is
> >+#                    updated to theme the appmenu-button
> 
> Make this ifdef MOZ_WIDGET_GTK2, remove the XXX comment. We can't hide the menu
> bar on OS X.

The current ifndef would make more sense if i renamed the MENUBAR_CAN_AUTOHIDE DEFINE to describe its current function.  THe reason I had this doing OS X as well was that about a zillion comments back, where the goal here was defined, the idea promoted was to make this a customizable toolbar button on al OS's except Windows.  I could just use MOZ_WIDGET_GTK2.  I was just trying to help out the OS X guys buy having this in place for them to enable once they did the theme piece.  I can make this Linux only as this is a Linux only bug.
> 
> >+      <toolbarbutton id="appmenu-button"
> >+                     class="toolbarbutton-1 chromeclass-toolbar-additional"
> >+                     type="menu"
> >+                     label="&appMenuButton.label;"
> >+                     tooltiptext="&appMenuButton.tooltip;"
> >+                     removable="true">
> >+#include browser-appmenu.inc
> >+      </toolbarbutton>
> >+#endif
> >+#endif
> 
> We need to make sure the button shows up as the user hides the menu bar (but
> not otherwise, which your patch doesn't seem to handle?). Making it removable
> seems like a footgun we better shouldn't provide.

If I do make it work that way, then all my comments about MENUBAR_CAN_AUTOHIDE above do not apply.  I just re-read the desired behavior specified in comment 69 and it does say the toolbar item should show if the menubar is hidden.  I guess that means the visibility of this button should depend on the menubar just as the Windows button does.  SO I will try to get that working.
(In reply to comment #143)
> Maybe we should use an icon like this one:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/places/bookmarksMenu.png
> 
> In fact, for now you could start referencing this very icon. We don't need a
> large version as the tab bar's icon size is always small.
> 
> It also seems like you shouldn't hide the dropmarker.

I thought the placement on the tabbar was supposed to be the default location, but the user was supposed to be able to place it on whatever toolbar they preferred.  If that is the case, then both sizes are required.  If we just want to have this non-customizable and always on tabbar left and visible when the menubar is hidden, I can do that as well.

The problem I had with the dropmarker was also related to making this customizable.  Using the style for a customizable button seemed to lock the width so that the dropmarker displayed below the icon.  That is why I removed it, and also why I changed the placemarker icon I was using to the down arrow on the idea that it at least looked  a bit like a dropmarker.

I guess I need to try to figure out how to get the dropmarker on the right.
> > >+# The Initial Developer of the Original Code is
> > >+# Bill Gianopoulos <bill@wg9s.com>.
> > 
> > You're really just moving it. ;-)
> 
> That is true.  Who did the original app button code?  should probably put them
> here.

"the Mozilla Organization" did...
Please add me (bug 570795) and Joshua M (bug 585370) as contributors.

> If I do make it work that way, then all my comments about MENUBAR_CAN_AUTOHIDE
> above do not apply.  I just re-read the desired behavior specified in comment
> 69 and it does say the toolbar item should show if the menubar is hidden.  I
> guess that means the visibility of this button should depend on the menubar
> just as the Windows button does.

Exactly.

(In reply to comment #146)
> I thought the placement on the tabbar was supposed to be the default location,
> but the user was supposed to be able to place it on whatever toolbar they
> preferred.  If that is the case, then both sizes are required.  If we just want
> to have this non-customizable and always on tabbar left and visible when the
> menubar is hidden, I can do that as well.

See the last part of comment 141. Also, the user can't move it on Windows, so locking it here seems fair.
> "the Mozilla Organization" did...

Err, it's the Mozilla Foundation.
(In reply to comment #146)
 
> I guess I need to try to figure out how to get the dropmarker on the right.

I don't know if it can help but Nautilus has such dropmarker in its sidepanel
see here "Orte v" http://www.cedynamix.fr/wp-content/uploads/2008/07/nautilus-tabs.png
(In reply to comment #147)
> See the last part of comment 141. Also, the user can't move it on Windows, so
> locking it here seems fair.

So much moving back and forth here... on windows, the button does not take away
tab space, so not being movable is not a big deal. Being part of the toolbar on
linux, it would be very bad to not be able to customize it. And customizing
includes removing in this case.

(In reply to comment #146)
> I guess I need to try to figure out how to get the dropmarker on the right.

You can probably copy it from the bookmarks menu button?
(In reply to comment #148)
> > "the Mozilla Organization" did...
> 
> Err, it's the Mozilla Foundation.

(In reply to comment #148)
> > "the Mozilla Organization" did...
> 
> Err, it's the Mozilla Foundation.

Oops typo.  I will fix that.

I should be able to come up with new patches addressing all the review comments over the weekend.
(In reply to comment #136)
> Comment on attachment 477794 [details]
> screenshot showing button on the tabbar

> -The menu should not contain keyboard shortcuts on the top level, these are
> already exposed on each of the sub-menus

I ran into an unexpected issue in trying to fix this.  I was originally under the impression that the issue here was that somehow there was something in winstripe that hid these, or that whatever else hid these did not work on a toolbar button.

The current code that controls this under Windows appears to be that checked in for bug 589139.

Anyway, in trying to prove this was not a it does not work under Linux issue, buy just doing a vanilla build including may previously posted patch from attachment 465030 [details] [diff] [review], which basically makes the linux build work as closely as possible to the windows version resulted in the menu still persisting in displaying the keyboard shortcuts.  So, it would seem that the fix in bug 589139 is for some reason windows only.

Anyway, I have CC'd margaret to see if we can figure out what the issue here is.
Attached image screenshot using bookmarkmenu.png icon (obsolete) —
(In reply to comment #143)
> Maybe we should use an icon like this one:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/places/bookmarksMenu.png
> 
> In fact, for now you could start referencing this very icon. We don't need a
> large version as the tab bar's icon size is always small.

As you can see form attachment 478566 [details], it would seem that we need an icon with a higher saturation level than that one for something we are trying to showcase as new feature.  It looks way too washed out and almost looks disabled.
Button styling of some kind as apposed to just the icon might help.

As you've said above, all the icons should be getting updated by theme experts so they can come up with something better later. It's decent enough looking for now, though, I think.
So from Stephan Horlander comment #69 i have:

If the menubar is hidden manually the Firefox button appears next to the tabs
by default but can be moved anywhere.

However, Dão Gottwald seems to be favoring the the it is at at the left to the tabs bar solution, which is clearly easier to implement, and might even be all we can implement in time for Firefox4.

My problem with that is that if you don't have tabs on top selected, does it not make sense to be able to place this on the navigation bar?

In any event, I really do not have time to develop 2 solutions here.  I really need the UX team to come up with one clear direction that I will try to implement here.
(In reply to comment #155)
> Button styling of some kind as apposed to just the icon might help.
> 
> As you've said above, all the icons should be getting updated by theme experts
> so they can come up with something better later. It's decent enough looking for
> now, though, I think.

It looks awful enough so that I refuse to use it for my personal builds.
My point here is not to criticize the original suggestion of using this icon.  It looked fine to me until I tried to actually use it in context.

It is just that it really looks so bad in the context for which it is intended, that I don't want my name associated with any patch to use it in that context.
(In reply to comment #156)
> However, Dão Gottwald seems to be favoring the the it is at at the left to the
> tabs bar solution, which is clearly easier to implement, and might even be all
> we can implement in time for Firefox4.
> 
> My problem with that is that if you don't have tabs on top selected, does it
> not make sense to be able to place this on the navigation bar?

Opting to select both old style tabs on bottom and also opting to add the new button doesn't seem like it would be a common combination. (Linux will have tabs on top as default whenever bug 593823 lands) I don't even think there'd be a problem with it in the tabs bar on the bottom if it's a small button. If it really is, you could just have it require tabs on top.

(In reply to comment #158)
> My point here is not to criticize the original suggestion of using this icon. 
> It looked fine to me until I tried to actually use it in context.
> 
> It is just that it really looks so bad in the context for which it is intended,
> that I don't want my name associated with any patch to use it in that context.

If you dislike the icon that much, just use the same orange Firefox button style from Windows until it can be updated to a good looking little icon in a followup bug.
Attachment #476569 - Attachment is obsolete: true
Attachment #477786 - Attachment is obsolete: true
Attachment #477793 - Attachment is obsolete: true
Comment on attachment 476569 [details] [diff] [review]
l10n patch required for tooltip on toolbar button (checked in)

This attachment isn't obsolete if it wasn't backed out.
Attachment #476569 - Attachment is obsolete: false
(In reply to comment #147)
> Please add me (bug 570795) and Joshua M (bug 585370) as contributors.
                                 ^^^^^^^^^^^^^^^^^^^^^

I have been unable to ascertain a full name or email address for Joshua M in order to properly cite him as a contributor.  Also the bug number referenced here appears to be incorrect as it it is this bug.
(In reply to comment #161)
> Comment on attachment 476569 [details] [diff] [review]
> l10n patch required for tooltip on toolbar button (checked in)
> 
> This attachment isn't obsolete if it wasn't backed out.

Guess I got carried away obsoleting old patches while posting the current WIP patch.
(In reply to comment #164)
> Created attachment 478588 [details]
> Screenshot showing icon used in WIP patch

OK.  There are issues with this patch as well.

1.  It is a stock gnome image.  If we decide to use this or an image based by this we need to make sure the licensing of the stock gnome images permits that.

2.  I intended to remove the arrow and try to make this look more like a restaurant menu.

3.  Although I originally intended this to look like a restaurant menu, I now realize it also looks like a book, and perhaps it might be confused with an icon for some bookmarks function.
(In reply to comment #159)
> (In reply to comment #156)
> > However, Dão Gottwald seems to be favoring the the it is at at the left to the
> > tabs bar solution, which is clearly easier to implement, and might even be all
> > we can implement in time for Firefox4.
> > 
> > My problem with that is that if you don't have tabs on top selected, does it
> > not make sense to be able to place this on the navigation bar?
> 
> Opting to select both old style tabs on bottom and also opting to add the new
> button doesn't seem like it would be a common combination. (Linux will have
> tabs on top as default whenever bug 593823 lands) I don't even think there'd be
> a problem with it in the tabs bar on the bottom if it's a small button. If it
> really is, you could just have it require tabs on top.
> 
> (In reply to comment #158)
> > My point here is not to criticize the original suggestion of using this icon. 
> > It looked fine to me until I tried to actually use it in context.
> > 
> > It is just that it really looks so bad in the context for which it is intended,
> > that I don't want my name associated with any patch to use it in that context.
> 
> If you dislike the icon that much, just use the same orange Firefox button
> style from Windows until it can be updated to a good looking little icon in a
> followup bug.

I was just trying to get all the things I wanted to post on this bug as single thought per comment.  Then After you see the choices between using the bookmarks menu icon and the one i have used a placekeeper a comment would make more sense.

As I understand it, Stephen Horlander is supposed to be the person I am getting my direction from and he wants a customizable movable toolbar icon which requires both a 16x16 and a 24x24 version which the bookmarks menu icon does not have.
(In reply to comment #159)
I am not trying to create an issue here.  It is just that my direction is to implement what was specified in comment 69 in this bug.  If that is not what is desired, I would like the UX team to get together and post a new specification so I have some idea what the desired end result is.
>I would like the UX team to get together and post a new specification
>so I have some idea what the desired end result is.

In general I would like us to get this landed and we can change things like the icon / string used for the menu later.  So you can feel free to land with the current interface as soon as you have code review.

We'll discuss this in the UX meeting on monday and should have a complete spec for you then.
(In reply to comment #168)
> >I would like the UX team to get together and post a new specification
> >so I have some idea what the desired end result is.
> 
> In general I would like us to get this landed and we can change things like the
> icon / string used for the menu later.  So you can feel free to land with the
> current interface as soon as you have code review.
> 
> We'll discuss this in the UX meeting on monday and should have a complete spec
> for you then.

Thanks for that.

My issue here is that I am way outside my comfort zone for dong this type of change.  I am a C programmer and no very little about XUL or XML bindings.  I only stepped up to do this because no one else was and I felt it was really important that Firefox $ not be released in a way that made it appear that Linux users were not being considered to be important.

So, normally if this were not so far outside my comfort level, I could probably figure out what to do about the criticisms of my approach.. My problem here is that I am really just learning what is possible to do within XUL/CSS/XML related to what is going on here, so cannot speak with any authority in trying to say why my approach might be any better that what anyone else is suggesting.
(In reply to comment #152)
> Anyway, in trying to prove this was not a it does not work under Linux issue,
> buy just doing a vanilla build including may previously posted patch from
> attachment 465030 [details] [diff] [review], which basically makes the linux build work as closely as
> possible to the windows version resulted in the menu still persisting in
> displaying the keyboard shortcuts.  So, it would seem that the fix in bug
> 589139 is for some reason windows only.
> 
> Anyway, I have CC'd margaret to see if we can figure out what the issue here
> is.

Are you still having this problem? I took at look at the patch I wrote in bug 589139, but I'm having trouble spotting what could cause it to break on Linux.
I am not a developer, but a user. Not having the UI makes FF4 clunky. With FF 3.6.* I am using 'Personal Menu' addon and it gives a bit of that, it also supports ALT hide-unhide.

With the screenshots of menu with the tab bar is quite a go! As a KDE user I don't feel I want to use a different window manager just for FF.
Bill, please don't worry about hiding the shortcut keys. We can take care of that in a follow-up bug.

Also, making the button (re)movable opens a can of worms which you'll probably want to avoid.
Still to do:

1.  Modify updateAppButtonDisplay do control the hiding/unhiding of the toolbar button.
2.  Fix the hiding of keyboard shortcuts in the Applications button menu.
Attachment #478581 - Attachment is obsolete: true
(In reply to comment #172)
> Bill, please don't worry about hiding the shortcut keys. We can take care of
> that in a follow-up bug.

OK I left this on my to-do list because I wanted to make one pore passover the winstripe theme files to see if there is code required for this that I need to copy to gnomestripe that I missed the frst time I looked.

> 
> Also, making the button (re)movable opens a can of worms which you'll probably
> want to avoid.

Yes, I have already run into this.  It is not the movable that has caused issues.  It is the possibility of removing it that necessitated replacing some "ifdef MENUBAR_CAN_AUTOHIDE" with the more klunky try/catch.

Since I currently have it re(movable) I will leave it that way until I get everything else working, and make in non (re)movable for the review ready patch.
Cannot get it build with last patch : 

nsNetscapeProfileMigratorBase.cpp
/home/fred/logs/fox/src/browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp: In member function ‘nsresult nsNetscapeProfileMigratorBase::ImportNetscapeCookies(nsIFile*)’:
/home/fred/logs/fox/src/browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp:391:32: attention : suggest parentheses around ‘&&’ within ‘||’
nsSeamonkeyProfileMigrator.cpp
nsPhoenixProfileMigrator.cpp
nsDogbertProfileMigrator.cpp
nsOperaProfileMigrator.cpp
nsModule.cpp
make[3]: *** [libs_tier_app] Erreur 2
make[2]: *** [tier_app] Erreur 2
make[1]: *** [default] Erreur 2
make: *** [build] Erreur 2
This implements hiding the toolbar button when the menubar is hidden and also uses a better toolbar icon.

ToDo:

Make the toolbar button non-(re)movable.
Attachment #479014 - Attachment is obsolete: true
Attached image screenshot showing new icon (obsolete) —
This is the new icon used by the latest WIP patch.  It is basically just an edit of the bookmarks.png icon with moire saturation and some of the official firefox orange color added.  My intent here was to create something good enough for a beta release.
Attachment #477794 - Attachment is obsolete: true
Attachment #478566 - Attachment is obsolete: true
Attachment #478588 - Attachment is obsolete: true
Target Milestone: --- → Firefox 4.0
Why not just use the Firefox icon for the Firefox menu?

It was suggested above that the Firefox icon will already be present in the title bar, but that's not true for anyone using either Ubuntu's default theme or Gnome 3's default theme. I suspect this will cover the majority of users.

If we're being consistent about branding this as the "Firefox menu" on all OSes, using any other icon would be confusingly off-brand.
Addresses all previous review comments except for the shortcuts displaying in the menu, which we agreed to handle in a followup bug.
Attachment #479875 - Flags: review?(dao)
Flags: in-litmus?
Only difference here is to dim the applications menu button when in customize toolbars mode to indicate that it cannot be (re)moved.
Attachment #479875 - Attachment is obsolete: true
Attachment #479897 - Flags: review?(dao)
Attachment #479875 - Flags: review?(dao)
Sorry to spam, but I cannot get it build with last version of the patch. I am under Archlinux (64 bits), with gcc 4.5.1, and make 3.8.2.

AboutRedirector.cpp
Traceback (most recent call last):
  File "/home/fred/logs/fox/src/config/JarMaker.py", line 506, in <module>
    main()
  File "/home/fred/logs/fox/src/config/JarMaker.py", line 503, in main
    localedirs=options.l10n_src)
  File "/home/fred/logs/fox/src/config/JarMaker.py", line 294, in makeJars
    jardir=jardir)
  File "/home/fred/logs/fox/src/config/JarMaker.py", line 239, in makeJar
    localedirs)
  File "/home/fred/logs/fox/src/config/JarMaker.py", line 360, in processJarSection
    outHelper, jf)
  File "/home/fred/logs/fox/src/config/JarMaker.py", line 392, in _processEntryLine
    raise RuntimeError('File "%s" not found in %s' % (src, ', '.join(src_base)))
RuntimeError: File "appmenu.png" not found in /home/fred/logs/fox/src/browser/themes/gnomestripe/browser, .
make[7]: *** [libs] Erreur 1
make[6]: *** [libs] Erreur 2
make[5]: *** [libs] Erreur 2
make[4]: *** [themes_libs] Erreur 2
make[4]: *** Attente des tâches non terminées....
DirectoryProvider.cpp
nsIFeedResultService.idl
nsIWebContentConverterRegistrar.idl
nsIFeedWriter.idl
nsFeedSniffer.cpp
nsPrivateBrowsingServiceWrapper.cpp
nsISessionStartup.idl
nsISessionStore.idl
nsIShellService.idl
nsGNOMEShellService.cpp
nsIBrowserProfileMigrator.idl
nsProfileMigrator.cpp
nsIBrowserGlue.idl
nsIBrowserHandler.idl
nsBrowserProfileMigratorUtils.cpp
nsNetscapeProfileMigratorBase.cpp
nsSeamonkeyProfileMigrator.cpp
nsPhoenixProfileMigrator.cpp
/home/fred/logs/fox/src/browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp: In member function ‘nsresult nsNetscapeProfileMigratorBase::ImportNetscapeCookies(nsIFile*)’:
/home/fred/logs/fox/src/browser/components/migration/src/nsNetscapeProfileMigratorBase.cpp:391:32: attention : suggest parentheses around ‘&&’ within ‘||’
nsDogbertProfileMigrator.cpp
nsOperaProfileMigrator.cpp
nsModule.cpp
make[3]: *** [libs_tier_app] Erreur 2
make[2]: *** [tier_app] Erreur 2
make[1]: *** [default] Erreur 2
make: *** [build] Erreur 2

Any idea ?

gcc -v
Utilisation des specs internes.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-unknown-linux-gnu/4.5.1/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configuré avec: ../configure --prefix=/usr --enable-languages=c,c++,fortran,objc,obj-c++,ada --enable-shared --enable-threads=posix --enable-__cxa_atexit --enable-clocale=gnu --enable-gnu-unique-object --enable-lto --enable-plugin --disable-multilib --disable-libstdcxx-pch --with-system-zlib --with-ppl --with-cloog --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info
Modèle de thread: posix
gcc version 4.5.1 (GCC)
Attached image appmenu.png (obsolete) —
(In reply to comment #181)
> Sorry to spam, but I cannot get it build with last version of the patch. I am
> under Archlinux (64 bits), with gcc 4.5.1, and make 3.8.2.

> RuntimeError: File "appmenu.png" not found in

Not sure if it is an issue with the patch or if you are applying it using a utility that does not support binary diffs.

In any event I just added an attachment containing the appmenu.png file.  If you add this file to browser/themes/gnomestripe/browser, after applying the rest of the patch, that should fix your issue.
I'm using good old patch :)

patch --version
patch 2.6.1

I have no time to verify this option before at least 6 hours from now. I will tell you.
(In reply to comment #184)
> I'm using good old patch :)
> 
> patch --version
> patch 2.6.1
> 
> I have no time to verify this option before at least 6 hours from now. I will
> tell you.

If you use do an "hg import --no-commit" instead of using patch, the patch as attached here should work correctly and create the appmenu.png file in the correct location.  patch does not support binary diffs.
Thanks for all the infos. I will wait until final patch is available, and I agree too with comments which talk about using firefox icon instead of a new one.
(In reply to comment #186)
> Thanks for all the infos. I will wait until final patch is available, and I
> agree too with comments which talk about using firefox icon instead of a new
> one.

The intention of this bug is to get the mechanics working correctly.  The final ICON used in the release version will be determined outside of the scope of this bug.
You can obviously add the following to userChrome.css to make it use the Application icon:

#appmenu-button {
  list-style-image: url("chrome://branding/content/icon16.png") !important;
}
I was looking at the proposed patches for bug 572160 and realized there is an issue with the patch for this bug in the way I have implemented it.  The issue is in misuse of MENUBAR_CAN_AUTOHIDE.

There were some feature driven ifdefs (#ifdef MENUBAR_CAN_AUTO_HIDE) that I have changed to be Platform dependent ifdefs.  In some cases this is definitely wrong.

What I should have done was defined another feature dependent (CAN_DRAW_IN_TITLEBAR) thing to use for the ifdef and use that when the code is definitely dependent on drawing in the titlebar.

In this way whatever fix eventually is decided on for bug 572160 it can depend on CAN_DRAW_IN_TITLEBAR instead of MENUBAR_CAN_AUTOHIDE or XP_WIN.

SO, I was going to rewrite my patch here to implement this.

However, I now have the question that assuming this gets landed for Firefox 4 and then a few months or a year later we get the ability to draw in the titlebar under Linux, would we:

1.  Convert Linux to use the text based not on tabbar firefox button as is now down in windows, or

2.  Leave it with the toolbar button and on the tabbar and just push the tabbar up into the titlebar?

This makes a difference as too how much of this patch I convert to not use the platform specific ifdefs.

So, I would like a UX team opinion as to which of the 2 above paths would be more likely to be followed.
I decided to just do 2 patches, one for each of the approaches in comment 189.  You can just r- the one you rule out saying you prefer the other approach.
Attached patch comment 189 approach 1 (obsolete) — Splinter Review
By way of explanation, the reason I added the define of APPMENU_ON_TABBAR in browser.xul is that in .xul files we don't seem to support:

  #if defined(MENUBAR_CAN_AUTOHIDE) && !defined(CAN_DRAW_IN_TITLEBAR)

which is what I wanted to do.

Doing it with seperate ifdefs was just too convoluted to follow.
Attachment #479897 - Attachment is obsolete: true
Attachment #480422 - Flags: review?(dao)
Attachment #479897 - Flags: review?(dao)
Attached patch comment 189 approach 2 (obsolete) — Splinter Review
Attachment #480424 - Flags: review?(dao)
It should be noted that either approach works equally well for this bug.  The reason for the 2 patches is that if we know what the plan is for after we can draw int he titlebar and pick the appropriate patch it should save some work when it comes time to implement drawing in the titlebar.
I should also point out that my personal opinion is that Approach 1 is the correct one.  I think that once we have the capability of drawing in the titlebar under Linux, continuing to perpetuate this divergence in what is the new look of Firefox is the correct approach form a branding perspective.

That is why the builds on my server at http://www.wg9s.com/mozilla/firefox/ include the approach 1 patch.

My only reason for including the alternative patch, was in case I was wrong and using a toolbar button for Linux was more of a Linux native look and feel decision rather than we really can't implement the windows design under Linux yet.
Many thanks Bill Gianopoulos for the patches builds : it makes testing easier.
I've tested the linux 64 bits one on Ubuntu 10.10 RC (GNOME) and i'd like to point out a strange behaviour :
The arrow close to the menu entry that is used to bring the submenu is not linked to the entry itself (see screenshot attached) therefore hovering the menu entry itself with the mouse doesn't bring the submenu unless the arrow itself is hovered.

Another thing that i've noticed is that the Edit menu entry has icons close to it (scissors, clipboard...) wheareas GNOME has a setting to not show these menu icons by default (since GNOME 2.28 - see https://bugzilla.gnome.org/show_bug.cgi?id=557469)
About the "arrow not linked to the corresponding entry" thing, i'd like to add that the Web Developer entry works nice unlike other entries
(In reply to comment #195)
> Many thanks Bill Gianopoulos for the patches builds : it makes testing easier.
> I've tested the linux 64 bits one on Ubuntu 10.10 RC (GNOME) and i'd like to
> point out a strange behaviour :
> The arrow close to the menu entry that is used to bring the submenu is not
> linked to the entry itself (see screenshot attached) therefore hovering the
> menu entry itself with the mouse doesn't bring the submenu unless the arrow
> itself is hovered.

This all works just a squirrelly under Windows.  The intention here was just to get a way to access the menu and not fix issues that already exist in how the menu works once it is opened.

> 
> Another thing that i've noticed is that the Edit menu entry has icons close to
> it (scissors, clipboard...) wheareas GNOME has a setting to not show these menu
> icons by default (since GNOME 2.28 - see
> https://bugzilla.gnome.org/show_bug.cgi?id=557469)

Well the way the Edit menu item works is that its function is entirely dependent on the icons.  It has no textbased submenu, so I guess you are asking that the entire Edit menu choice not display if the user has selected not to display menu icons?  That might make sense, but I think it is another thing that is outside of the scope of this bug and would be a followup bug after this lands (assuming it actually does land).
(In reply to comment #198)
> Well the way the Edit menu item works is that its function is entirely
> dependent on the icons.

From a GNOME POV I don't see the problem. This is no traditional UI, which only has icons in addition to labels but more like actual buttons. Chrome does the same thing, so this may very well end up as "what users expect".
>>therefore hovering the
>> menu entry itself with the mouse doesn't bring the submenu unless the arrow
>> itself is hovered.
>
>This all works just a squirrelly under Windows.

we'll be fixing that with bug 589146 for both platforms
in reply to comment #198 (1°) & 200 : i'm not sure to understand : that bug (having the arrow independant from the label) is known and is not Linux specific ? Even considering that the Web Developer entry already works nice ?

in reply to comment #198 (2°) & 199 : i didn't know that Chromium acted that way. Looks weird to me (why having sometimes submenu and sometimes buttons ?! there is no consistency) but this is not the place to discuss that. 
On the contrary i think that buttons should be label-only on GNOME when menus_have_icons is set to false* (like Chromium does) whereas buttons could be icon-only on the contrary

2 more things (see  former screenshot for both of them) :
When Menu bar is unticked in the View menu, the new button is shown but the the menu bar remains
The menu that is displayed from the new button has 2 pane  (but i think it's a feature of Firefox 4 ?)

Thanks

* which is default since GNOME 2.28
Comment on attachment 480424 [details] [diff] [review]
comment 189 approach 2

This patch has bit-rottted, and I decided that I don't really have the time to keep both versions up-to-date with trunk changes.  Since I think the other approach is better, I am withdrawing this patch form review consideration.
Attachment #480424 - Attachment is obsolete: true
Attachment #480424 - Flags: review?(dao)
Update patch to current trunk.
Attachment #480422 - Attachment is obsolete: true
Attachment #481736 - Flags: review?(dao)
Attachment #480422 - Flags: review?(dao)
(In reply to comment #170)
> Are you still having this problem? I took at look at the patch I wrote in bug
> 589139, but I'm having trouble spotting what could cause it to break on Linux.

Yes, I am still having this issue, but I have done a lot more testing and I think that somehow the native Linux themeing is interfering with the selectors in browser/base/content/browser.css properly matching.

In particualr this code:

.split-menuitem-menu > .menu-accel-container {
  display: none;
}

in browser.css does not seem to be applied even though dom inspector would lead me to believe it should be.

Everything works fine if I use any third party theme.  This only fails to work if i use the defualt theme so I thkink it is some kind of issue with the native themeing code that is overriding the rules I am using in css.
Target Milestone: Firefox 4.0 → Firefox 4.0b8
(In reply to comment #204)
> Everything works fine if I use any third party theme.  This only fails to work
> if i use the defualt theme so I thkink it is some kind of issue with the native
> themeing code that is overriding the rules I am using in css.

I should have pointed out before you ask.  None of these third party themes that I have tested with have been updated to Firefox 4. They are all Firefox 3 third party themes, so I don;t think the issue is that gnomestirpe needs some kind of appmenu support update that I missed somewhere.
Comment on attachment 481736 [details] [diff] [review]
patch updated to tip - 2010/10/07

>+#ifdef APPMENU_ON_TABBAR
>+      <toolbaritem id="appmenu-toolbar-button-container"
>+                   class="chromeclass-toolbar-additional"
>+                   title="&brandShortName;">
>+        <toolbarbutton id="appmenu-button"
>+                       class="toolbarbutton"
>+                       type="menu"
>+                       label="&brandShortName;"

Make this &appMenuButton.label;

>+                       tooltiptext="&appMenuButton.tooltip;">
>+#include browser-appmenu.inc
>+        </toolbarbutton>
>+      </toolbaritem>

Why is the structure <toolbaritem><toolbarbutton/></toolbaritem> instead of just <toolbarbutton/>?

>+#appmenu-button.toolbarbutton {

This selector looks broken.

>+#appmenuSecondaryPane {
>+  border-left: 1px solid;

Use -moz-border-start instead for RTL support.
Attachment #481736 - Flags: review?(dao) → review-
(In reply to comment #206)
> Comment on attachment 481736 [details] [diff] [review]
> patch updated to tip - 2010/10/07
> 
> >+#ifdef APPMENU_ON_TABBAR
> >+      <toolbaritem id="appmenu-toolbar-button-container"
> >+                   class="chromeclass-toolbar-additional"
> >+                   title="&brandShortName;">
> >+        <toolbarbutton id="appmenu-button"
> >+                       class="toolbarbutton"
> >+                       type="menu"
> >+                       label="&brandShortName;"
> 
> Make this &appMenuButton.label;

Done!

> 
> >+                       tooltiptext="&appMenuButton.tooltip;">
> >+#include browser-appmenu.inc
> >+        </toolbarbutton>
> >+      </toolbaritem>
> 
> Why is the structure <toolbaritem><toolbarbutton/></toolbaritem> instead of
> just <toolbarbutton/>?

I did this when the toolbar button was (re)movable.  Preliminary testing indicates it is no longer required, so I removed it.
 
> >+#appmenu-button.toolbarbutton {
> 
> This selector looks broken.

I do not understand the issue here.  This is a perfectly valid CSS selector that matches an element with an identifier of appmenu-button, if and only if it also has the class toolbarbutton associated with it.  My intention here was to match the Linux appmenu button by using a rule which would not match the Windows appmenu button.
This way third party themers that copied the code form here could do something like this in their theme:

#appmenu-button-container > #appmenu-button {
  styling for windows button goes here
}
#appmenu-button.toolbarbutton {
  styling for Linux button goes here
}

In this way they could provide one theme to work with both Linux and Windows that could sperratley style this element for the 2 cases.

> >+#appmenuSecondaryPane {
> >+  border-left: 1px solid;
> 
> Use -moz-border-start instead for RTL support.

Done!

Thanks for the tip and the explanation.  I had wondered why so many places used this non-standard method of defining the border when there seemed to be a standard way to accomplish the same thing.

I will post a new patch as soon as my builds finish and I can do some testing.
(In reply to comment #207)
> > >+#appmenu-button.toolbarbutton {
> > 
> > This selector looks broken.
> 
> I do not understand the issue here.  This is a perfectly valid CSS selector
> that matches an element with an identifier of appmenu-button, if and only if it
> also has the class toolbarbutton associated with it.  My intention here was to
> match the Linux appmenu button by using a rule which would not match the
> Windows appmenu button.
> This way third party themers that copied the code form here could do something
> like this in their theme:
> 
> #appmenu-button-container > #appmenu-button {
>   styling for windows button goes here
> }
> #appmenu-button.toolbarbutton {
>   styling for Linux button goes here
> }
> 
> In this way they could provide one theme to work with both Linux and Windows
> that could sperratley style this element for the 2 cases.

My alternative idea here was to give the button a different identifier if it is a toolbar button.  Like use appmenu-toolbar-button instead of appmenu-button.  I could do that if you prefer.
Attached patch addresses review comments (obsolete) — Splinter Review
Addresses review comments.

Also, I decided, as an extension author who has an extension that modifies the Firefox menu, that any perceived advantage for extensions authors in maintaining the same identifier for the appmenu-button under Windows and Linux is far outweighed by the complications it adds to theme developers by having elements with such diverse requirements for themeing having the same identifier.

So, I have gone with the idea of changing the identifier for the applications menu under Linux as being appmenu-toolbar-button.

This also clears your review issue with the #appmenu-button.toolbarbutton CSS selector.
Attachment #481736 - Attachment is obsolete: true
Attachment #482113 - Flags: review?(dao)
Attached patch addresses review comments (obsolete) — Splinter Review
It appears that I accidentally attached the wrong patch file the first time.
Attachment #482113 - Attachment is obsolete: true
Attachment #482132 - Flags: review?(dao)
Attachment #482113 - Flags: review?(dao)
Comment on attachment 482132 [details] [diff] [review]
addresses review comments

>+          </hbox>
>+            <menuitem id="appmenu_downloads"

Indentation is off

>+#ifdef APPMENU_ON_TABBAR
>+      <toolbarbutton id="appmenu-toolbar-button"
>+                     class="toolbarbutton chromeclass-toolbar-additional"

Did you mean class="toolbarbutton-1 ..."?

> %ifdef XP_MACOSX
> toolbar[type="menubar"] {
>   min-height: 0 !important;
>   border: 0 !important;
> }
>-%endif
>-
>-%ifdef XP_WIN
>+%else
> toolbar[type="menubar"][autohide="true"] {
>   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide");
>   overflow: hidden;
> }

You need to take autohide="true" into account in global.css for the :-moz-system-metric(menubar-drag) case.
Attachment #479761 - Attachment is obsolete: true
Attachment #481273 - Attachment is obsolete: true
Attachment #477810 - Attachment is obsolete: true
Attachment #477813 - Attachment is obsolete: true
Attachment #472240 - Attachment is obsolete: true
(In reply to comment #211)
> Comment on attachment 482132 [details] [diff] [review]
> addresses review comments
> 
> >+          </hbox>
> >+            <menuitem id="appmenu_downloads"
> 
> Indentation is off

Fixed!  (although I think I deserve extra credit here.  I just copied the bad indentation from browser.xul)

> >+#ifdef APPMENU_ON_TABBAR
> >+      <toolbarbutton id="appmenu-toolbar-button"
> >+                     class="toolbarbutton chromeclass-toolbar-additional"
> 
> Did you mean class="toolbarbutton-1 ..."?

Well these comments from browser.xul seem to indicate that the toolbarbutton-1 class is intended for (re)movable toolbar buttons only, as it seemed to indicate this class was related to the items displayed in the toolbar palette.  It is very possible that I am wrong here.  So should I change this to toolbarbutton-1 and add the identifier to primaryToolbarButtons?

    <toolbarpalette id="BrowserToolbarPalette">
 
# Update primaryToolbarButtons in browser/themes/browserShared.inc when adding
# or removing default items with the toolbarbutton-1 class. 
 
> > %ifdef XP_MACOSX
> > toolbar[type="menubar"] {
> >   min-height: 0 !important;
> >   border: 0 !important;
> > }
> >-%endif
> >-
> >-%ifdef XP_WIN
> >+%else
> > toolbar[type="menubar"][autohide="true"] {
> >   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide");
> >   overflow: hidden;
> > }
> 
> You need to take autohide="true" into account in global.css for the
> :-moz-system-metric(menubar-drag) case.

Is this what you meant, or were you looking for something different?

+toolbar[type="menubar"][autohide="true"]:-moz-system-metric(menubar-drag),
 toolbar[type="menubar"]:not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
 }
(In reply to comment #212)
> > >+#ifdef APPMENU_ON_TABBAR
> > >+      <toolbarbutton id="appmenu-toolbar-button"
> > >+                     class="toolbarbutton chromeclass-toolbar-additional"
> > 
> > Did you mean class="toolbarbutton-1 ..."?
> 
> Well these comments from browser.xul seem to indicate that the toolbarbutton-1
> class is intended for (re)movable toolbar buttons only, as it seemed to
> indicate this class was related to the items displayed in the toolbar palette. 
> It is very possible that I am wrong here.  So should I change this to
> toolbarbutton-1 and add the identifier to primaryToolbarButtons?

toolbarbutton-1 isn't only for removable items. Anyway, it may not make a difference here.

There's no "toolbarbutton" class, though, and I don't think you want to introduce it either.

> > You need to take autohide="true" into account in global.css for the
> > :-moz-system-metric(menubar-drag) case.
> 
> Is this what you meant, or were you looking for something different?
> 
> +toolbar[type="menubar"][autohide="true"]:-moz-system-metric(menubar-drag),
>  toolbar[type="menubar"]:not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
>    -moz-binding:
> url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
>  }

The toolbar-drag binding must not be applied if the menubar is supposed to use the autohide binding.
(In reply to comment #213)
> toolbarbutton-1 isn't only for removable items. Anyway, it may not make a
> difference here.
> 
> There's no "toolbarbutton" class, though, and I don't think you want to
> introduce it either.

I could have sworn that I did a serach and found another non-movable button that had a class of toolbarbutton and copied. I sure can;t find it now.  maybe I was dreaming.  So I changed it to toolbarbutton-1.
 
> The toolbar-drag binding must not be applied if the menubar is supposed to use
> the autohide binding.

OK so more like this then:

toolbar[type="menubar"]:not([autohide="true"]):not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
}

Running new builds and will attach a new patch as soon as I can test.
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #482132 - Attachment is obsolete: true
Attachment #482427 - Flags: review?(dao)
Attachment #482132 - Flags: review?(dao)
Comment on attachment 482427 [details] [diff] [review]
patch v5

this needs an update for bug 602662
Attachment #482427 - Flags: review?(dao)
(In reply to comment #216)
> Comment on attachment 482427 [details] [diff] [review]
> patch v5
> 
> this needs an update for bug 602662

Thanks for the heads-up on this!

This saved me the effort of kicking off my builds before I left for work just have them fail and also in trying to figure out where the conflict actually was.

New patch is forthcoming.
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #482427 - Attachment is obsolete: true
Attachment #482773 - Flags: review?(dao)
Comment on attachment 482773 [details] [diff] [review]
patch v6

>+#appmenuSecondaryPane {
>+  -moz-border-start: 1px solid;

You should specify a color here, e.g. ThreeDShadow.
Attachment #482773 - Flags: review?(dao) → review+
(In reply to comment #219)
> Comment on attachment 482773 [details] [diff] [review]
> patch v6
> 
> >+#appmenuSecondaryPane {
> >+  -moz-border-start: 1px solid;
> 
> You should specify a color here, e.g. ThreeDShadow.

I tried ThreeDShadow, and the problem I ran into was that with a dark OS theme, the border became invisible (I guess this could be an issue with the theme I was using).  Anyway, MenuText seemed to work, but I got scared away from that by the Not all system colors are supported on all systems disclaimer.

Would it be acceptable to you for me to use -moz-MenuBarText as the color here?
(In reply to comment #220)
> Would it be acceptable to you for me to use -moz-MenuBarText 
> as the color here?

In most cases MenuBarText is black, right? In "normal" themes, this looks bad IMHO. ThreeDShadow is used for the menu border and horizontal separators was well, right? If so, why care for the vertical separator?

If there is a problem with dark themes, it is how the ThreeDShadow color is calculated.
Another thing: why does the menu list "Options..." at the top level and "Preferences" in the submenu?

- differently named menu items both open the same window
- the window is called "Firefox Preferences" (matches GNOME HIG)
- ellipsis on "Options..." is wrong according to GNOME HIG

IMHO this is quite confusing. The problem probably doesn't exist on windows, where the prefs window is called "Options" and other ellipis guidelines apply
(In reply to comment #221)
> (In reply to comment #220)
> > Would it be acceptable to you for me to use -moz-MenuBarText 
> > as the color here?
> 
> In most cases MenuBarText is black, right? In "normal" themes, this looks bad
> IMHO. ThreeDShadow is used for the menu border and horizontal separators was
> well, right? If so, why care for the vertical separator?
> 
> If there is a problem with dark themes, it is how the ThreeDShadow color is
> calculated.

OK I looked at how the separator is done.  it is actually a ThreeDShadow adjacent to a ThreeDHighlight.  I guess the theory is that one or the other will be visible.  I can do that.
(In reply to comment #222)
> Another thing: why does the menu list "Options..." at the top level and
> "Preferences" in the submenu?
> 
> - differently named menu items both open the same window
> - the window is called "Firefox Preferences" (matches GNOME HIG)

I will fix this.

> - ellipsis on "Options..." is wrong according to GNOME HIG
> 
> IMHO this is quite confusing. The problem probably doesn't exist on windows,
> where the prefs window is called "Options" and other ellipis guidelines apply

I can remove the elipsis also I guess, but I actually think that on both Windows and Linux the elipsis is wrong on many of these menus.  Seems to me there should either be an elipsis or the arrow.  Having both just seems redundant.

I will attach a new patch and ask for a second review from you.
(In reply to comment #224)
> I can remove the elipsis also I guess, but I actually think that on both
> Windows and Linux the elipsis is wrong on many of these menus.  Seems to me
> there should either be an elipsis or the arrow.  Having both just seems
> redundant.

The problem here is that traditionally, a menu item is either some kind of action or a category name (headline for the submenu). The new firefox menu breaks with this tradition. Still, the GNOME HIG is clear I think:

---
Label the menu item with a trailing ellipsis ("...") only if the command requires further input from the user before it can be performed. Do not add an ellipsis to items that only present a confirmation dialog (such as Delete), or that do not require further input (such as Properties, Preferences or About).
---

My first impression was that "Options..." has the ellipsis because it is also a sub menu, but this is not true because "New Tab" would have ellipsis too in that case.
This addresses the border and Preferences menu item issues.

Carrying Dão's r+ forward.
Attachment #482773 - Attachment is obsolete: true
Attachment #482857 - Flags: review?
Attachment #482857 - Flags: review? → review?(michael.monreal+moz)
(In reply to comment #226)
> Carrying Dão's r+ forward.

Don't do that when making changes previously not discussed...
Attachment #482857 - Flags: review?(michael.monreal+moz) → review?(dao)
Attached image v7 screenshot (obsolete) —
(In reply to comment #223)
> OK I looked at how the separator is done.  it is actually a ThreeDShadow
> adjacent to a ThreeDHighlight.

The separators in the new main menu do not seem to have ThreeDHighlight? With this, v7 now looks bad on the top and bottom line, because the separator draws all the way to the border (-> screenshot). I would just go with a normal ThreeDShadow for now and get this committed. Maybe clean up some cosmetic issues later.
(In reply to comment #228)
> Created attachment 482867 [details]
> v7 screenshot
> 
> (In reply to comment #223)
> > OK I looked at how the separator is done.  it is actually a ThreeDShadow
> > adjacent to a ThreeDHighlight.
> 
> The separators in the new main menu do not seem to have ThreeDHighlight? With
> this, v7 now looks bad on the top and bottom line, because the separator draws
> all the way to the border (-> screenshot). I would just go with a normal
> ThreeDShadow for now and get this committed. Maybe clean up some cosmetic
> issues later.

OK.  I'll go back to just the ThreeDShadow and if there is really an issue with dark themes that can be worked later.  This actually looks much better on my system than it does on yours, which is part of the issue with trying to customize the look of something that depends partially on the native themeing I guess.
(In reply to comment #229)
> This actually looks much better on my system than it does on yours

Sure... I selected this color scheme to become aware of exactly this kind of problems in Fx :)
Attached patch patch v8 (obsolete) — Splinter Review
OK.  THis si what Dao R+'d before with just the change from "Options..." to "Preferences" in the Menu.

Not sure this really needs a new r+ but asking anyway.
Attachment #482857 - Attachment is obsolete: true
Attachment #482867 - Attachment is obsolete: true
Attachment #482913 - Flags: review?(dao)
Attachment #482857 - Flags: review?(dao)
(In reply to comment #230)
> (In reply to comment #229)
> > This actually looks much better on my system than it does on yours
> 
> Sure... I selected this color scheme to become aware of exactly this kind of
> problems in Fx :)

Actually, it is more that with the theme I am using, the things that look bad at the intersection, do not actually intersect.
(In reply to comment #227)
> (In reply to comment #226)
> > Carrying Dão's r+ forward.
> 
> Don't do that when making changes previously not discussed...

OK. Sorry.
Attachment #482913 - Flags: review?(dao) → review+
Comment on attachment 482913 [details] [diff] [review]
patch v8

Asking for approval2.0 or approval for beta8, whatever is being done now.

I would like this to be checked in soon to maximize the amount of trunk bake time before this gets before it is incorporated into a beta or release candidate.
Attachment #482913 - Flags: approval2.0?
(In reply to comment #233)
> (In reply to comment #227)
> > (In reply to comment #226)
> > > Carrying Dão's r+ forward.
> > 
> > Don't do that when making changes previously not discussed...
> 
> OK. Sorry.

Actually Dão, I would like to thank you for the review.  I learned a lot here, which I think is one of the points of the review process.  Besides making sure people don't check in things without peer review, I think the idea is that people trying to work on an area they are not familiar with submitting a patch and asking for a review from the responsible people should end up learning more about an area of the codebase that you they not be familiar with.

Also, thank you for being patient with me as someone trying to learn here.
I filed bug 604650 and bug 604651 on the accelerator text and the icon.
Blocks: 552302
Depends on: 604650, 604651
No longer depends on: 552302
Comment on attachment 482913 [details] [diff] [review]
patch v8

This needs ui+r before approval. I bet screenshots would help with that.
Attachment #482913 - Flags: ui-review?(faaborg)
This shows the menu in the open state.
It is important to note that for the purpose of this bug, the UI review is not really to make sure this is in the final release state, just to make sure that what we have here is OK for beta8.  Bug 604650 is intended to select the proper toolbar icon and bug 604651 is still pending to hide the accelerator shortcuts from the top level applications menu.  I have a patch on this that is dependent on the patch here.  I will be posting a screenshot of what the menu looks like with that patch applied.

Also this screenshot was taken on a system with the gnome default of not displaying the menu icons being overridden to show what the menu looks like with the icons.  If you don't override that gnome setting this displays, as expected, with no menu icons.
I've tried latest build from Bill Gianopoulos server on Ubuntu 10.10 :
nice work but i've noticed 2 things :
- Web Developer entry still behave differently from other entries (regarding the fact pointing the label displays the submenu (note that i much prefer this behaviour than the other)
- i'd like to see my c#201 discussed, concerning the cut/copy/paste icons that are displayed even with GNOME is set to not show these menu
icons by default. On Chromium, these buttons are label-only on GNOME when
menus_have_icons is set to false. I think these buttons should have 2 state : label-only on GNOME when menus_have_icons is set to false and icon-only on the contrary. At present time icon-only is the only implementation which seems really weird considering that no other GNOME application use these icons by default. Thanks
(In reply to comment #240)
> - i'd like to see my c#201 discussed, concerning the cut/copy/paste icons that
> are displayed even with GNOME is set to not show these menu
> icons by default. On Chromium, these buttons are label-only on GNOME when
> menus_have_icons is set to false. I think these buttons should have 2 state :
> label-only on GNOME when menus_have_icons is set to false and icon-only on the
> contrary. At present time icon-only is the only implementation which seems
> really weird considering that no other GNOME application use these icons by
> default. Thanks

No other GNOME app use these icons? Open gedit, for example. Look at the toolbar. Notice the icons. These are buttons. Even on this Firefox "menu", the copy/cut/paste icons are buttons, not menu entries with an icon (or not) illustrating the menu entry.
(In reply to comment #241)
> (In reply to comment #240)
> > - i'd like to see my c#201 discussed, concerning the cut/copy/paste icons that
> > are displayed even with GNOME is set to not show these menu
> > icons by default. On Chromium, these buttons are label-only on GNOME when
> > menus_have_icons is set to false. I think these buttons should have 2 state :
> > label-only on GNOME when menus_have_icons is set to false and icon-only on the
> > contrary. At present time icon-only is the only implementation which seems
> > really weird considering that no other GNOME application use these icons by
> > default. Thanks
> 
> No other GNOME app use these icons? Open gedit, for example. Look at the
> toolbar. Notice the icons. These are buttons. Even on this Firefox "menu", the
> copy/cut/paste icons are buttons, not menu entries with an icon (or not)
> illustrating the menu entry.

You can't take these exemples since none of these applications makes use of these "buttons within menu" that can be find in current patch. 
From a user point of view, everything that sit in a menu is a menu entry. It can be a label-only entry, an icon-only entry or an icon+label entry. 
When GNOME has menus_have_icons set to false, you'll not find any application displaying cut/copy/paste icons within menus, only labels.
For the record, Bug 465669 was about considering scaling back the use of menu icons in the Tango theme
(In reply to comment #240)
> I've tried latest build from Bill Gianopoulos server on Ubuntu 10.10 :
> nice work but i've noticed 2 things :
> - Web Developer entry still behave differently from other entries (regarding
> the fact pointing the label displays the submenu (note that i much prefer this
> behaviour than the other)

This is being worked in bug 589146.

> - i'd like to see my c#201 discussed, concerning the cut/copy/paste icons that
> are displayed even with GNOME is set to not show these menu
> icons by default. On Chromium, these buttons are label-only on GNOME when
> menus_have_icons is set to false. I think these buttons should have 2 state :
> label-only on GNOME when menus_have_icons is set to false and icon-only on the
> contrary. At present time icon-only is the only implementation which seems
> really weird considering that no other GNOME application use these icons by
> default. Thanks

I would rather not do any more changes than absolutely necessary to the menu itself until after this lands on the trunk.  The most difficult part of keeping this patch up to date is syncing the new include file with changes being made in other bugs to the Firefox menu.  Every change I make to the include file in this bug just increases the work involved in keeping it in sync with other menu changes.

Also, once this lands on the trunk, it will be easier for a wider audience to see how this works and intelligently comment on what you are talking about.

So, my preference would be to do a followup bug on this issue.

Personally, I don't like the way the edit buttons work even under windows.  If they are going to be buttons, the probably belong on the titlebar next to the Firefox button.  Having them in the menu but not work at all like like menu items and having the word edit grayed out always just makes no sense.
(In reply to comment #240)
> I've tried latest build from Bill Gianopoulos server on Ubuntu 10.10 :
> nice work but i've noticed 2 things :
> - Web Developer entry still behave differently from other entries (regarding
> the fact pointing the label displays the submenu (note that i much prefer this
> behaviour than the other)

I should have mentioned that the reason for the difference int he way the hover works, is that on the menu items where the label and the arrow highlight separately is because clicking on the label has a different action than clicking on the arrow.  The reason that WebDeveloper hover works differently is that there is no special action for clicking on the label, hence it performs the same function as clicking on the arrow and so the highlight on hover just highlights both.
(In reply to comment #245)
> (In reply to comment #240)
> > I've tried latest build from Bill Gianopoulos server on Ubuntu 10.10 :
> > nice work but i've noticed 2 things :
> > - Web Developer entry still behave differently from other entries (regarding
> > the fact pointing the label displays the submenu (note that i much prefer this
> > behaviour than the other)
> 
> I should have mentioned that the reason for the difference int he way the hover
> works, is that on the menu items where the label and the arrow highlight
> separately is because clicking on the label has a different action than
> clicking on the arrow.  The reason that WebDeveloper hover works differently is
> that there is no special action for clicking on the label, hence it performs
> the same function as clicking on the arrow and so the highlight on hover just
> highlights both.

Thanks for the precision. However don't you think that this kind of behaviour may be too subtle for the vast majority of users ? (I'ts probably beyond the scope of this bug)
Hopefully menu bar will not be hidden by default on Linux and will remain an (useful - especially for notebooks) advanced feature
(In reply to comment #246)
> (In reply to comment #245)
> > (In reply to comment #240)
> > > I've tried latest build from Bill Gianopoulos server on Ubuntu 10.10 :
> > > nice work but i've noticed 2 things :
> > > - Web Developer entry still behave differently from other entries (regarding
> > > the fact pointing the label displays the submenu (note that i much prefer this
> > > behaviour than the other)
> > 
> > I should have mentioned that the reason for the difference int he way the hover
> > works, is that on the menu items where the label and the arrow highlight
> > separately is because clicking on the label has a different action than
> > clicking on the arrow.  The reason that WebDeveloper hover works differently is
> > that there is no special action for clicking on the label, hence it performs
> > the same function as clicking on the arrow and so the highlight on hover just
> > highlights both.
> 
> Thanks for the precision. However don't you think that this kind of behaviour
> may be too subtle for the vast majority of users ? (I'ts probably beyond the
> scope of this bug)
> Hopefully menu bar will not be hidden by default on Linux and will remain an
> (useful - especially for notebooks) advanced feature

It is beyond the scope of this bug as this is the identical behavior as under windows.  As I said above, the way this works is being redesigned and addressed in bug 589146.
(In reply to comment #240)
> - i'd like to see my c#201 discussed, concerning the cut/copy/paste icons that
> are displayed even with GNOME is set to not show these menu
> icons by default. On Chromium, these buttons are label-only on GNOME when
> menus_have_icons is set to false.

Chromium is a bad example it displays no menu icons and these buttons are label only even if menus_have_icons is set to true.

<soapbox>It should also be noted, if you actually read the bug that created the gnome change, this was not made a a design decision that menus should not have icon, it was in fact, a performance improvement suggestion that since menus open faster without icons, then menu icons should not be displayed to speed up the opening of the menu.  I would have though at least some attempt could have been made to actually fix the performance issue in displaying menu icons before copping out and just not showing them and pretending this was a design looks better decision.</soapbox>
Comment on attachment 482913 [details] [diff] [review]
patch v8

A few small changes to bring this more in line with the design of the Firefox menu on other platforms:

-The button should either contain a Firefox icon or the string "Firefox"  We want this to be consistently known as "The Firefox Button."  Adding a caveat of "or a small menu icon if you are on Linux" to support documents is clunky.  Also, the commands follow the normal menu effect of building through a sentence through the path: "Firefox Print" "Firefox Find" (kind of like giving commands to a dog)

-Keyboard shortcuts should be removed from the top level.  They are available with tooltips, and on first item of the second level (which is intentionally redundant partly to expose the keyboard shortcut).  The additional complexity of displaying the keyboard shortcuts on the top level reduces the relative discoverabilty of each item, because there is more information being shown.

-The right pane of the menu should contain a different background color to differentiate it from the left pane.  Even if the user doesn't conceptualize that the left pane contains actions and the right pane contains places (new tabs and windows), the different styling will help them leverage spatial memory (bookmarks on the darker area).  Just using the window color for the right pane is fine, since on linux we are limited to extracted system colors.

-There should be some space in between downloads and add-ons to visually convey that there are two groups of commands on the right side (each containing three items).
Attachment #482913 - Flags: ui-review?(faaborg) → ui-review-
Alternatively we could address those points in follow up bugs and land this patch, really whatever people prefer.
>this was not made a a design decision that menus should not have
>icon, it was in fact, a performance improvement suggestion that since menus
>open faster without icons

There's a human performance aspect here as well, if you only display icons for the highly used items, they are easier for the user to spot (compared to using icons for all of the items), so users are more likely to be able to quickly find the target they are looking for.  For instance on windows the context menu of the recycle bin places an icon next to "empty recycle bin" but not on any of the other items in the menu.  This visual search optimization is why we only included icons for some of the items in the Firefox menu.
(In reply to comment #249)
> Comment on attachment 482913 [details] [diff] [review]
> patch v8
> 
> A few small changes to bring this more in line with the design of the Firefox
> menu on other platforms:
> 
> -The button should either contain a Firefox icon or the string "Firefox"  We
> want this to be consistently known as "The Firefox Button." 

That is what I wanted to do int he first plcae.  I can easily change to using the application icon and to using brandShortname as the button label, but Dao seemed to be unwilling to give me an r+ with that.


> -Keyboard shortcuts should be removed from the top level.  They are available
> with tooltips, and on first item of the second level (which is intentionally
> redundant partly to expose the keyboard shortcut).  The additional complexity
> of displaying the keyboard shortcuts on the top level reduces the relative
> discoverabilty of each item, because there is more information being shown.

It appears this is very difficult to do because of some interaction that I do not understand between either trying to do this via the current bindings or by trying to add a new class on the elements and do it via css and Linux native themeing that seems to somehow break both of these approaches.

> -The right pane of the menu should contain a different background color to
> differentiate it from the left pane.  Even if the user doesn't conceptualize
> that the left pane contains actions and the right pane contains places (new
> tabs and windows), the different styling will help them leverage spatial memory
> (bookmarks on the darker area).  Just using the window color for the right pane
> is fine, since on linux we are limited to extracted system colors.
> 

I tried using a different color for the righthand pane and the issue was that it did not work if i used a gnome theme that was dark rather than light.  I thought this was perhaps an issue that could be resoved by the linux theme team after this landed or was hoping someone could give me a clue as to how to make this work better.


> -There should be some space in between downloads and add-ons to visually convey
> that there are two groups of commands on the right side (each containing three
> items).

I should be able to do something here given that it does display that way under windows.  However, I might ask the question, if everywhere else where we make this type of distinction, we use a separator, why do we not do so here?
>That is what I wanted to do int he first plcae.  I can easily change to using
>the application icon and to using brandShortname as the button label, but Dao
>seemed to be unwilling to give me an r+ with that.

right, vaguely remember my previous comments higher up in the bug.  In that case we should have beltzner make the final call on the button name.

>It appears this is very difficult to do because of some interaction that I do
>not understand between either trying to do this via the current bindings or by
>trying to add a new class on the elements and do it via css and Linux native
>themeing that seems to somehow break both of these approaches.

Let's move that to a follow up bug then.

>I tried using a different color for the righthand pane and the issue was that
>it did not work if i used a gnome theme that was dark rather than light.  I
>thought this was perhaps an issue that could be resoved by the linux theme team
>after this landed or was hoping someone could give me a clue as to how to make
>this work better.

Shouldn't text color always work on window color?

>I should be able to do something here given that it does display that way under
>windows.  However, I might ask the question, if everywhere else where we make
>this type of distinction, we use a separator, why do we not do so here?

Two reasons (but neither of them are really strong reasons).  We wanted the set of items to visually group together since they are all "places" while still providing some separation to reduce visual scan time (although really just the background color is probably enough to achieve that).  The second reason is that the line would have a strange appearance when viewed with the items on the left side.  Due to the padding that the lines get it wouldn't really line up with anything land mid command and would feel unbalanced.
(In reply to comment #253)
> >That is what I wanted to do int he first plcae.  I can easily change to using
> >the application icon and to using brandShortname as the button label, but Dao
> >seemed to be unwilling to give me an r+ with that.
> 
> right, vaguely remember my previous comments higher up in the bug.  In that
> case we should have beltzner make the final call on the button name.

On the button name, it seems to me that the argument to change form using brandShortname to Menu was a holdover from when we were trying to make it be a (re)movable item.  When it was in the toolbar palette it seemed to make no sense to have it's label be Firefox or Minefield as this did not seem to indicate  the function of the button so Menu made a lot more sense.  NOw that it is no longer (re)movable it would seem it woudl make more sense to revert to it being labeled as brandShortname, the same as under Windows.

As far as the icon goes, I always wanted to use the application icon, but some people thought it looked redundant because under most Linux windows managers with tabs-on-top it would be positioned directly under the application icon in the titlebar.

> 
> >It appears this is very difficult to do because of some interaction that I do
> >not understand between either trying to do this via the current bindings or by
> >trying to add a new class on the elements and do it via css and Linux native
> >themeing that seems to somehow break both of these approaches.
> 
> Let's move that to a follow up bug then.
> 
> >I tried using a different color for the righthand pane and the issue was that
> >it did not work if i used a gnome theme that was dark rather than light.  I
> >thought this was perhaps an issue that could be resoved by the linux theme team
> >after this landed or was hoping someone could give me a clue as to how to make
> >this work better.
> 
> Shouldn't text color always work on window color?
> 

I will attach screenshots of what this ens up looking like using the "high Contrast Inverse" gnome theme which is one of the ones installed by default under fedora.

 
> >I should be able to do something here given that it does display that way under
> >windows.  However, I might ask the question, if everywhere else where we make
> >this type of distinction, we use a separator, why do we not do so here?
> 
> Two reasons (but neither of them are really strong reasons).  We wanted the set
> of items to visually group together since they are all "places" while still
> providing some separation to reduce visual scan time (although really just the
> background color is probably enough to achieve that).  The second reason is
> that the line would have a strange appearance when viewed with the items on the
> left side.  Due to the padding that the lines get it wouldn't really line up
> with anything land mid command and would feel unbalanced.

OK New patch coming up that fixes the Secondary panel spacing issue.
(In reply to comment #250)
> Alternatively we could address those points in follow up bugs and land this
> patch, really whatever people prefer.

Here are my reasons why this should be checked-in ASAP.

1.  On the assumption that this will be in Firefox 4, I think the state it is in in is good enough to get it to the Linux UI team can massage it to a final state, and getting it in now gives more time for user feedback on any issues it might cause.

2.  I really think we need to fix the issue of MENUBAR-CAN_AUTOHIDE being overloaded to also mean CAN_DRAW_IN_TITLEBAR.  This patch separates the 2 into separate defines so that code that depends on each can be correctly ifdef'd.

3.  (largely selfish) It is a pain to keep this bug from bit-rotting because of changes being made to the Applications menu.

The overriding negative to checking it in now, is that if the issue of not being able to hide Keyboard shortcuts from the top level is a showstopper for getting this into Firefox 4, then based on my attempts to do this so far, i am not sure getting that done in time is a reasonable target.  So if that is really a showstopper, we might be at an impasse for getting this landed quickly.
This addresses the spacing issue on the secondary menu pane.
Attachment #482913 - Attachment is obsolete: true
Attachment #484610 - Flags: ui-review?(faaborg)
Attachment #484610 - Flags: review+
Attachment #482913 - Flags: approval2.0?
Attachment #484130 - Attachment is obsolete: true
Comment on attachment 484610 [details] [diff] [review]
patch v9-fix spacing of Secondary pane menu items

giving a ui-review+ to get this landed, with the assumption that we'll work on a few additional aspects of it in follow up bugs.
Attachment #484610 - Flags: ui-review?(faaborg) → ui-review+
This is with the High Contrast Inverse Theme
This is with specifying the same background color as we use in winstripe but using the High Contrast Inverse Gnome Theme.

Obviously I could make the text etc. be visible by specifying test and highlight colors, etc., but this still would never fit in with the OS theme.  That is why I left it without specifying the background color in my original patch.
Attachment #484610 - Flags: approval2.0?
OK carrying forward Dão's review, I am once again seeking approval-2.0 to try to get this landed soon for the following reasons:

1.  On the assumption that this will be in Firefox 4, I think the state it is
in in is good enough to get it to the Linux UI team can massage it to a final
state, and getting it in now gives more time for user feedback on any issues it
might cause.

2.  I really think we need to fix the issue of MENUBAR-CAN_AUTOHIDE being
overloaded to also mean CAN_DRAW_IN_TITLEBAR.  This patch separates the 2 into
separate defines so that code that depends on each can be correctly ifdef'd.

3.  (largely selfish) It is a pain to keep this bug from bit-rotting because of
changes being made to the Applications menu.
Alex: you're really OK with adding this button on Linux and Linux only? Seems a little awkward to me, really, and I had always thought that we'd decided not to implement the Firefox Button on Linux and OSX.

Pretty sure I said as much earlier, and while I don't mean to spite the work, I'm still not sure I understand why we want this. Is the assumption that since it's a toolbar button that users have to manually customize that we should just let it in?
(In reply to comment #263)
> Alex: you're really OK with adding this button on Linux and Linux only? Seems a
> little awkward to me, really, and I had always thought that we'd decided not to
> implement the Firefox Button on Linux and OSX.
> 
> Pretty sure I said as much earlier, and while I don't mean to spite the work,
> I'm still not sure I understand why we want this. Is the assumption that since
> it's a toolbar button that users have to manually customize that we should just
> let it in?

I'm only a user but i'd like to give some reasons in favor of having such a button.
1°) According to Bill Gianopoulos, Linux users really expects not behing left behind ; they expect to benefit from some UI changes too. The Firefox button is one of them.
2°) Firefox 4 will left the status bar, and having that button would allow to save twice the vertical space which is important on a notebook with a 7 or 9" screen : notebook users will appreciate. Moreover some Linux vendors may appreciate having such an option to keep using Firefox even if i can't speak for them : i know that Canonical was holding a discussion about switching to Chromium for its netbook version for instance
3°) If I've got it right, hiding the menu bar in favor of this button will not be default setting therefore i see some real benefits and no drawbacks
Thanks
(In reply to comment #264)
> 1°) According to Bill Gianopoulos, Linux users really expects not behing left
> behind ; they expect to benefit from some UI changes too. The Firefox button 
> is one of them.

I know what Bill thinks. I don't think this is giving them the benefit of the UI changes. The Firefox Button is about removing the menubar and replacing that UI space with a button that replaces it. That's not what this patch does. It adds a button to the toolbar. We don't do that on any other operating system for a reason: doing so removes space from the tabstrip.

> 2°) Firefox 4 will left the status bar, and having that button would allow to
> save twice the vertical space which is important on a notebook with a 7 or 9"
> screen : notebook users will appreciate. Moreover some Linux vendors may
> appreciate having such an option to keep using Firefox even if i can't speak
> for them : i know that Canonical was holding a discussion about switching to
> Chromium for its netbook version for instance

Irrelevant. If distributors want to make a modification like this, they can and do know how to get in touch with us to make such modifications.

> 3°) If I've got it right, hiding the menu bar in favor of this button will not
> be default setting therefore i see some real benefits and no drawbacks
> Thanks

Adding code always has drawbacks. It must be maintained, it must be designed, and it must be tested.
(In reply to comment #263)
> Pretty sure I said as much earlier, and while I don't mean to spite the work,
> I'm still not sure I understand why we want this. Is the assumption that since
> it's a toolbar button that users have to manually customize that we should just
> let it in?

Users wouldn't customize the button. They would hide the menu bar and the button would appear.
(In reply to comment #266)
> Users wouldn't customize the button. They would hide the menu bar and the
> button would appear.

Yeah, then I'm pretty sure that we don't want this in the tabstrip. The design is that the menuButton goes in the toolbar.

Alex: does this make you reconsider, or do you feel that I'm thinking about this wrong?
Trying to balance cross platform UI consistency while also meshing with the interactive design with the surrounding platform is in many cases (like this one) simply not possible.  There are however a lot of reasons why users would want to customize Firefox on one platform to have the same interactive design as Firefox on another platform.  For instance, the user might have a Windows 7 machine at work, and a linux machine at home, and want to have the two applications behave the same.  So our approach has been to fracture the Firefox UI across platforms for the sake of interactive integration on each, but still allow users to customize it to be the same everywhere.  This is also why we would like to add the Firefox button as an optional control on OS X (and already provide the option on XP).

The additional benefit is that users can save some vertical screen space with the Firefox button, so they may want to use it even if they aren't familiar with the interface from using a Windows machine.  Or the may prefer it because it makes the main window less visually complex.

>Yeah, then I'm pretty sure that we don't want this in the tabstrip. The design
>is that the menuButton goes in the toolbar.

Placing in the tabstrip isn't ideal, but without the ability to draw in the title bar this was the only way for us to expose an app level control at the right level of the interface.  We could however just ignore that for the time being and put it in the current tab's navigation toolbar just to free up some tabstrip space.  I'm mostly indifferent with a slight preference towards placing it at the right level at the cost of taking some tabstrip space.
I thought it was about time I weighed in with my opinion on this.  First I want to provide a bit of background as to how we arrived at where we are.

The original idea was to make this a user placeable toolbar item with the default position being at the left hand side of the tabbar with the idea being that, although placing it on the tabbar might not make sense for a menu, with the tabs-on-top layout that would put the menu button at the upper left of the window, which was the most logical location.  If the user felt this should be someplace else, it could easily be moved.

It was then found that at several places in the code, #ifdef MENUBAR_CAN_AUTOHIDE was used to ensure that document.getElementById of appmenu items would not be null.  The problem is that if the button we removed form the toolbars and placed in the toolbar palette bu the user, that is exactly what would happen.  So either null checks or try/catch needed to be added to avoid exceptions.  As an example of this problem, private browsing mode failed to correctly initialize until I fixed the code there when I first implemented this.

So, a decision was made to make it non user (re)movable.

So, that is the current state of the patch here.

If we now want to move this to the toolbar, I have no idea where it would make sense to put it on the toolbar as a non-movable item.  The far left seems to NOT be a good idea at all based on all the complaints we have in the past when we accidentally had extra padding or something to the left of the back button that prevented users form being able to do a back by pinning the mouse on the screen left when the screen is is maximized mode.  I suspect that if we place a new item to the left of the back button a riot might ensue.

Therefore, at this point I think the choices are:

1.  Land the patch as is.

2.  Go back to making the item (re)movable and come up with a logical default location (which might be just leave it in the palette).

3.  Do nothing and I will try to implement this as an extension after the content and layout of the menu is finalized.
The maintenance cost is pretty low since the interesting code is shared across platforms. To me being able to hack the menu on Linux would in fact be a help.

An add-on would need to start from scratch and I wouldn't encourage Linux distributors to develop and ship their own.
(In reply to comment #268)
> Trying to balance cross platform UI consistency while also meshing with the
> interactive design with the surrounding platform is in many cases (like this
> one) simply not possible.  There are however a lot of reasons why users would

I guess the sticking point for me is: why Linux but not OS X? I can understand why we're making it the default in Windows 7 and Vista, and understand how we get from there to making it available (but not default) in XP.

Is it the case that we're saying since OS X has a global menu bar, we're not interested in implementing the Firefox button to save on UI space since it's not a concern on that platform? And thus on Linux, which may or may not have that global element (depending on the distro and what you believe will or will not happen with future versions of Ubuntu) we are providing it, but not turning it on by default?

> Placing in the tabstrip isn't ideal, but without the ability to draw in the
> title bar this was the only way for us to expose an app level control at the
> right level of the interface.  We could however just ignore that for the time
> being and put it in the current tab's navigation toolbar just to free up some
> tabstrip space.  I'm mostly indifferent with a slight preference towards
> placing it at the right level at the cost of taking some tabstrip space.

Placing it in the navigation toolbar doesn't make sense to me at all with tabs on top, no. If we do place it in the tabstrip, it must appear as it does on other platforms ("Firefox" with a down arrow), though, not as a single icon. That's going to cut into tabstrip space, which will be a tradeoff that I guess we let users decide.

Looks like we're moving forward, here, thanks for the clarification.
(In reply to comment #271)
> I guess the sticking point for me is: why Linux but not OS X? I can understand
> why we're making it the default in Windows 7 and Vista, and understand how we
> get from there to making it available (but not default) in XP.

My early patches from when this was user placeable, added it for all platforms except Windows.  Under Linux it was placed by default on the tabbar, for other OS's it was just left unplaced in the toolbar palette.  During patch review, it was decided to keep this bug Linux only and allow the UI people for other platforms make their own decision on what to do in separate bugs.
This pretty much sums up my point of view:

> Is it the case that we're saying since OS X has a global menu bar, we're not
> interested in implementing the Firefox button to save on UI space since it's
> not a concern on that platform? And thus on Linux, which may or may not have
> that global element (depending on the distro and what you believe will or will
> not happen with future versions of Ubuntu) we are providing it, but not turning
> it on by default?

A global menu bar on Linux would likely need our explicit support like on OS X. Once we provide that and depending on the desktop environment, we'd remove the "menu bar" toggle that this bug's patch adds.
If I well  remember, we want too the firefox button to be implemented in Linux because it could drive to addons fracture between Linux and Windows (as Linux won't benefit of windows addons ; a few addons developer will develop specific overlays for Linux ; Linux don't have a global menu like OSX).
Notes on this patch:

1.  I made no attempt to theme the Firefox button.  I figured any attempt on my part was unlikely to be what was decided on and only delay getting the patch landed so that the ui team could theme this as desired.

2.  I segregated the css code into code required to make things work, which I put into browser/base/content/browser.css from css code to define the look and feel which I put into browser/themes/gnomestripe/browser/browser.css.  This results in the Firefox button working correctly with most third party themes in a way that it did not with the previous patches I attached here.

3.  I should note, before I am asked, that it was intentional to remove the toolbarbutton-1 class from the button.  I looked through the source with mxr and found that all of the extra styling that the toolbarbutton-1 class was adding was things that I was overriding in css code in the gnomestripe theme, so that seemed counterproductive.
Attachment #479763 - Attachment is obsolete: true
Attachment #480062 - Attachment is obsolete: true
Attachment #484612 - Attachment is obsolete: true
Attachment #484617 - Attachment is obsolete: true
Attachment #484618 - Attachment is obsolete: true
Attachment #485642 - Flags: ui-review?(faaborg)
Attachment #485642 - Flags: review?(dao)
Attachment #484610 - Attachment is obsolete: true
Attachment #484610 - Flags: approval2.0?
Attached image screenshot of themed button (obsolete) —
Here is the userChrome.css code I used to do this.  If this is considered better than the non-themed patch, I can add this.

#appmenu-toolbar-button {
  -moz-appearance: none !important;
  color: white !important;
  font-weight: bold !important;
  background: -moz-linear-gradient(rgb(247,182,82), rgb(215,98,10) 95%);
}
I decided to just submit the patch for review including my themeing attempt.  Since I am asking for ui review also, I guess this can be discussed via the ui review process.
Attachment #485642 - Attachment is obsolete: true
Attachment #485648 - Flags: ui-review?(faaborg)
Attachment #485648 - Flags: review?(dao)
Attachment #485642 - Flags: ui-review?(faaborg)
Attachment #485642 - Flags: review?(dao)
Attachment #485648 - Attachment description: patch v10-including my attempt to theme the button → patch v11-including my attempt to theme the button
(In reply to comment #279)
> Attachment 485643 [details] looks way better than attachment 485647 [details] to me.

I am actually not particularly a fan of either one.  The first one seems to not have enough prominence to differentiate it from other items on the tab bar.  Since Beltzner is concerned that this does not logically belong on the tabs bar, it would be nice to have more visual difference than my first option provides.

I also suspect he would prefer it to look more like the windows implementation than this does.

The second choice I provided, I think is too far int he other direction.  I just took the windows styling and then copied it and added bolding the application name because it did not look as well with normal weight white on the background as it does under windows.

This one I think is a bit too distinctive, the orange is way too prominent and it is therefore distracting.  I am also not sure that the white text looks right given that the drop-marker is still black.

I think maybe going back to the first choice and figuring out some way to add maybe an slight Firefox orange glow tot he button is maybe a good compromise.
>I guess the sticking point for me is: why Linux but not OS X? 

The only reason is that Bill is working on this patch for Linux, while we haven't had anyone pick up the work on OS X yet.
>Attachment 485643 [details] looks way better than attachment 485647 [details] to me.

I agree, let's land this without any stronger visually styling for the time being.  We can try to follow up to give it more prominence later, but the range of OS themes on Linux will also somewhat limit what we are able to do.
Comment on attachment 485648 [details] [diff] [review]
patch v11-including my attempt to theme the button

Let's roll back to the un-themed button in v10.  Without any significant interface changes you can feel free to carry the previous ui-reviews forward so we can get this landed once it passes code review.
Attachment #485648 - Flags: ui-review?(faaborg)
Comment on attachment 485642 [details] [diff] [review]
patch v10-use text label with brandShortName instead of icon

I was going to suggest this myself.  I realized today that the v10 patch was better because it was easier to play with approaches to add some orange via userChrome.css, if that is what we want, to something that does not have extraneous themeing.
Attachment #485642 - Attachment is obsolete: false
Attachment #485642 - Flags: ui-review+
Attachment #485642 - Flags: review?(dao)
Attachment #485648 - Attachment is obsolete: true
Attachment #485648 - Flags: review?(dao)
Attachment #484618 - Attachment is obsolete: false
Attachment #484617 - Attachment is obsolete: false
Attachment #484612 - Attachment is obsolete: false
Attachment #485647 - Attachment is obsolete: true
Comment on attachment 485642 [details] [diff] [review]
patch v10-use text label with brandShortName instead of icon

The label increases the tab bar height; it shouldn't do that.

You need to update aboutPrivateBrowsing.xhtml, which uses appMenuButton.label.

>+#appmenu-toolbar-button > .toolbarbutton-text {
>+  display: inherit;
>+}

s/inherit/-moz-box/
Attachment #485642 - Flags: review?(dao) → review-
(In reply to comment #285)
> Comment on attachment 485642 [details] [diff] [review]
> patch v10-use text label with brandShortName instead of icon
> 
> The label increases the tab bar height; it shouldn't do that.

Fixed.  I should have caught that.  I put the change for this in the gnomestripe theme as I think the height of the tabbar is a theme thing even though I had put the change to showing the label in browser/base/content as I determined that change to really be theme independent, at least for the default.

 
> You need to update aboutPrivateBrowsing.xhtml, which uses appMenuButton.label.

Good catch!
 
> >+#appmenu-toolbar-button > .toolbarbutton-text {
> >+  display: inherit;
> >+}
> 
> s/inherit/-moz-box/

Fixed.
Carrying faaborg's ui-review+ forward.
Attachment #485642 - Attachment is obsolete: true
Attachment #486218 - Flags: ui-review+
Attachment #486218 - Flags: review?(dao)
Comment on attachment 486218 [details] [diff] [review]
patch v12-fix button height and other review issues

Oops.  Accidentally built a patch with all my pending patches instead of only those for this bug.  New patch forthcoming.
Attachment #486218 - Attachment is obsolete: true
Attachment #486218 - Flags: review?(dao)
Correct patch.  Once again carrying faaborg's ui-review+ forward.
Attachment #486313 - Flags: ui-review+
Attachment #486313 - Flags: review?
Comment on attachment 486313 [details] [diff] [review]
patch v12a-fix button height and other review issues

>--- a/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
>+++ b/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
>@@ -40,22 +40,21 @@
>   <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd">
>   %htmlDTD;
>   <!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">
>   %netErrorDTD;
>   <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
>   %globalDTD;
>   <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">
>   %browserDTD;
>-#ifdef XP_WIN
>   <!ENTITY basePBMenu.label   "<span class='appMenuButton'>&brandShortName;</span><span class='toolsMenu'>&toolsMenu.label;</span>">
>-#elifdef XP_MACOSX
>+#ifdef XP_MACOSX
>   <!ENTITY basePBMenu.label   "&toolsMenu.label;">
> #else
>-  <!ENTITY basePBMenu.label   "<span class='appMenuButton'>&appMenuButton.label;</span><span class='toolsMenu'>&toolsMenu.label;</span>">
>+  <!ENTITY basePBMenu.label   "<span class='appMenuButton'>&brandShortName;</span><span class='toolsMenu'>&toolsMenu.label;</span>">
> #endif

This is broken (defines basePBMenu.label twice).
Attachment #486313 - Flags: review? → review-
(In reply to comment #290)

> This is broken (defines basePBMenu.label twice).

Sigh ... Looks like I can't even cut and paste correctly.  Lucky version 13 coming up.
Attached patch patch v13-fix review issues (obsolete) — Splinter Review
Once again, Carrying faaborg's ui-review+ forward.
Attachment #486313 - Attachment is obsolete: true
Attachment #486897 - Flags: ui-review+
Attachment #486897 - Flags: review?(dao)
Attached patch interdiff v12a to v13 (obsolete) — Splinter Review
Attachment #486897 - Flags: review?(dao) → review+
Attachment #486897 - Flags: approval2.0?
(In reply to comment #291)
> Sigh ... Looks like I can't even cut and paste correctly.

Do anything a thousand times and you'll get better at it, but will eventually not notice the 1001th time may be an error. ;)

Thanks for your hard work on this patch!
(In reply to comment #251)
> >this was not made a a design decision that menus should not have
> >icon, it was in fact, a performance improvement suggestion that since menus
> >open faster without icons
> 
> There's a human performance aspect here as well, if you only display icons for
> the highly used items, they are easier for the user to spot (compared to using
> icons for all of the items), so users are more likely to be able to quickly
> find the target they are looking for.  For instance on windows the context menu
> of the recycle bin places an icon next to "empty recycle bin" but not on any of
> the other items in the menu.  This visual search optimization is why we only
> included icons for some of the items in the Firefox menu.

The patch for bug 604650 results in icons only being displayed for menuitems with the menuitem-iconic-tooltip class instead of for all menutimems
Blocks: 608555
I landed the part which moves the menu popup code to browser-appmenu.inc, as this eases dealing with the code regardless of this bug:

http://hg.mozilla.org/mozilla-central/rev/443276883c2d
Should I attach a new patch minus the landed part carrying reviews forward?
Now that I see bug 608717, I will put a new patch here omitting both the already landed part, and the part I end up moving to bug 608717, and make this bug dependent on that one.
I will ask for review once this passes my tests.
Attachment #486897 - Attachment is obsolete: true
Attachment #486899 - Attachment is obsolete: true
Attachment #486897 - Flags: approval2.0?
Attachment #487459 - Attachment description: patch v14-remove coda allready landed and that split off with bug 608717 → patch v14-remove code already landed and that split off with bug 608717
This should be closer.
Attachment #487459 - Attachment is obsolete: true
Depends on: 608717
Once again, carrying faaborg's ui-review+ forward.
Attachment #487462 - Attachment is obsolete: true
Attachment #487502 - Flags: review?(dao)
Attachment #487502 - Attachment is obsolete: true
Attachment #487502 - Flags: review?(dao)
Attachment #487512 - Flags: ui-review+
Attachment #487512 - Flags: review?(dao)
Attachment #484618 - Attachment is obsolete: true
Attachment #484612 - Attachment is obsolete: true
Attachment #484617 - Attachment is obsolete: true
Comment on attachment 487512 [details] [diff] [review]
patch v16-update to merge with latest bug 608717 patch

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css

>+#appmenu-toolbar-button > .toolbarbutton-text {
>+  display: -moz-box;
>+}
> #appmenu_offlineModeRecovery:not([checked=true]) {
>   display: none;
> }

Can you add the appropriate ifdefs here?

>--- a/toolkit/themes/gnomestripe/global/global.css
>+++ b/toolkit/themes/gnomestripe/global/global.css

>-toolbar[type="menubar"]:not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
>+toolbar[type="menubar"]:not([autohide="true"]):not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
>   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
> }

This will need to merge with bug 608522.
(In reply to comment #303)
> Comment on attachment 487512 [details] [diff] [review]
> patch v16-update to merge with latest bug 608717 patch
> 
> >--- a/browser/base/content/browser.css
> >+++ b/browser/base/content/browser.css
> 
> >+#appmenu-toolbar-button > .toolbarbutton-text {
> >+  display: -moz-box;
> >+}
> > #appmenu_offlineModeRecovery:not([checked=true]) {
> >   display: none;
> > }
> 
> Can you add the appropriate ifdefs here?

Sure!

> 
> >--- a/toolkit/themes/gnomestripe/global/global.css
> >+++ b/toolkit/themes/gnomestripe/global/global.css
> 
> >-toolbar[type="menubar"]:not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
> >+toolbar[type="menubar"]:not([autohide="true"]):not(-moz-lwtheme):-moz-system-metric(menubar-drag) {
> >   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
> > }
> 
> This will need to merge with bug 608522.

OK.  I'll post a new patch tonight.
Depends on: 608522
Attached patch patch v17;ui-review=faaborg (obsolete) — Splinter Review
Attachment #487512 - Attachment is obsolete: true
Attachment #487791 - Flags: review?(dao)
Attachment #487512 - Flags: review?(dao)
Attachment #487791 - Attachment description: patch v16;ui-review=faaborg → patch v17;ui-review=faaborg
Comment on attachment 487791 [details] [diff] [review]
patch v17;ui-review=faaborg

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css
>@@ -140,16 +140,23 @@ toolbar[mode="icons"] > #reload-button[d
> }
> 
> .menuitem-iconic-tooltip,
> .menuitem-tooltip[type="checkbox"],
> .menuitem-tooltip[type="radio"] {
>   -moz-binding: url("chrome://browser/content/urlbarBindings.xml#menuitem-iconic-tooltip");
> }
> 
>+%ifdef MENUBAR_CAN_AUTOHIDE
>+%ifndef CAN_DRAW_IN_TITLEBAR
>+#appmenu-toolbar-button > .toolbarbutton-text {
>+  display: -moz-box;
>+}
>+%endif
>+%endif
> #appmenu_offlineModeRecovery:not([checked=true]) {
>   display: none;
> }

Extend MENUBAR_CAN_AUTOHIDE to include #appmenu_offlineModeRecovery, add a blank line before #appmenu_offlineModeRecovery.
Attachment #487791 - Flags: review?(dao) → review+
Thanks for the review Dão.

Carrying reviews forward.  Looking for approval to land this and associated patches as soon as the trunk opens for beta8 to maximize the bake time on nightlies before this goes out in a beta.
Attachment #487791 - Attachment is obsolete: true
Attachment #487892 - Flags: ui-review+
Attachment #487892 - Flags: review+
Attachment #487892 - Flags: approval2.0?
Isn't beta 7 the feature freeze? If so, it should land for beta 7?
(In reply to comment #308)
> Isn't beta 7 the feature freeze? If so, it should land for beta 7?

Beta7 is done.  There will be no more bugs accepted for inclusion in beta 7 except for serious regressions.
(In reply to comment #308)
> Isn't beta 7 the feature freeze? If so, it should land for beta 7?

A strings patch has already landed in this bug (a cheat to get around freezes) and the feature in question has been available on Windows for quite some time. "Feature freeze" means no new features, not no porting to other OSes. This would have to land for a beta, but not necessarily 7, hopefully 8 at this pace.
Ok :) Thanks for the clarification.
Attachment #487892 - Attachment is obsolete: true
Attachment #490053 - Flags: approval2.0?
Attachment #487892 - Flags: approval2.0?
using the latest build on Bill's server i have noticed that Firefox still alows the user to untick the preference that always shows the tab bar.

Then, having the menu button sitting in the tab bar instead of the toolbar let the user without any menu (menu bar nor menu button) if the preference is unticked and the user only has a tab.

Maybe the menu button should sit it the toolbar ?
(In reply to comment #314)
> using the latest build on Bill's server i have noticed that Firefox still alows
> the user to untick the preference that always shows the tab bar.

That should be going away at some point in bug 594614.
Has the visual appearance / interaction of this patch changed since faaborg ui-r+'d the screenshot of patch v9 (attachment 484611 [details])? I see the latest patch is v.19. I'm hesitant to a+ this until that's clear.
Attachment #485643 - Attachment description: screenshot showing new button on tabbar → screenshot showing new button on tabbar - patch v10
(In reply to comment #316)
> Has the visual appearance / interaction of this patch changed since faaborg
> ui-r+'d the screenshot of patch v9 (attachment 484611 [details])? I see the latest patch
> is v.19. I'm hesitant to a+ this until that's clear.

The only major visual appearance / interaction since v9 was the change form an icon for the firefox button, to a text button which was done in the v10 patch.  faaborg approved that in comment 289.

I am attaching a screenshot similar to that shown for the v9 patch to show that is the only change.

If you want, I can ask faaborg for one last ui-review.
Attachment #492048 - Flags: ui-review?(faaborg)
(In reply to comment #317)
> (In reply to comment #316)
> > Has the visual appearance / interaction of this patch changed since faaborg
> > ui-r+'d the screenshot of patch v9 (attachment 484611 [details] [details])? I see the latest patch
> > is v.19. I'm hesitant to a+ this until that's clear.
> 
> The only major visual appearance / interaction since v9 was the change form an
> icon for the firefox button, to a text button which was done in the v10 patch. 
> faaborg approved that in comment 289.
                           ^^^^^^^^^^^

I meant comment 283.
Attachment #492048 - Flags: ui-review?(faaborg) → ui-review+
Attachment #490053 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/0601ba3e9395
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The landing of this bug broke the mozmill suite on mozilla-central.  Because we do not have a clear policy on how to deal with these types of failures, and we don't run mozmill tests on the try server, I just filed bug 613802 for the failure, and hid the Linux mozmill boxes on mozilla-central, and let the patch remain on trunk.
This also caused a 2.92% Txul regression on Linux:

Regression :( Txul increase 2.92% on Linux Firefox
--------------------------------------------------
    Previous: avg 93.972 stddev 0.501 of 30 runs up to revision 8af7288622ea
    New     : avg 96.716 stddev 0.177 of 5 runs since revision 0601ba3e9395
    Change  : +2.744 (2.92% / z=5.475)
    Graph   : http://mzl.la/aHwNFc

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8af7288622ea&tochange=0601ba3e9395
(In reply to comment #321)
> The landing of this bug broke the mozmill suite on mozilla-central.  Because we
> do not have a clear policy on how to deal with these types of failures, and we
> don't run mozmill tests on the try server, I just filed bug 613802 for the
> failure, and hid the Linux mozmill boxes on mozilla-central, and let the patch
> remain on trunk.

This is surprising.  It will be interesting to find out what happened here.
(In reply to comment #322)
> This also caused a 2.92% Txul regression on Linux:
> 
> Regression :( Txul increase 2.92% on Linux Firefox
> --------------------------------------------------
>     Previous: avg 93.972 stddev 0.501 of 30 runs up to revision 8af7288622ea
>     New     : avg 96.716 stddev 0.177 of 5 runs since revision 0601ba3e9395
>     Change  : +2.744 (2.92% / z=5.475)
>     Graph   : http://mzl.la/aHwNFc
> 
> Changeset range:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8af7288622ea&tochange=0601ba3e9395

This is disturbing.  Especially since, by default, the old menu is displayed.  Was there a similar Windows Txul regression when the new menu and menubar autohide landed under Windows?
The button is added to the very right of the tab bar, and it doesn't seem to be movable via the customize dialog. How can I move it to the very left?
(In reply to comment #324)
> Was there a similar Windows Txul regression when the new menu and menubar
> autohide landed under Windows?

I don't think so.

(In reply to comment #325)
> The button is added to the very right of the tab bar, and it doesn't seem to be
> movable via the customize dialog.

This happens if you customized your toolbars in a previous 4.0 beta / nightly.

> How can I move it to the very left?

"Restore default set" should do it.
(In reply to comment #326)
> "Restore default set" should do it.

Worked, thanks.
(In reply to comment #326)
> (In reply to comment #324)
> > Was there a similar Windows Txul regression when the new menu and menubar
> > autohide landed under Windows?
> 
> I don't think so.

Bug 575093 says otherwise.
Bug 575093 says "25% TXul regression from hiding the menu bar". We're not hiding the menu bar on Linux -- not by default.
I pushed this to the try server to see if it helps txul, hoping "-t chrome" makes it run txul: http://hg.mozilla.org/try/rev/7413f7c67643
Depends on: 613859
(In reply to comment #324)
> This is disturbing.  Especially since, by default, the old menu is displayed. 
> Was there a similar Windows Txul regression when the new menu and menubar
> autohide landed under Windows?

We had a couple regressions on windows, one from some unnecessary xpcom overhead in the widget patches, and another from an extra reflow that was triggered. The total regression was around 5%, 3% xpcom, 2% reflow.
Depends on: 613872
Maybe someone wants to have a look at bug #613880... this way submenus are
positioned looks bad on this new menu.
Depends on: 613911
(In reply to comment #330)
> I pushed this to the try server to see if it helps txul, hoping "-t chrome"
> makes it run txul: http://hg.mozilla.org/try/rev/7413f7c67643

Didn't help, as far as I could interpret the numbers.
Depends on: 613880
No longer depends on: 613880
(In reply to comment #325)
> The button is added to the very right of the tab bar, and it doesn't seem to be
> movable via the customize dialog. How can I move it to the very left?

Funny enough, I like the button so much better on the right side. Is there a hack to always have it there? I will have to press reset to default sooner or later...
(In reply to comment #334)
> Funny enough, I like the button so much better on the right side. Is there a
> hack to always have it there? I will have to press reset to default sooner or
> later...

The easiest way to move it would be to make some customization on the Tabs toolbar then exit the browser and edit the localstore.rdf file.  In this file, find the TabsToolbar RDF entry and edit the currentset by inserting/moving "appmenu-toolbar-button" at the desired location.
Depends on: 615056
No longer depends on: 615056
No longer depends on: 613859
For the record, note that the decision to keep menu bar by default on Linux (i don't criticize that decision) makes bug 606744#c6 very sensible on Linux  : panorama button moves up and down depending if Panorama mode is ON or OFF with menu bar displayed.
gtk3 has been out for a while and it would be desirable if ff4 or a closer update to ff4 could handle drawing the ff button on the title bar
Depends on: 645700
Verified the presence of the Firefox button with Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Though this bug has already been marked as fixed, I have something to add: the menu button on KDE.

Don't know if it has been addressed anywhere else but I haven't seen this here except for the comment of Dave Garrett. 

It *is* already possible to draw the button on the windows decoration. It seems to be that it's planned for KDE 4.8 but it's already here:

http://kde-look.org/content/show.php?content=141254&forumpage=0

And it looks like this:
http://img88.imageshack.us/i/menubtn3.jpg/

The only problem is that when it's drop down, it has the classic manu inside:
"File>
 Edit>
 etc>.."

(I can't take a screenshot right now... it doesn't let me when I have the menu pressed)

But, if there's a addon or something, special for KDE, that show the classic menu as the "new menu", so it will draw the new menu at the end.

Just an option.
(In reply to comment #339)
> Though this bug has already been marked as fixed, I have something to add:
> the menu button on KDE.
> 
> Don't know if it has been addressed anywhere else but I haven't seen this
> here except for the comment of Dave Garrett.

Did you just mean mention of KDE or something else specific?

> (I can't take a screenshot right now... it doesn't let me when I have the
> menu pressed)

I listed the steps to do that under KDE up in comment 132.

> But, if there's a addon or something, special for KDE, that show the classic
> menu as the "new menu", so it will draw the new menu at the end.

Nobody here is likely to write an addon for this, and while I do personally prefer KDE, Firefox is a GTK application (though not GNOME; that distinction needs to be made). The KDE stuffs always gets the back seat, unfortunately, though I can't really disagree with that being the case. When drawing the Firefox button in the title bar is done on Linux I would expect it to be done in a cross-DE way via GTK features (which was discussed above somewhere).

In any case, this bug is long and winding and done. Whomever starts work on this will do so in a new bug.
@Francisco I like the new KDE menu and I guess the classic menu this way is a welcome change. But yeah I agree with Dave, KDE gets a backseat most of the times.
> Did you just mean mention of KDE or something else specific?

Sorry. Yes, I was referring KDE. 


> I listed the steps to do that under KDE up in comment 132.

Ok. Here's the screenshot
http://www.imagengratis.org/images/ffmenu.png

(the most embarrassing part is that I had already read comment 132...)
 
> Nobody here is likely to write an addon for this, and while I do personally
> prefer KDE, Firefox is a GTK application (though not GNOME; that distinction
> needs to be made). The KDE stuffs always gets the back seat, unfortunately,
> though I can't really disagree with that being the case. When drawing the
> Firefox button in the title bar is done on Linux I would expect it to be
> done in a cross-DE way via GTK features (which was discussed above
> somewhere).
> 
> In any case, this bug is long and winding and done. Whomever starts work on
> this will do so in a new bug.

Well, after reading what I wrote about a new "addon" I realized it was quite silly from me. I was referring that maybe an addon would be enough for getting it work.

I understand your point. It's just I followed this thread when it first appeared and the "GTK2 can't draw buttons in the title bar" last year or so. 

When I knew that (at least in KDE) it was already possible to draw the button in the title bar, I remembered this thread and tried to add something. And it's a pitty, because it's possible to put tabs con title bar also. But well, I understand that FF is s GTK app. I just tried to add more information.

Regards
You need to log in before you can comment on or make changes to this bug.