324180 - make Firefox Application menu work under Cocoa widgets

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Josh Aas]

Cocoa widgets has a different way of building the Application menu on Mac OS X than Carbon widgets do. See bug 316076, in particular, comment 53:

https://bugzilla.mozilla.org/show_bug.cgi?id=316076#c53

We need to add support in Firefox for the Application menu under Cocoa widgets.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Jo Hermans]

Would this take into account bug 288322  ?

Comment 3

[Josh Aas]

It will not handle 288322. Please read the comment listed in the summary for this bug if you want to know how the new system works. We will handle the issue in 288322 later.

Comment 4

[Mark Mentovai]

Comment on attachment 209142 [details] [diff] [review]
fix v1.0

>Index: base/content/baseMenuOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/baseMenuOverlay.xul,v
>retrieving revision 1.11
>diff -u -r1.11 baseMenuOverlay.xul
>--- base/content/baseMenuOverlay.xul	26 Jul 2005 23:21:13 -0000	1.11
>+++ base/content/baseMenuOverlay.xul	20 Jan 2006 22:00:06 -0000
>@@ -51,9 +51,13 @@
> <script type="application/x-javascript" src="chrome://help/content/contextHelp.js"/>
> 
> #ifdef XP_MACOSX
>-<!-- nsMenuBarX moves menu_preferences to the Application Menu -->
>+<!-- nsMenuBarX hides these and uses them to build the Application menu -->
>     <menupopup id="menu_ToolsPopup">
>-        <menuitem id="menu_preferences" oncommand="openPreferences();"/>
>+        <menuitem id="menu_preferences" label="&preferencesCmdMac.label;" key="key_openMacPreferences" oncommand="openPreferences();"/>
>+        <menuitem id="menu_mac_services" label="&servicesMenuMac.label;"/>
>+        <menuitem id="menu_mac_hide_app" label="&hideThisAppCmdMac.label;" key="key_hideThisAppMac"/>
>+        <menuitem id="menu_mac_hide_others" label="&hideOtherAppsCmdMac.label;" key="key_hideOtherAppsMac"/>
>+        <menuitem id="menu_mac_show_all" label="&showAllAppsCmdMac.label;"/>

Adding these nodes doesn't have any noticeable impact when Carbon widgets are in use, right?

>@@ -123,6 +127,16 @@
>         <key id="key_openHelpMacFrontend"
>              key="&openHelpMac2.frontendCommandkey;"
>              modifiers="&openHelpMac2.frontendModifiers;"/>
>+# These are used to build the Application menu under Cocoa widgets

That's not an XML comment!

>Index: locales/en-US/chrome/browser/baseMenuOverlay.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd,v
>retrieving revision 1.6
>diff -u -r1.6 baseMenuOverlay.dtd
>--- locales/en-US/chrome/browser/baseMenuOverlay.dtd	14 Sep 2005 23:58:05 -0000	1.6
>+++ locales/en-US/chrome/browser/baseMenuOverlay.dtd	20 Jan 2006 22:00:07 -0000
>@@ -31,3 +31,19 @@
> <!ENTITY helpReleaseNotes.accesskey     "N">
> <!ENTITY updateCmd.label                "Check for Updates...">
> <!ENTITY updateCmd.accesskey            "o">
>+
>+<!ENTITY preferencesCmdMac.label        "Preferences...">

We don't have &hellip; available to us here, but maybe we should.  Map it to "&#x2026;" on the Mac and other platforms where it's the right thing to do, and "..." elsewhere.  I'm thinking that the three periods might negatively imapct external a10y tools.  That would be another bug, this is fine for now.

>+<!ENTITY openMacPreferences.commandkey  ",">
>+<!ENTITY openMacPreferences.modifiers   "accel">

Any reason why you switched from preferencesCmdMac to openMacPreferences?  You didn't do this below.

>+<!ENTITY hideThisAppCmdMac.label        "Hide &brandShortName;">
>+<!ENTITY hideThisAppMac.commandkey      "H">
>+<!ENTITY hideThisAppMac.modifiers       "accel">

Any reason you dropped the Cmd?

Comment 5

[Benjamin Smedberg]

Comment on attachment 209142 [details] [diff] [review]
fix v1.0

>Index: locales/en-US/chrome/browser/baseMenuOverlay.dtd

>+<!ENTITY preferencesCmdMac.label        "Preferences...">
>+<!ENTITY openMacPreferences.commandkey  ",">
>+<!ENTITY openMacPreferences.modifiers   "accel">

Please use matching prefixes for these and the ones below, although I'm wondering why we're making the commandkey/modifier localizable in the first place (isn't it command-, in all languages? command keys generally shouldn't be localizable).

>+<!ENTITY quitApplicationCmdMac.label    "Quit">
>+<!ENTITY quitApplicationCmdMac.key      "Q">

