68098 - Preferences and Quit should be in their own menu on Mac OS X

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Mike Pinkerton (not reading bugmail)]

under the new aqua ui guidelines, the application menu provided by the OS
contains the 'quit' command, not the file menu, and it should no longer be in
the file menu.

we need to conditionalize our code to remove the quit item from the file menu if
we detect we're on osx:

err = Gestalt(gestaltMenuMgrAttr,&result);
if (!err &&(result & gestaltMenuMgrAquaLayoutMask)) {
  // remove quit menu
  // remove separator
}

shouldn't be tough to do with some appropriate ID's on the right menu items.

Comment 1

[Thomas Deniau]

I think that only Carbon/Cocoa apps should do this. Mozilla is a Classic 

app, so it doesn't have the Aqua UI. Therefore, it doesn't have an 

Application menu. And we shouldn't do this test. This is only for Fizzilla, I 

think.

Comment 2

[Mike Pinkerton (not reading bugmail)]

fizzilla is a carbon app today and the daily builds will be carbon in a few 
weeks, so we need to do this.

Comment 3

[Steve Dagley]

And we need it in Mozilla 0.9

Comment 4

[timeless]

Does our menu system support css display:none or collapsed?   if so we could 
use css instead of hard coding this.  (I prefer per platform/os/ver css over 
c++ for stuff like this)

Comment 5

[Mike Pinkerton (not reading bugmail)]

sorry timeless. all mac menus are already display:none.

Comment 6

[Andrew Thompson]

<delurk>
Can't the overlay mechanism handle this in the same way it makes it Quit on Mac 
OS and Exit on Windows?

Perhaps the right fix is to fix overlays to have a value which says "don't 
include this item". Surely this must be possible as View->Text Size is removed on 
Mac OS. 

Of course, then we'd need a MacOSX directory in the chrome, and build system 
changes to handle it....

Oh and since we'll be running Fizilla on Mac OS 9 maybe it does have to be in C++

Oh well, its an idea, I leave it to more knowledgable people to shoot it down if 
it won't work.

</delurk>

Comment 7

[Mike Pinkerton (not reading bugmail)]

the way i plan on doing this is to tag the quit item with an id, and if we're 
running on osx (using the gestalt apple provides), remove that menu while 
building the menubar.

Comment 8

[Andrew Thompson]

Ok, I think the place to attempt this is 
NS_METHOD nsMenuX::AddMenuItem(nsIMenuItem * aMenuItem)

using the check pink mentions above. One thing though, if we're gonna do this by 
detecting ids, that could be quite tricky using nsIMenuItem which doesn't expose 
the   nsCOMPtr<nsIContent>      mContent; interface that would be needed to read 
the id, unless I'm missing the id you meant Mike?

Comment 9

[Andrew Thompson]

Searching lxr for menu_FilePopup is quite instructive. As expected the Quit item 
is added in the platform overlays. The seperator remains in each component 
individually though.

I'm tempted to move the seperator into the overlay on all platforms, then proceed 
as Mike suggests, give both ids and decline to add them if we detect we're 
running on Mac OS X. The part I still don't see is what I covered in my last 
comment ... if nsMenuX::AddItem is the place to make the change, how to find the 
id? 

Comment 10

[Mike Pinkerton (not reading bugmail)]

you have the dom node associated with the menu item, so you can QI it to 
nsIContent to get the info.

It might be better to do this in nsMenu, or better still in nsMenuBar, rather 
than making ever menu item check if it's a quit menu.

Comment 11

[Andrew Thompson]

Well, you changed to DOM node to a nsIContent anyway, but I was proposing to do 
it in nsMenu... that's my point, all I have at that point is the public interface 
nsIMenuItem...

Comment 12

[Andrew Thompson]

Attachment: Moves the seperator before Quit to the overlay, and adds ids to both

Comment 13

[zs98]

Doesn't the Preferences menu item need to be moved as well?

Comment 14

[Andrew Thompson]

Attachment: Got hit by XUL syntax changes. My tree is now updated. This should be right

Comment 15

[Mike Pinkerton (not reading bugmail)]

i was thinking more about this, and we should probably just do the following in 
nsMenuBar's create method:

- find the prefs item, the quit menu item, and the quit separator in the dom
- remove these nodes from the dom before we create menus

this way, we dont' have to do any checking while building ("are you the quit 
item? are you the quit item? are you the quit item?") and we get them out of 
the way in one fell swoop before any iteration over the dom to create menus.

how does that sound?

Comment 16

[Andrew Thompson]

I can't think of any reason not to do it that way. I don't know the APIs in 
question though. I'll try it if I get time.

Comment 17

[Andrew Thompson]

OK, here's a patch that does what Mike suggests I think. Since I haven't been 
able to get a Carbon build going I don't know if it even compiles properly, so 
I'm pasting it for discussion rather than attaching it:

