Closed Bug 68098 Opened 20 years ago Closed 20 years ago

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

Categories

(Core :: XUL, defect, P2)

PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: mikepinkerton, Assigned: mikepinkerton)

References

(Blocks 1 open bug)

Details

(Keywords: polish, Whiteboard: OSX)

Attachments

(2 files, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Keywords: nsmac2, polish
Target Milestone: --- → mozilla1.0
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
Target Milestone: mozilla1.0 → mozilla0.9
Target Milestone: mozilla0.9 → mozilla0.9.1
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)
Keywords: mozilla1.0
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.
Blocks: 73812
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.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
*** Bug 81814 has been marked as a duplicate of this bug. ***
Summary: MacOS X File menu has 'Quit' → Preferences and Quit should be in their own menu on Mac OS X
Whiteboard: OSX
Target Milestone: mozilla0.9.2 → mozilla0.9.3
*** Bug 86540 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.3 → mozilla0.9.4
are we still looking for this in 0.9.4?
Keywords: nsbranch
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
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Blocks: 99194
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. :-)
Priority: -- → P2
nsbranch-, not stop ship
Keywords: nsbranchnsbranch-
Blocks: 102998
Target Milestone: mozilla0.9.5 → mozilla0.9.6
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.
Attachment #30109 - Attachment is obsolete: true
Attachment #30114 - Attachment is obsolete: true
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
Attachment #53770 - Flags: superreview+
Comment on attachment 53771 [details] [diff] [review]
changes to menu code for carbon

r=sdagley
Attachment #53771 - Flags: review+
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.
Attachment #53771 - Attachment is obsolete: true
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
Attachment #53811 - Flags: review+
Comment on attachment 53811 [details] [diff] [review]
updated patch

sr=sfraser
Attachment #53811 - Flags: superreview+
fixed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
yep! vrfy'ing, those menu items work just fine in their new locations.
Status: RESOLVED → VERIFIED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.