please use "accesskey", various automated l10n tools use special .accesskey semantics to make sure the accesskey matches something in the label (or is this a commandkey?... the entity should make that clear.

Comment 6

[Josh Aas]

Attachment: fix v1.1

Comment 7

[Josh Aas]

BTW - this application menu system is fully backwards-compatible with Carbon widgets. We only really need to look out for certain DOM node IDs, which have been preserved in cases where they did exist and in cases where we are just adding DOM nodes, it doesn't matter what they are called.

Comment 8

[Mark Mentovai]

Comment on attachment 209735 [details] [diff] [review]
fix v1.1

Sorry for the delay.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 209735 [details] [diff] [review]
fix v1.1


>-# Show IE Users menu item on Windows only
>+<!-- Show IE Users menu item on Windows only -->
> #ifdef XP_WIN

this should be a preprocessor comment still, it explains the preprocessing...

>         <menuitem label="&helpForIEUsers.label;"
>                   accesskey="&helpForIEUsers.accesskey;"
>@@ -110,12 +115,11 @@
>     <keyset id="baseMenuKeyset">
>         <key id="key_openHelp"
>             oncommand="openHelp('firefox-help', 'chrome://browser/locale/help/help.rdf');"
>-# VK_HELP doesn't work on other platforms yet
> #ifdef XP_MACOSX
>             keycode="&openHelpMac.commandkey;"/>

don't remove this comment, unless VK_HELP is actually fixed to work elsewhere.

>Index: base/content/browser-menubar.inc
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-menubar.inc,v
>retrieving revision 1.70
>diff -u -r1.70 browser-menubar.inc
>--- base/content/browser-menubar.inc	17 Jan 2006 20:34:39 -0000	1.70
>+++ base/content/browser-menubar.inc	26 Jan 2006 15:46:33 -0000
>@@ -72,6 +72,9 @@
> #ifdef XP_WIN
> 			  label="&quitApplicationCmdWin.label;"
> 			  accesskey="&quitApplicationCmdWin.accesskey;"
>+#elif XP_MACOSX
>+        label="&quitApplicationCmdMac.label;"
>+        key="key_quitApplicationCmdMac"
> #else

er, indentation?  looks like tabs in the previous block, please fix that

>+<!ENTITY preferencesCmdMac.label        "Preferences...">
>+<!ENTITY preferencesCmdMac.commandkey  ",">
>+<!ENTITY preferencesCmdMac.modifiers   "accel">

oddball indent

>+<!ENTITY servicesMenuMac.label          "Services">
>+
>+<!ENTITY hideThisAppCmdMac.label        "Hide &brandShortName;">
>+<!ENTITY hideThisAppCmdMac.commandkey   "H">
>+<!ENTITY hideThisAppCmdMac.modifiers    "accel">
>+
>+<!ENTITY hideOtherAppsCmdMac.label      "Hide Others">
>+<!ENTITY hideOtherAppsCmdMac.commandkey "H">
>+<!ENTITY hideOtherAppsCmdMac.modifiers  "accel,shift">
>+
>+<!ENTITY showAllAppsCmdMac.label        "Show All">

I'm not convinced these shouldn't be in toolkit or somewhere non-app-specific, but that's really a followup  (base toolkit menu stuff for Mac)

Comment 10

[Josh Aas]

fixed on trunk

Comment 11

[Mike Connor [:mconnor]]

Is the other bug going onto the 1.8.1 branch? if not, this needs to be refactored with some #ifndef MOZILLA_1_8_BRANCH love.  Should have thought of that earlier....

Comment 12

[Asaf Romano (gone)]

(In reply to comment #11)
> Is the other bug going onto the 1.8.1 branch? if not, this needs to be
> refactored with some #ifndef MOZILLA_1_8_BRANCH love.  Should have thought of
> that earlier....
> 

Looking at the patch, the IDs of About/Preferences menuitem have not been changed, so this shouldn't be an issue.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 209735 [details] [diff] [review]
fix v1.1

Really, this should be either an INC or an overlay in toolkit/content.

Comment 14

[Wayne Woods]

OK, I'm reaching here... I don't suppose anything in these changes affect Carbon builds, do they? Something checked in around 20060201-20060202 caused trunk bug 325859, which does affect the Application menu, and I'm short on potential culprits ;)

Comment 15

[Josh Aas]

Wayne - I'm pretty sure the patch for this bug did that. I know what is wrong, I'll fix it.

Comment 16

[Marek Stępień [:marcoos, inactive]]

Comment on attachment 209735 [details] [diff] [review]
fix v1.1

This:

+<!ENTITY hideOtherAppsCmdMac.label      "Hide Others">
+<!ENTITY hideOtherAppsCmdMac.commandkey "H">
+<!ENTITY hideOtherAppsCmdMac.modifiers  "accel,shift">

sets the command keys to Cmd+Shift+H. While doing Polish l10n for this bug our Mac guru has pointed to me that this shortcut should be Cmd+Alt+H, as it is in Safari, Finder and other native Mac apps...

Comment 17

[Marek Stępień [:marcoos, inactive]]

Attachment: The <appname> menu in Mac OS X Finder

This is how it looks like in Finder.

Comment 18

[Josh Aas]

Reopening to fix that issue.

Comment 19

[Josh Aas]

Attachment: fix key modifier for "Hide Others" menu item, v1.0

Comment 20

[Josh Aas]

Fix checked in.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 209735 [details] [diff] [review]
fix v1.1

a+me for 1.8.1 in the spirit of keeping things in sync (along with followup patch to fix accel keys)

Comment 22

[Josh Aas]

Landed on MOZILLA_1_8_BRANCH with all follow-up and a comment. Should be in sync with the trunk now.

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 209735 [details] [diff] [review]
fix v1.1

So, can this changeset get wrapped in #ifndef MOZILLA_1_8_BRANCH preprocessor bits, so as make it explicit what is and isn't going to do anything on the branch?