210204 - Remote XUL Tutorial does not work on Mac OS X (no menubar)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Thilo Planz]

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

Comment 1

[Matthias Versen [:Matti]]

works on win2k....

Comment 2

[Christopher Kohlhaas]

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.

Comment 3

[jhp (no longer active)]

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?

Comment 4

[Simon Fraser [no longer active]]

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

Comment 5

[jhp (no longer active)]

Attachment: patch

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

Comment 6

[jhp (no longer active)]

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?

Comment 7

[jhp (no longer active)]

Attachment: patch v1.1

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

Comment 8

[jhp (no longer active)]

Attachment: patch v1.1 (diff -w)

bz, what do you think of this approach?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[jhp (no longer active)]

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[jhp (no longer active)]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Chatzilla uses a root chrome shell, though, no?

Comment 14

[Asaf Romano (gone)]

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

Comment 15

[jhp (no longer active)]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[jhp (no longer active)]

Attachment: patch v1.2

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 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[jhp (no longer active)]

Attachment: patch v1.3

Patch with bz's suggestions.

Comment 20

[[e-mail]]

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

Comment 21

[jhp (no longer active)]

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

Comment 22

[Asaf Romano (gone)]

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