Closed Bug 863753 Opened 7 years ago Closed 7 years ago

Retire Firefox button

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: addon-compat, user-doc-needed, Whiteboard: [Australis:M4])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 863780
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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?
Attachment #739699 - Flags: feedback?(gavin.sharp)
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?
Attachment #739699 - Flags: feedback?(jaws)
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.
Attachment #739699 - Flags: feedback?(gavin.sharp)
Attached patch Patch v1 (obsolete) — Splinter Review
De-bitrotted for Jamun branch.

Jared, if you're overloaded on reviews, let me know, and I'll find someone else to take this.
Attachment #739699 - Attachment is obsolete: true
Attachment #739699 - Flags: feedback?(jaws)
Attachment #742003 - Flags: review?(jaws)
Comment on attachment 742003 [details] [diff] [review]
Patch v1

Matt, care to take a peek at this?
Attachment #742003 - Flags: review?(jaws) → review?(mnoorenberghe+bmo)
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Adding addon-compat since extensions and themes will need to be updated to reflect the fact that the appmenu will be removed.
Keywords: addon-compat
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.
Attachment #742003 - Flags: review?(mnoorenberghe+bmo)
Attachment #742003 - Flags: review-
Attachment #742003 - Flags: feedback?(dao)
(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).
Attachment #742003 - Flags: feedback?(dao)
Whiteboard: [Australis:M4]
Attached patch Patch v1.1Splinter Review
(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.
Attachment #742003 - Attachment is obsolete: true
(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.
Attachment #745194 - Flags: review?(mnoorenberghe+bmo)
Attachment #745194 - Flags: review?(dao)
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.
Attachment #745194 - Flags: review?(mnoorenberghe+bmo) → review+
Comment on attachment 745194 [details] [diff] [review]
Patch v1.1

Thanks Matt!
Attachment #745194 - Flags: review?(dao)
Jamun: https://hg.mozilla.org/projects/jamun/rev/3b1bc4a63d32
UX: https://hg.mozilla.org/projects/ux/rev/3b1bc4a63d32
Whiteboard: [Australis:M4] → [Australis:M4][fixed-in-jamun][fixed-in-ux]
Depends on: 870634
Depends on: 910657
Blocks: 770316
Depends on: 916908
https://hg.mozilla.org/mozilla-central/rev/3b1bc4a63d32
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M4][fixed-in-jamun][fixed-in-ux] → [Australis:M4]
Target Milestone: --- → Firefox 28
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?
Blocks: 837547
You need to log in before you can comment on or make changes to this bug.