Part of the Australis UI update is to retire the Firefox button in preference of the menu button which will be at the end of the nav-bar. This is similar to bug 840697, but this is not a UX-only work. This is for eventual landing on m-c.

This does the job on Windows. Haven't tried Linux yet.

Comment on attachment 739699 [details] [diff] [review] WIP Patch 1 Not sure if you're the right one to look at this Gavin, but am I hitting all of the right spots here to remove this? Anything big I'm missing?

Comment on attachment 739699 [details] [diff] [review] WIP Patch 1 Or Jared? Is there anything big I'm forgetting here? Or should I just go ahead and request review?

Comment on attachment 739699 [details] [diff] [review] WIP Patch 1 Sorry for the delayed response - I don't really have any recollection of any tricky bits related to the Firefox button that might not be covered by this, and haven't had the time to dig in and look into it in detail. Also this patch doesn't apply on m-c (I guess it's against UX?), and I don't have a local UX tree handy.

De-bitrotted for Jamun branch. Jared, if you're overloaded on reviews, let me know, and I'll find someone else to take this.

Comment on attachment 742003 [details] [diff] [review] Patch v1 Matt, care to take a peek at this?

Adding addon-compat since extensions and themes will need to be updated to reflect the fact that the appmenu will be removed.

Comment on attachment 742003 [details] [diff] [review] Patch v1 Review of attachment 742003 [details] [diff] [review]: ----------------------------------------------------------------- I think you are missing some appmenu references in browser/base/content/browser.css which should be removed[1] unless you're using them for the new menu? [1] https://mxr.mozilla.org/mozilla-central/search?string=appmenu&find=content%2Fbrowser.css After the removal there I think there will only be one usage of MENUBAR_CAN_AUTOHIDE and then the definition. It's probably fine to keep around since I don't have a cleaner alternative. Suggestions welcome. I'll need to take a closer look for v.2 f?(dao) on the splitmenu removal ::: browser/base/content/browser.js @@ -1270,5 @@ > - if (event.target != appMenuPopup || !appMenuOpening) > - return; > - let duration = new Date() - appMenuOpening; > - appMenuOpening = null; > - Services.telemetry.getHistogramById("FX_APP_MENU_OPEN_MS").add(duration); File a follow-up to have the probe added to the new app menu. ::: browser/base/content/browser.xul @@ -518,5 @@ > #include browser-menubar.inc > </toolbaritem> > > #ifdef CAN_DRAW_IN_TITLEBAR > - <hbox class="titlebar-placeholder" type="appmenu-button" ordinal="0"/> Won't we need this for OS X tabs in titlebar? The @type value is not ideal for that case though. It seems like implementing that bug first would help to know how it would work. @@ -826,5 @@ > label="&closeTab.label;" > tooltiptext="&closeTab.label;"/> > > #ifdef CAN_DRAW_IN_TITLEBAR > - <hbox class="titlebar-placeholder" type="appmenu-button" ordinal="0"/> ditto ::: browser/locales/en-US/chrome/browser/browser.dtd @@ -316,5 @@ > -<!ENTITY appMenuToolbarLayout.label "Toolbar Layout…"> > -<!ENTITY appMenuSidebars.label "Sidebars"> > -<!ENTITY appMenuFind.label "Find…"> > -<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks"> > -<!ENTITY appMenuWebDeveloper.label "Web Developer"> Shouldn't we also keep the Web Developer item for the new app menu? @@ -318,5 @@ > -<!ENTITY appMenuFind.label "Find…"> > -<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks"> > -<!ENTITY appMenuWebDeveloper.label "Web Developer"> > -<!ENTITY appMenuGettingStarted.label "Getting Started"> > -<!ENTITY appMenuSafeMode.label "Restart with Add-ons Disabled…"> Somewhat off-topic: where will things like these help items go on Windows after the Firefox button is removed? ::: browser/themes/windows/browser-aero.css @@ -409,5 @@ > } > - > -/* ::::: splitmenu highlight style that imitates Windows 7 start menu ::::: */ > -@media (-moz-windows-default-theme) { > - .splitmenu-menuitem, It seems like you should also remove the binding from urlbarBindings.xml along with the CSS for the binding from browser/base/content/browser.css unless this used for other UI? I don't see other uses in Firefox and at quick glance, most add-on usage seems to be for the app menu. Bug 613156 comment 12 also said this wasn't ready for other consumers although that may have changed. I'd like Dão's opinion and I'll mark the bug as addon-compat. ::: browser/themes/windows/browser.css @@ +2590,1 @@ > background-image: url("chrome://browser/skin/privatebrowsing-dark.png"); The Firefox button is currently the only indicator that the user is in private browsing mode when the menubar is hidden so this patch will make it indistinguishable. Do we know what the plan is for a private browsing indicator for the Australis release? ::: browser/themes/windows/jar.mn @@ +19,5 @@ > #ifdef MOZ_SERVICES_SYNC > skin/classic/browser/aboutSyncTabs.css > #endif > skin/classic/browser/actionicon-tab.png > skin/classic/browser/appmenu.png For those following-along: appmenu.png is the new appmenu icon for the navbar.