Index: mozilla/widget/src/mac/nsMenuBarX.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/mac/nsMenuBarX.cpp,v
retrieving revision 1.6
diff -u -2 -r1.6 nsMenuBarX.cpp
--- mozilla/widget/src/mac/nsMenuBarX.cpp	2001/04/04 03:41:30	1.6
+++ mozilla/widget/src/mac/nsMenuBarX.cpp	2001/04/14 17:35:26
@@ -314,4 +314,56 @@
 {
   SetParent(aParent);
+  
+  //if this is Mac OS X, do a depth first left to right search across the 
menubar
+  //DOM and remove the Quit and Preferences nodes
+  SInt32 result;OSStatus err = Gestalt(gestaltMenuMgrAttr,&result);
+	if (!err &&(result & gestaltMenuMgrAquaLayoutMask)) {
+
+
+
+    nsCOMPtr<nsIDocument> containingDoc;
+    mMenuBarContent->GetDocument(*getter_AddRefs(containingDoc);
+    NS_ENSURE_TRUE(nsIDocument, NS_ERROR_FAILURE);
+    nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(containingDoc));
+    NS_ENSURE_TRUE(nsIDOMDocument, NS_ERROR_FAILURE);
+
+    nsCOMPtr<nsIDOMElement> menuItem;
+    
+    //quit menu item
+    domDoc->GetElementById(NS_LITERAL_STRING("menu_FileQuitItem"), 
getter_AddRefs(menuItem));
+    if(menuItem) {
+      nsCOMPtr<nsIDOMNode> quitNode(do_QueryInterface(menuItem));
+      nsCOMPtr<nsIDOMNode> quitParent;
+      quitNode->GetParentNode(getter_AddRefs(quitParent));
+      if(quitParent) {
+        nsCOMPtr<nsIDOMNode> removed;
+        quitParent->RemoveChild(quitNode,getter_AddRefs(removed));
+      }
+    }
+    
+    //quit menu seperator
+    domDoc->GetElementById(NS_LITERAL_STRING("menu_FileQuitSeparator"), 
getter_AddRefs(menuItem));
+    if(menuItem) {
+      nsCOMPtr<nsIDOMNode> quitNode(do_QueryInterface(menuItem));
+      nsCOMPtr<nsIDOMNode> quitParent;
+      quitNode->GetParentNode(getter_AddRefs(quitParent));
+      if(quitParent) {
+        nsCOMPtr<nsIDOMNode> removed;
+        quitParent->RemoveChild(quitNode,getter_AddRefs(removed));
+      }
+    }
+
+    //preferences menu item
+    domDoc->GetElementById(NS_LITERAL_STRING("menu_preferences"), 
getter_AddRefs(menuItem));
+    if(menuItem) {
+      nsCOMPtr<nsIDOMNode> prefNode(do_QueryInterface(menuItem));
+      nsCOMPtr<nsIDOMNode> prefParent;
+      quitNode->GetParentNode(getter_AddRefs(prefParent));
+      if(prefParent) {
+        nsCOMPtr<nsIDOMNode> removed;
+        quitParent->RemoveChild(quitNode,getter_AddRefs(removed));
+      }
+    }
+  }
   return NS_OK;
 }

Comment 18

[Andrew Thompson]

Oh, I'm not leaking menuItem twice there am I? nsCOMPtr releases the existing 
reference when you reassign it?

Comment 19

[Andrew Thompson]

Bah. sorry for the spam. told you it wouldn't compile... those last couple of 
lines should read "prefNode" and "prefParent'

Comment 20

[Mike Pinkerton (not reading bugmail)]

looks good (just eyeballed it). first off, we need to find out how to hookup the 
Prefs menu item in the app menu before we can remove it from Edit. second, the 
comment 

// ...do a depth first left to right search across the menubar ...

Is that how getElementByID() works? I thought it was a hash table.

Comment 21

[Andrew Thompson]

tee hee
No, that's a comment left over from my original plan, which was to walk the 
content tree myself. I realised that was a dumb thing to do :)

Comment 22

[Mike Pinkerton (not reading bugmail)]

From a list I'm on:

> How does one enable/disable the Preferences menu item in the Application
> menu on Mac OS X?
>
> With Carbon Events, you can use kHICommandPreferences and install a
> handler to detect the menu item selection and enable/disable the item.
>
> However, there is also a kAEShowPreferences Apple Event that the header
> says Mac OS X will send when choosing the Preferences item. That's nice,
> but how does one enable the menu item without a Carbon Event handler?

Use EnableMenuCommand with kHICommandPreferences.

Comment 23

[Mike Pinkerton (not reading bugmail)]

*** Bug 81814 has been marked as a duplicate of this bug. ***

Comment 24

[Mike Pinkerton (not reading bugmail)]

*** Bug 86540 has been marked as a duplicate of this bug. ***

Comment 25

[Steve Dagley]

are we still looking for this in 0.9.4?

Comment 26

[Conrad Carlen (not reading bugmail)]

This is worse than cosmetic. When "Quit" happens from the system menu, we're
bypassing the code in globalOverlay.js which goes through windows asking to save
changes. If you had been working on a doc for a while, chose "Quit" from the
system menu, expecting it to ask to save changes, you could be quite upset.
FWIW, I have a simple JS component which does what the code in
goQuitApplication() does with closing the windows. It can be used from C++ as
well as JS. This means we could add the window closing stuff into a place
(probably C++) where it would be called.

