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.

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.

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

And we need it in Mozilla 0.9

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)

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

<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>

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.

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?

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?

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.

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...

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

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?

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.

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; }

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

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

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.

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 :)

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.

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

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

are we still looking for this in 0.9.4?

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.

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

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

prolly won't get to this with the l10n freeze

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

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

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.

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. :-)

nsbranch-, not stop ship

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?

> Conrad Carlen's issue with the Quit item It's fixed - see bug 92750.

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 on attachment 53770 [details] [diff] [review] updated xul changes sr=blake

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

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.

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 on attachment 53811 [details] [diff] [review] updated patch r=sdagley

Comment on attachment 53811 [details] [diff] [review] updated patch sr=sfraser

fixed.

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