365405 - the xbl binding for a menupopup is attached only once the menu is opened, after popupshowing is dispatched

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Asaf Romano (gone)]

In native menus, the XBL binding for a menupopup is only attached once you open the menu, after popupshowing is dispatched. This is OK unless that binding has its own popupshwoing handler which for that reason is only called starting the second time the menu is opened.

This breaks the places bookmarks menu on mac. Its binding sets a <handler event="popupwhoing"> for creating the menu content. Thus, the bookmarks menu (the non-static part, to be clear), is empty the first time it's opened.

Comment 1

[Asaf Romano (gone)]

Could we manually bound XBL here, synchronously? As things stands, it's only bound when dispatching the popupshowing event to a JS event listener.

Comment 2

[Boris Zbarsky [:bzbarsky]]

You can do that from JS easily -- just access the node that the XBL is attached to.  Or are you working from C++?  In that case you could manually do the XPConnect wrapping, I guess...

Comment 3

[Asaf Romano (gone)]

C++, native mac menus code.

Comment 4

[Asaf Romano (gone)]

Attachment: like this?

This doesn't seem to make any difference.

Comment 5

[(not reading, please use [e-mail] instead)]

asaf asked me if I saw this problem on windows xp.  I'm not seeing the issue he describes in his original comment, but I am seeing this:

with my trunk build (with --enable-places-bookmarks), the first time I click the personal toolbar overflow chevron I get a flash of a menupopup with all the bookmarks and then it quickly adjusts to be the correct (overflowed) items.

the second time I click within a session, I don't get the flash.

note, this doesn't happen all the time, but enough times that I noticed it.

Comment 6

[Phil Ringnalda (:philor)]

Say hello to my little friend bug 331297

Comment 7

[(not reading, please use [e-mail] instead)]

> Say hello to my little friend bug 331297

thanks phil.  that's exactly what I'm seeing.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Asaf, at the point when OnCreate runs, what is popupContent->GetCurrentDoc()?  And is that call happening after layout has started?  nsElementSH::PostCreate only attaches the binding if:

1)  The content is in a document.
2)  The document has a presshell (GetShellAt(0) returns something).

I wouldn't be suprised if one of those two conditions is not yet true in your code.

Comment 9

[Asaf Romano (gone)]

(In reply to comment #8)
> Asaf, at the point when OnCreate runs, what is popupContent->GetCurrentDoc()? 

Same as the owner document.

> And is that call happening after layout has started?

Of popupContent or the document? The former has no frame (display:none), as for the later, the method is called when you open the menu.

> only attaches the binding if:
> 
> (GetShellAt(0) returns something).

it does.

Please see comment 0, the element is bounded here, just way too late (when this method dispatches popupshoing to a JS listener).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can you breakpoint in the method you're changing, then when you hit that breakpoint in nsElementSH::PostCreate and step through to see what the deal is with the XBL binding attachment there?

Comment 11

[Asaf Romano (gone)]

Hrm, no PostCreate or GetNewOrUsed on the stack when WrapNative is called.

Comment 12

[Asaf Romano (gone)]

Oh, I misunderstood the meaning of the fourth argument of WrapNative. We're failing here:

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednativeinfo.cpp#234

because I'm passing an IID of a non-scriptable interface.

Comment 13

[Asaf Romano (gone)]

Attachment: patch

Thanks, Boris.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 250392 [details] [diff] [review]
patch

Hrm, this wouldn't work for  empty menus (before binding).

Comment 15

[Asaf Romano (gone)]

Attachment: patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

I won't be able to review this in a reasonable timeframe (probably not before mid-April at this point).  Please ask someone else?

Comment 17

[Asaf Romano (gone)]

mozilla/widget/src/cocoa/Makefile.in 1.62
mozilla/widget/src/cocoa/nsMenuX.h 1.17
mozilla/widget/src/cocoa/nsMenuX.mm 1.36