Comment 27

[Simon Fraser [no longer active]]

Conrad: danm was looking at this last week, for bug 92750.

Comment 28

[Mike Pinkerton (not reading bugmail)]

don't confuse the issue. that's another bug that danm is working on.

Comment 29

[Mike Pinkerton (not reading bugmail)]

prolly won't get to this with the l10n freeze

Comment 30

[Jaime Rodriguez, Jr.]

Propose we nsbranch- this one, unless it is a hard fast req for out OS X story.

Comment 31

[Steve Dagley]

We need to fix this.  See Conrad Carlen's comments from 09/04 regarding the 
dataloss implictions of this bug.

Comment 32

[Mike Pinkerton (not reading bugmail)]

once again, i remind people not to confuse this bug with the other bug that danm 
has about the quit menu item not going through the same path. this is a 
_different bug_ and in itself does not cause data loss. Futhermore, fixing this 
bug will not fix the data loss problem in the other bug.

Comment 33

[Andy Lyttle]

There's also a seperator above Preferences.

In 0.9.4 (2001091313), the application menu has a "Preferences..." option which
is disabled, and a "Quit Mozilla" option (cmd-Q).  Cosmetically, these are
located in the right places.  There is also "Quit" (cmd-Q) after a seperator at
the bottom of the File menu, and "Preferences..." after a seperator at the
bottom of the Edit menu.  The latter need to be moved to the positions of the
former, in order to comply with the Aqua UI specs
(http://developer.apple.com/techpubs/macosx/Essentials/AquaHIGuidelines/AHIGMenus/Standard_Pu_e_Menu_Bar_.html).

It's important to keep in mind that this is not a per-build issue, since the OSX
build also runs on OS9, and these menu changes must not apply when running on OS9.

While you're playing with menus, please check Bug 51125.

Hope this was helpful to somebody. :-)

Comment 34

[saari (gone)]

nsbranch-, not stop ship

Comment 35

[Andy Lyttle]

Conrad Carlen's issue with the Quit item in the Application menu not 
prompting to save changes should be documented in the release notes 
as a problem, if this doesn't get fixed for the next release.  Not prompting 
to save unsaved documents when expected causes data loss, which is a 
Bad Thing(tm).

Does Cmd-Q do the broken system Quit in the application menu, or the 
proper Mozilla quit in the File menu?  What about a "quit" AppleEvent, or 
whatever the OS sends on logout/shutdown?

Comment 36

[Conrad Carlen (not reading bugmail)]

> Conrad Carlen's issue with the Quit item

It's fixed - see bug 92750.

Comment 37

[Mike Pinkerton (not reading bugmail)]

Attachment: updated xul changes

Comment 38

[Mike Pinkerton (not reading bugmail)]

Attachment: changes to menu code for carbon

Comment 39

[Mike Pinkerton (not reading bugmail)]

ok, cpp and xul changes attached, separated to make it easier for people to wade
through each. pulling quit and prefs out of the file/edit menus respectively and
hooking up the app menu.

i changed it so that we hijack quit as a menu command instead of letting the
apple-event get processed. that way we don't end up with those crappy NSPR lockups.

needing r/sr.

Comment 40

[Blake Ross]

Comment on attachment 53770 [details] [diff] [review]
updated xul changes

sr=blake

Comment 41

[Steve Dagley]

Comment on attachment 53771 [details] [diff] [review]
changes to menu code for carbon

r=sdagley

Comment 42

[Simon Fraser [no longer active]]

Comments:
+NS_METHOD nsMenuX::AddSeparator ( ) 
extra spacing.

+nsMenuBarX :: AquifyMenuBar ( )
I hereby register an objection to "foo :: bar" spacing here, because it breaks 
everyone who wants to search for "::bar" to find the implemtentation of a method.
The space before the '(' also breaks searches for "bar(".

This spacing is also inconsistent with other older methods in this file.

+nsEventStatus
+nsMenuBarX :: ExecuteCommand ( nsIContent* inDispatchTo )
bad spacing in this method.

Comment 43

[Mike Pinkerton (not reading bugmail)]

Attachment: updated patch

Comment 44

[Mike Pinkerton (not reading bugmail)]

ok new patch up that fixes some tabs, only uses 1 UPP for the command event
handler, and only enables the prefs menu item when the window has a pref item in
its menu DOM (eg, bookmarks window does not).

needing new r/sr. only c++ changed. xul is still the same.

Comment 45

[Steve Dagley]

Comment on attachment 53811 [details] [diff] [review]
updated patch

r=sdagley

Comment 46

[Simon Fraser [no longer active]]

Comment on attachment 53811 [details] [diff] [review]
updated patch

sr=sfraser

Comment 47

[Mike Pinkerton (not reading bugmail)]

fixed.

Comment 48

[sairuh (rarely reading bugmail)]

yep! vrfy'ing, those menu items work just fine in their new locations.