(In reply to Matthew N. [:MattN] from comment #8) > I don't see other uses in Firefox and at > quick glance, most add-on usage seems to be for the app menu. Bug 613156 > comment 12 also said this wasn't ready for other consumers although that may > have changed. It's still true. > For those following-along: appmenu.png is the new appmenu icon for the > navbar. This should be part of Toolbar.png (and Toolbar-inverted.png).

(In reply to Matthew N. [:MattN] from comment #8) > Comment on attachment 742003 [details] [diff] [review] > Patch v1 > > Review of attachment 742003 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think you are missing some appmenu references in > browser/base/content/browser.css which should be removed[1] unless you're > using them for the new menu? > > [1] > https://mxr.mozilla.org/mozilla-central/ > search?string=appmenu&find=content%2Fbrowser.css > Ah, good catch - removed. > After the removal there I think there will only be one usage of > MENUBAR_CAN_AUTOHIDE and then the definition. It's probably fine to keep > around since I don't have a cleaner alternative. Suggestions welcome. > I don't have a problem keeping it around. > I'll need to take a closer look for v.2 > > f?(dao) on the splitmenu removal > > ::: browser/base/content/browser.js > @@ -1270,5 @@ > > - if (event.target != appMenuPopup || !appMenuOpening) > > - return; > > - let duration = new Date() - appMenuOpening; > > - appMenuOpening = null; > > - Services.telemetry.getHistogramById("FX_APP_MENU_OPEN_MS").add(duration); > > File a follow-up to have the probe added to the new app menu. > Will do, right after I post this comment. > ::: browser/base/content/browser.xul > @@ -518,5 @@ > > #include browser-menubar.inc > > </toolbaritem> > > > > #ifdef CAN_DRAW_IN_TITLEBAR > > - <hbox class="titlebar-placeholder" type="appmenu-button" ordinal="0"/> > > Won't we need this for OS X tabs in titlebar? The @type value is not ideal > for that case though. It seems like implementing that bug first would help > to know how it would work. This has been solved in bug 865374, as OSX gets the ordinal values set differently, and has a titlebar-placeholder for the fullscreen button. > > @@ -826,5 @@ > > label="&closeTab.label;" > > tooltiptext="&closeTab.label;"/> > > > > #ifdef CAN_DRAW_IN_TITLEBAR > > - <hbox class="titlebar-placeholder" type="appmenu-button" ordinal="0"/> > > ditto Ditto! :) > > ::: browser/locales/en-US/chrome/browser/browser.dtd > @@ -316,5 @@ > > -<!ENTITY appMenuToolbarLayout.label "Toolbar Layout…"> > > -<!ENTITY appMenuSidebars.label "Sidebars"> > > -<!ENTITY appMenuFind.label "Find…"> > > -<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks"> > > -<!ENTITY appMenuWebDeveloper.label "Web Developer"> > > Shouldn't we also keep the Web Developer item for the new app menu? > The new app menu button will likely be implemented via the new widget API, which will need to use strings from a .properties file for localization, so I think we're OK removing these. > @@ -318,5 @@ > > -<!ENTITY appMenuFind.label "Find…"> > > -<!ENTITY appMenuUnsorted.label "Unsorted Bookmarks"> > > -<!ENTITY appMenuWebDeveloper.label "Web Developer"> > > -<!ENTITY appMenuGettingStarted.label "Getting Started"> > > -<!ENTITY appMenuSafeMode.label "Restart with Add-ons Disabled…"> > > Somewhat off-topic: where will things like these help items go on Windows > after the Firefox button is removed? > I believe the "Help" button in the panel menu will either open a subview showing those items, or something else. I need to follow-up with the UX team on that. > ::: browser/themes/windows/browser-aero.css > @@ -409,5 @@ > > } > > - > > -/* ::::: splitmenu highlight style that imitates Windows 7 start menu ::::: */ > > -@media (-moz-windows-default-theme) { > > - .splitmenu-menuitem, > > It seems like you should also remove the binding from urlbarBindings.xml > along with the CSS for the binding from browser/base/content/browser.css > unless this used for other UI? I don't see other uses in Firefox and at > quick glance, most add-on usage seems to be for the app menu. Bug 613156 > comment 12 also said this wasn't ready for other consumers although that may > have changed. I'd like Dão's opinion and I'll mark the bug as addon-compat. > Ok, I've removed the binding. > ::: browser/themes/windows/browser.css > @@ +2590,1 @@ > > background-image: url("chrome://browser/skin/privatebrowsing-dark.png"); > > The Firefox button is currently the only indicator that the user is in > private browsing mode when the menubar is hidden so this patch will make it > indistinguishable. Do we know what the plan is for a private browsing > indicator for the Australis release? > As was mentioned in the Australis weekly meeting, we're OK for now removing this sole indicator of PB mode, and we'll have a solution to this soon (likely putting an indicator in the titlebar at a bare minimum). > ::: browser/themes/windows/jar.mn > @@ +19,5 @@ > > #ifdef MOZ_SERVICES_SYNC > > skin/classic/browser/aboutSyncTabs.css > > #endif > > skin/classic/browser/actionicon-tab.png > > skin/classic/browser/appmenu.png > > For those following-along: appmenu.png is the new appmenu icon for the > navbar.

(In reply to Dão Gottwald [:dao] from comment #9) > > For those following-along: appmenu.png is the new appmenu icon for the > > navbar. > > This should be part of Toolbar.png (and Toolbar-inverted.png). I'll file a follow-up bug for this as well.

Filed follow-ups from those last few comments - bug 868462 and bug 868464.

Comment on attachment 745194 [details] [diff] [review] Patch v1.1 Review of attachment 745194 [details] [diff] [review]: ----------------------------------------------------------------- r=me I filed bug 868636 for the help menu and bug 868640 for the private browsing indicator.

Comment on attachment 745194 [details] [diff] [review] Patch v1.1 Thanks Matt!

Jamun: https://hg.mozilla.org/projects/jamun/rev/3b1bc4a63d32 UX: https://hg.mozilla.org/projects/ux/rev/3b1bc4a63d32

All I miss from this new UI is movable "new menu button". I would prefer it to be on the left side, as Firefox menu was by default. Could you make it possible to drag it and move as other objects in customization mode?