User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5a) Gecko/20030616 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5a) Gecko/20030616 The remote XUL tutorial 'http://www.mozilla.org/docs/tutorials/sitenav' demonstrates the use of a XUL file to make a site navigation menu bar. This bar does not display at all on Mac OS X. Reproducible: Always Steps to Reproduce: 1. expected result: http://www.mozilla.org/docs/tutorials/sitenav/4.png (menubar on top) 2. no menubar on http://www.mozilla.org/docs/tutorials/sitenav/4.xul Actual Results: menubar not rendered Expected Results: render menubar

works on win2k....

Workaround: You can replace the <menubar> element with <hbox>. The top level-menus will not excactly behave like proper menus, but it should work until the bug is fixed.

The relevant piece of code is http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSFrameConstructor.cpp#5890. This prevents any XUL menubar from being created, so it can be handled in the Mac's menu bar. We need to be smarter here, though. I suggest something like this: #ifdef XP_MACOSX if (in chrome) { // use native menu bar on the Mac for chrome apps aHaltProcessing = PR_TRUE; return NS_OK; } else #endif { processChildren = PR_TRUE; rv = NS_NewMenuBarFrame(mPresShell, &newFrame); } How do we find out if we are in chrome at this point, though?

Maybe some code like in PresShell::SetPreferenceStyleRules(), also in IsChrome() in nsRuleNode.cpp. Maybe this should be an API on nsIPresShell?

This patch seems to work well. Thanks for the pointers, Simon. Doing some more testing to make sure everything is legit.

Strange. With this patch, the XUL menu at the URL above or at mab.mozdev.org looks fine. However, in Firefox, the menu text is sqeezed together. Maybe there is some CSS style that is missing or #ifdef'd out on Firefox. Mano, any ideas?

Remove unnecessary |if(NS_SUCCEEDED(result))| check.

bz, what do you think of this approach?

I'd need some time to look at it carefully (at least a week, most likely)... The first thing I wonder about is whether we really want to draw this line at chrome vs content docshells the way this patch is doing. For example should a non-root chrome shell really not show a menubar?

If I understand this correctly, then a root chrome shell with a menubar has the menubar hidden in the window and displayed in Mac's system menubar. Is there a valid user case where a non-root chrome shell would have a menubar?

The only difference between a chrome and non-chrome shell, in general, is the attribute used on the <xul:browser>... So one could have extensions that add chrome shells and allow the user to load content in them. That seems identical to the case here, no?

(In reply to comment #11) > So one could have extensions that add > chrome shells and allow the user to load content in them. Isn't this what Chatzilla does? When loading Chatzilla on the Mac, it's menubar gets put on the system menubar.

Chatzilla uses a root chrome shell, though, no?

Javier, try this xpi om Window & Mac Firefox (View->Open test): http://mano.fastmail.fm/Mozilla/testcases/210204/210204.xpi Screenshots http://mano.fastmail.fm/Mozilla/testcases/210204/210204._win.png http://mano.fastmail.fm/Mozilla/testcases/210204/210204_mac.png (fyi, those are w/o your patch applied) That's an example of the case Boris mentioned in comment 9 (An iframe inside a chrome window).

Thanks for the test XPI, Mano. Even with my patch, I see the top menubar get placed on Mac's system menubar, but the child menubar does not appear. So what is the correct way to handle this? Should only root chrome shell menubars be put on the system menubar, and everything else is displayed in the window? How do I tell if we are in a root chrome shell?

> Should only root chrome shell menubars be put on the system menubar, and > everything else is displayed in the window? That would make the most sense to me. > How do I tell if we are in a root chrome shell? You check that's it's a chrome shell, and then you check that it has no parent (or equals the root shell; your call). See nsIDocShellTreeItem.

This patch makes it so only menubars in the root chrome shell are put on the system menubar. Works with mab.mozdev.org and other examples, as well as Mano's test extension.

Comment on attachment 179092 [details] [diff] [review] patch v1.2 >Index: nsCSSFrameConstructor.cpp >+ nsCOMPtr<nsIDocShellTreeItem> >+ docShell(do_QueryInterface(container, &rv)); >+ if (NS_SUCCEEDED(rv) && docShell) { No need to check rv; if docShell is non-null, it better be valid here. Also, I'd prefer you name that variable treeItem, not docShell (since it may in fact not be a docshell, in general). >+ if (!parent) >+ isRootChromeShell = PR_TRUE; How about isRootChromeShell = !parent; >+ return NS_OK; >+ } else Else after return? No need for that. >+ { >+ processChildren = PR_TRUE; >+ rv = NS_NewMenuBarFrame(mPresShell, &newFrame); >+ } Which means no need to put curlies around this. r=bzbarsky with those fixed.

Patch with bz's suggestions.

(In reply to comment #18) >(From update of attachment 179092 [details] [diff] [review]) >>Index: nsCSSFrameConstructor.cpp >>+ if (!parent) >>+ isRootChromeShell = PR_TRUE; >How about > > isRootChromeShell = !parent; Better still? if (!parent) { *aHaltProcessing = PR_TRUE; return NS_OK; }

Well, I checked this in yesterday, before I saw Neil's comment. -> FIXED

Please open follow-up bugs to fix the mac themes...