279703 - Need to redesign how XUL popups work

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

At the moment, XUL popups (menus, tooltips, etc) open and close by toggling the
"menugenerated" attribute on a menupopup or popup node.  This attribute change
causes a restyle (because of a rule in xul.css) that toggles the display
property of the popup, causing it to be shown.

Now the problem is that this attribute is set from all sorts of wacky places:

1)  From inside the AttributeChanged() handler of the menu frame
2)  From inside nsMenuFrame::GetPrefSize() (yes, folks, this is _inside_reflow_)

I see two possible solutuons.  Either we make the actual opening/closing of the
popup async, or we redesign everything so that we can deal with some sorts of
frame construction in the middle of reflow and such.  The former is probably not
too difficult (to handle the reflow case, we'd need to reflow the menu frame
when the popup actually comes up or something).

Thoughts?  Am I missing something?

Note that I'm pretty sure I can write XUL testcases where the current behavior
will cause crashes.  So marking this bug critical.

Comment 1

[[e-mail]]

Did you mean to say XUL menus open and close by toggling the "open" attribute?
The "menugenerated" attribute gets set once and never unset. This appears to be
to save time creating hidden popups on startup.

Comment 2

[Boris Zbarsky [:bzbarsky]]

No, I meant the menugenerated attribute.  It gets unset at least sometimes (see
DestroyPopup()).  But yes, it's not toggled on every open/close.

It _is_ toggled on the first open, and we need to eliminate that code.  So
either this optimization needs to go, or we need to put the attribute change on
an event or something.  I'm open to suggestions.

One question I have is what the relationship is between the thing setting
menugenerated and the popup it's being set on.  Is the popup always a descendant
of the other thing?

Comment 3

[[e-mail]]

(In reply to comment #2)
>No, I meant the menugenerated attribute.  It gets unset at least sometimes
>see DestroyPopup()).  But yes, it's not toggled on every open/close.
Ah, I'd looked at nsMenuFrame::DestroyPopup(), not the nsPopupSetFrame version.

>One question I have is what the relationship is between the thing setting
>menugenerated and the popup it's being set on.  Is the popup always a descendant
>of the other thing?
For a regular submenu, it's the immediate child, but for a context menu, it
could be a native anonymous popup set.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  So just selecting popups using CSS and the state of the menu won't work....

Comment 5

[Sébastien Auger]

I think the redesign would be the smarter approach here for 1.9 while considering something like a workaround or partial fix for 1.8.1 to avoid critical bugs caused by this one like the crash issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

There is no reasonable partial fix here.

Comment 7

[Neil Deakin]

OK, I'm working on a design and implementation for improving popups. I spent a couple of days and my initial work now opens popups asynchonously. The menugenerated attribute isn't used. Instead, popup frames are always created unlike before where they were created on demand. I'd still like it to generate the contents within the popup on demand, as performance might be an issue. I don't think it will be a problem to create the frames when needed.

When a request to open the popup is made, the popup is initialized, is marked as dirty so that it is laid out and the popup opened.

Comment 8

[Wayne Woods]

I assume this won't be ready before Firefox 2 is released? If not, I'll try to get a cheap hack on the branch for bug 331297, at least.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I doubt there's any way to create a patch here that would be safe for branch at this point....

Comment 10

[Neil Deakin]

Attachment: Redesign XUL popups

OK, here's what I have so far. Still needs a bit of polish and lots of testing.
 - use asynchronous frame construction, removes the menugenerated code
 - use asynchronous events
 - decomtamination, makes nsIMenuParent a non-xpcom interface
 - apart from a couple of functions needed for native themes, does the same for nsIMenuFrame
 - removes significant duplicated code, and code that does similar things in two places differently.
 - consolidates code such that there is only one API where popups are opened from.
 - moves much code out of frames and into a service
 - adds a <panel> element for popups that are not menus

I added a lazy frame generation though for the contents of popups. Not sure if this is a good approach, but am concerned about window opening performance otherwise.

I also would still like to move more code out of frames.

Comments and help testing are welcome.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I just looked at the frame constructor changes, and have a few comments:

1)  Could we replace the asserts that things QI to nsIPopupSetFrame with
    frametype checks or something instead of removing them wholesale?
2)  No need to null-check aParentFrame in the NS_STYLE_DISPLAY_POPUP block in
    nsCSSFrameConstructor::ConstructXULFrame -- it can't be null.
3)  NS_DispatchToCurrentThread is null-safe, so no need to do the OOM check in
    AddLazyChildren().  But I'd like us to use an nsCOMPtr<nsIRunnable> here
    and not a weak class pointer.
4)  Yay for removing the code in AttributeChanged!
5)  Getting presshell 0 in LazyGenerateChildrenEvent::Run looks wrong; we
    should be using whatever presshell was associated with the frame
    constructor that posed the event, no?  As far as that goes, what happens if 
    multiple frame constructors are live at once for the same document?
6)  Should we really be passing the primary frame as the parent in
    LazyGenerateChildrenEvent::Run?  Does that do the right thing with
    scrollframes?  Or can those not appear?
7)  Should LazyGenerateChildrenEvent::Run flush restyles?
8)  What should happen if the display of a LazyGenerateChildrenEvent's mContent 
    changes while the event is pending such that it no longer has a popup
    frame?

Comment 12

[Martijn Wargers (dead)]

I have the patch now in my debug build, some bugs I'm seeing:
- With menus, on first load, I see briefly a small popup (2px by 2px) before all the menu-items are displayed.
- When opening the url bar autocomplet widget, I get this assertion:
###!!! ASSERTION: rollup widget reassigned before release: '!gRollupWidget', fil
e c:/mozilla/mozilla/widget/src/windows/nsWindow.cpp, line 1174
and afterwards the popup opens, but it is completely blank (I should get history items). This happens with any autocomplete I see (search bar also, as well as autocomplet for websites).
- I don't get a popup anymore, when pressing on the overflow tab bar or the bookmarks toolbar overflow dropmarker.
- When a submenu has been shown in a context menu, the submenu will never disappear again, and you can't hover anymore over the items in the context menu of the parent menu.
- Something odd here: http://www.xulplanet.com/tutorials/xultu/examples/ex_popups_3.xul
When clicking for the 3rd time on the button, suddenly the popup moves to the top left, while it should remain just under the button.
- In "Customize toolbar", the text inside "Show: Icons and Text/Icons/Text" drop down doesn't show anymore and I can't open the drop down.
- I can't close a tab, by clicking on the close button in the tab, while the tooltip of the close button is open.

Comment 13

[Neil Deakin]

(In reply to comment #11)
> As far as that goes, what happens if multiple frame constructors are live at once for the same document?

When would such a situation occur?

> 6)  Should we really be passing the primary frame as the parent in
>     LazyGenerateChildrenEvent::Run?  Does that do the right thing with
>     scrollframes?  Or can those not appear?

Popups can't be scrollable as mayBeScrollable is always false when constructing popup frames. Instead, there is an arrowscrollbox defined in the anonymous content XBL for <menupopup>.

> 8)  What should happen if the display of a LazyGenerateChildrenEvent's mContent 
>     changes while the event is pending such that it no longer has a popup
>     frame?

What should eventually happen is that it should just return early without doing anything. I could add some code which just checks if it is a popup frame that hasn't had its contents generated yet. I think I may need to think about this lazy generation for a bit.

Comment 14

[[e-mail]]

(In reply to comment #13)
>Instead, there is an arrowscrollbox defined in the anonymous content XBL
Or simply a scrollbox in the case of a Classic menulist.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> When would such a situation occur?

At the moment, during printing.  In general, "any time someone sets up more than one presentation".

> Popups can't be scrollable

OK.  We should probably assert accordingly in LazyGenerateChildrenEvent::Run.

> What should eventually happen is that it should just return early without
> doing anything

Is that ideally, or with the existing patch?  I don't see how the existing patch does this.

Comment 16

[Neil Deakin]

(In reply to comment #12)
> I have the patch now in my debug build, some bugs I'm seeing:
> - With menus, on first load, I see briefly a small popup (2px by 2px) before
> all the menu-items are displayed.
> - When opening the url bar autocomplet widget, I get this assertion:
> ###!!! ASSERTION: rollup widget reassigned before release: '!gRollupWidget',
> fil
> e c:/mozilla/mozilla/widget/src/windows/nsWindow.cpp, line 1174
> and afterwards the popup opens, but it is completely blank (I should get
> history items). This happens with any autocomplete I see (search bar also, as
> well as autocomplet for websites).
> - When a submenu has been shown in a context menu, the submenu will never
> disappear again, and you can't hover anymore over the items in the context menu
> of the parent menu.
> - Something odd here:
> http://www.xulplanet.com/tutorials/xultu/examples/ex_popups_3.xul
> When clicking for the 3rd time on the button, suddenly the popup moves to the
> top left, while it should remain just under the button.

Martijn, I don't see any of these four issues above. Which platform are these on? The other ones I do see and will look into them.

Comment 17

[Martijn Wargers (dead)]

(In reply to comment #16)
> Martijn, I don't see any of these four issues above. Which platform are these
> on? The other ones I do see and will look into them.

I'm on windowsXP.

Comment 18

[Neil Deakin]

Attachment: Fix issues raised by bz and martijn

This should fix all the issues except for the one Martijn raised about the 2x2 popup which I can't reproduce.

Comment 19

[Neil Deakin]

Testcases are at:

http://xulplanet.com/ndeakin/tests/xts/menu/
 and
http://xulplanet.com/ndeakin/tests/xts/popup/

Comment 20

[Martijn Wargers (dead)]

(In reply to comment #18)
> Created an attachment (id=239217) [edit]
> Fix issues raised by bz and martijn
> 
> This should fix all the issues except for the one Martijn raised about the 2x2
> popup which I can't reproduce.

I've tried this new patch, I can still see the 2x2 issue with that patch.
All other issues I mentioned are fixed.

Issues, I see with this patch:
- I get this assertion:
  ###!!! ASSERTION: Unexpected parent: 'parent && parent->GetType() == nsGkAtoms::
  popupSetFrame', file c:/mozilla/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1905
  when doing "Customize.." or "Bookmark this page".
- When looking at a feed preview page, for instance: http://www.mozillazine.org/contents.rdf
  Open the combobox at the top and choose "Choose Application...". After that, the filepicker dialog pops up, but the dropdown floats above that dialog.
- In the bookmarks toolbar, the folders act like menus (clicking on one of them, then hovering over others open other folders). This works still, with the patch, but only partially, some folders won't open for me.

Comment 21

[Neil Deakin]

Attachment: Fix issues

This should fix the issues described above, and should also fix the 2x2 popup issue.

Comment 22

[Martijn Wargers (dead)]

(In reply to comment #21)
> This should fix the issues described above, and should also fix the 2x2 popup
> issue.

I've tried the patch, it indeed fixes the 2x2 popup issue, except for the bookmarks toolbar dropmarker, where I still can see the issue on first load.
Some other issues, I see with the patch:
- I don't get a context menu, when right-clicking on a bookmark, only a 2x2 popup.
- I get this assertion very often when doing Customize..: (to customize the toolbar items in the browser)
###!!! ASSERTION: attempt to remove an element that was never added: 'hep != nsn
ull && *hep != nsnull', file c:/mozilla/mozilla/content/xul/document/src/nsEleme
ntMap.cpp, line 281
I can't remember I got that before (I might be wrong, though)
- On first load, I don't get arrowsscrollbars when I open up my bookmark popup.
- When looking at a feed preview page, for instance:
http://www.mozillazine.org/contents.rdf
  Open the combobox at the top and _right-click_ (not left-click_ "Choose Application...". After that, the filepicker dialog pops up, but the dropdown floats above that dialog.
- After clicking on a menu-item in a menu, the menu-button is going back into its hovered state.
- When moving through the menu-buttons while the popup is open, you get to see briefly a hovered over menu-button, that shouldn't happen, it should directly get a depressed look.
- When opening the history sidebar, and then opening the View menu-popup, and then choosing 'by size', 'by date', etc, I get multiple checked menu-items after a while.

Comment 23

[Neil Deakin]

> Some other issues, I see with the patch:
> - I don't get a context menu, when right-clicking on a bookmark, only a 2x2
> popup.

This appears to be a microsummaries error when I context-click a bookmark, likely a new issue related to disabling places and unrelated to the popup changes.


> - I get this assertion very often when doing Customize..: (to customize the
> toolbar items in the browser)
> ###!!! ASSERTION: attempt to remove an element that was never added: 'hep !=
> nsn
> ull && *hep != nsnull', file
> c:/mozilla/mozilla/content/xul/document/src/nsEleme
> ntMap.cpp, line 281
> I can't remember I got that before (I might be wrong, though)

I see that on a build without this patch also, so unrelated.

> - On first load, I don't get arrowsscrollbars when I open up my bookmark popup.

I see this and will see if I can find a fix.
 
> - When looking at a feed preview page, for instance:
> http://www.mozillazine.org/contents.rdf
>   Open the combobox at the top and _right-click_ (not left-click_ "Choose
> Application...". After that, the filepicker dialog pops up, but the dropdown
> floats above that dialog.

That is a bug in the feedpreview code which should be using the command event rather than the click event to respond to the menu. It also makes it usuable with the keyboard.

> - After clicking on a menu-item in a menu, the menu-button is going back into
> its hovered state.

Fixed this issue.

> - When moving through the menu-buttons while the popup is open, you get to see
> briefly a hovered over menu-button, that shouldn't happen, it should directly
> get a depressed look.

Not clear what you mean here, or maybe I just don't see it. You are rolling the mouse over the items on a menubar?

> - When opening the history sidebar, and then opening the View menu-popup, and
> then choosing 'by size', 'by date', etc, I get multiple checked menu-items
> after a while.

Fixed this issue.

Thanks for all the testing help. I think the patch is getting better ;)

Comment 24

[Asaf Romano (gone)]

FWIW, the change from command to click event in the feed preview page was intentional, see bug 351263 where i forgot to check for event.button...

Comment 25

[Neil Deakin]

Attachment: Fixed latest set of issues

Comment 26

[Neil Deakin]

Attachment: C++ changes only

OK Martijn thinks this is in good enough shape to be reviewed, although there are a couple of minor issues which I'll look into. Comment 10 lists the main changes.

Comment 27

[Boris Zbarsky [:bzbarsky]]

I'll try to get to this ASAP, but that probably means at least a week.  I'm right in the middle of doing two largish reviews right now.  :(

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If Boris is going to sr this (and I think he should) then do you really need me to review too?

Comment 29

[[e-mail]]

Comment on attachment 242019 [details] [diff] [review]
C++ changes only

>-NS_INTERFACE_MAP_BEGIN(XULPopupListenerImpl)
>-  NS_INTERFACE_MAP_ENTRY(nsIXULPopupListener)
>+NS_INTERFACE_MAP_BEGIN(XULPopupListener)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMMouseListener)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMContextMenuListener)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIDOMEventListener, nsIDOMMouseListener)
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIXULPopupListener)
> NS_INTERFACE_MAP_END
I think you need to change nsIXULPopupListener to nsIDOMMouseListener i.e.
NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMMouseListener)

> NS_INTERFACE_MAP_BEGIN(nsMenuFrame)
>-  NS_INTERFACE_MAP_ENTRY(nsIMenuFrame)
>   NS_INTERFACE_MAP_ENTRY(nsIScrollableViewProvider)
>+  NS_INTERFACE_MAP_ENTRY(nsIMenuFrame)
> NS_INTERFACE_MAP_END_INHERITING(nsBoxFrame)
Heh ;-)

Comment 30

[timeless]

this missed bz's window. roc is it still on your agenda?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can't bz review this next month?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> this missed bz's window.

Can I be the judge of that?  I have network till 12/20 or so, just not much in the way of a development environment.  This is on my list of things to try to review before then, if I can.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Neil, I started looking at this and I was wondering whether there's a description  of how the new world works either in the patch or elsewhere, just as a rough guide to reviewing.

Comment 34

[Neil Deakin]

Comment 10 lists the specific changes that were made. The general idea is that all access to popups is done via the singleton service nsXULPopupManager rather than calling frame code. The apis for this are in nsXULPopupManager.h which use content nodes rather than frames. (The functions in there that take frames as arguments shouldn't be called except from the menu/popup frame code; I should make that part clearer in the file documentation) Now, the only way to open a popup is to call the ShowXXX methods which deal with sending events asyncronously. This service also maintains a linked list of all open popups so that they don't get into a state where some hang around and can't be closed.

Much of the code changes involve moving code into the service so that there are far fewer places where frame code opens a popup directly. There are still a couple in the menu mouse event handling which now should be doing things asynchonously.

Eventually, more code should be moved out of the frames, and the popup/menu box objects can be removed.

There's some info about the old popup code at http://wiki.mozilla.org/XUL:Popups:Issues I'll try and write a similar document for the new way.

Comment 35

[[e-mail]]

(In reply to comment #34)
>Eventually, more code should be moved out of the frames, and the popup/menu box
>objects can be removed.
Then what's the replacement for this.menuBoxObject.handleKeyPress(event); or this.menuBoxObject.openMenu(val); ?

Comment 36

[Neil Deakin]

bz, do you want me to make a more up to date patch for this?

Comment 37

[Boris Zbarsky [:bzbarsky]]

If it would be easy, that would rock.  If you put it up sometime today, I've got a nice long plane flight tomorrow when I was going to start looking!

Comment 38

[Neil Deakin]

Attachment: updated popup reworking patch to trunk

Comment 39

[[e-mail]]

Comment on attachment 260959 [details] [diff] [review]
updated popup reworking patch to trunk

Note: next time, don't forget to try patches out on non-minimal trees - in this case this patch breaks accessible/base/src/nsRootAccessible.cpp.

Comment 40

[[e-mail]]

Comment on attachment 260959 [details] [diff] [review]
updated popup reworking patch to trunk

Some good news: this almost fixes bug 357337, except for some reason you need orient="vertical" on the tooltip to make the test case work. However, there did appear to be one regression: auto-selecting the current item when you open a menulist doesn't work, which may explain why keyboard navigation in a closed menulist doesn't work either. (And this was a suiterunner build so it's using toolkit's widgets in case that makes any difference.)

Comment 41

[Boris Zbarsky [:bzbarsky]]

So I'm still only partway through, but I figure I should post what I have so far (or at least the coherent, not-lots-of-notes-to-self) part of it:

>Index: browser/base/content/browser.xul
>-    <popup type="autocomplete" chromedir="&locale.dir;" id="PopupAutoComplete"/>
>+    <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoComplete"/>

This change should get OKed by an owner for this code.

>Index: content/base/src/nsGkAtomList.h
>+GK_ATOM(after_pointer, "after_pointer")

This doesn't seem to be used.  Is this legwork for using FindAttrValueIn in nsMenuPopupFrame::InitializePopup?

>Index: content/xul/content/src/nsXULElement.cpp

> nsXULElement::AddPopupListener(nsIAtom* aName)

>+    nsCOMPtr<nsIAtom> listenerAtom = isContext ?

Why not just nsIAtom* to avoid extra addrefs?

>+    nsCOMPtr<nsIDOMEventListener> popupListener =
>+        NS_STATIC_CAST(nsIDOMEventListener*, GetProperty(listenerAtom));

And why not just nsIDOMEventListener* here?  I guess this simplifies the code below?  If so, leaving this as an nsCOMPtr sounds fine by me.

>+    nsIDOMEventListener* listener = popupListener;
>     NS_ADDREF(listener);

Could also do:

  nsIDOMEventListener* listener = nsnull;
  popupListener.swap(listener);

to avoid extra refcounting.  In either case, please add a comment saying that after the SetProperty call we want the property to own one of the refs, which is why we're not releasing it here.

>+    target->AddEventListener(NS_LITERAL_STRING("mousedown"), popupListener, PR_FALSE);
>+    target->AddEventListener(NS_LITERAL_STRING("contextmenu"), popupListener, PR_FALSE);

So is it me, or does nsXULPopupListener ignore MouseDown() for context popups and ContextMenu() for non-context ones?  And if so, would it make sense to only register for the event we really care about, based on isContext?

>Index: content/xul/content/src/nsXULPopupListener.cpp

>+XULPopupListener::~XULPopupListener(void)
> #ifdef DEBUG_REFS
>     --gInstanceCount;
>-    fprintf(stdout, "%d - RDF: XULPopupListenerImpl\n", gInstanceCount);
>+    fprintf(stdout, "%d: XULPopupListener\n", gInstanceCount);

This seems to be dead code (in particular, no one I see increments gInstanceCount).  Just remove it?

>+XULPopupListener::PreLaunchPopup(nsIDOMEvent* aMouseEvent)

>   // This is a gross hack to deal with a recursive popup situation happening in AIM code. 

File followup bugs as needed if this needs improving or removing, ok?

>+  if (xulDocument) {

So this stuff no longer depends on being in a nsXULDocument to work right?  Or it never did?

>-  xulDocument->SetTrustedPopupEvent( aMouseEvent );

I think this patch makes Get/SetTrustedPopupEvent unused.  Want to remove the IDL decls of these methods and the implementations too?

>+    if (button == 0) {
>+      // Time to launch a popup menu.

Couldn't we just do an early return here if button != 0, now that we don't have to SetTrustedPopupEvent(nsnull)?

>+      LaunchPopup(aMouseEvent, targetContent);
>       aMouseEvent->StopPropagation();
>       aMouseEvent->PreventDefault();

Then that chunk of code could just come after the if/else (the if branch would do the FireFocusOnTargetContent, the else branch would do the button check, and if we don't return early we would run this shared code).

>+XULPopupListener :: ClosePopup ( )

Why the extra spaces?

>+    if (pm)
>+      pm->HidePopup(mPopupContent, PR_FALSE, PR_TRUE);

Document why this is synchronous?

> // LaunchPopup
...
> // This looks for an attribute on |aElement| of the appropriate popup type 

This doesn't really seem to match the code anymore.  Change that to mElement instead?

Or should that be aTargetContent?  How is that related to mElement anyway, or to aEvent?  Document the relationship?  Oh, and the arg names in the header are different; might be worth making them match.

>+    pm->ShowPopup(mPopupContent, content, NS_LITERAL_STRING(""), 0, 0,

EmptyString(), please.

>+    // position the menu two pixels down and to the right from the current
>+    // mouse position. This makes it easier to dismiss the menu by just clicking
>+    if (mIsContext) {

Put the comment inside the |if|?


>Index: content/xul/content/src/nsXULPopupListener.h

> +//   This is the popup listener implementation for popup menus and context menus.

Put that as a "/**" comment before the include guard so LXR will pick it up?

>+class XULPopupListener : public nsIDOMMouseListener,

As long as you're renaming the class, why not call it nsXULPopupListener?

>+    XULPopupListener(nsIDOMElement *aElement, PRBool aIsContext);

Document the args?

>+    static void
>+    AdjustClientXYForNestedDocuments(nsIContent* aTarget, nsIContent* aPopupElement,
>+                                     PRInt32 &aClientX, PRInt32 &aClientY);
>+    virtual nsresult LaunchPopup(nsIDOMEvent* anEvent, nsIContent* aTarget);
>+    virtual void ClosePopup();

Please document these too.  In particular, what exact coordinate transformation AdjustClientXYForNestedDocuments is performing and what the arguments of LaunchPopup mean.

>+    nsresult PreLaunchPopup(nsIDOMEvent* aMouseEvent);
>+    nsresult FireFocusOnTargetContent(nsIDOMNode* aTargetNode);

I know you just moved these, but maybe document them as well?

>+    static nsIView* GetRootViewForPopup(nsIFrame* aStartFrame);

I don't see this implemented anywhere.  Am I missing something?

>+    // |mElement| is the node to which this listener is attached.
>+    nsIDOMElement* mElement;               // Weak ref. The element will go away first.
>+
>+    // The popup that is getting shown on top of mElement.
>+    nsCOMPtr<nsIContent> mPopupContent; 

Could those end up pointing to the same element?  If so, would that leak?  Or could we have two listeners, set as event listeners on two different elements and pointing to each other's targets via mPopupContent?

It looks like nsGenericElement::Traverse already traverses the popup and context menu listeners; would it make sense to make this class participate in cycle collection and just use strong refs here?

Or do the rules about when mPopupContent is null prevent that from happening?  Worth documenting exactly when it's null, and explaining how that affects the ownership model if that's how we avoid leaking.

>+nsresult
>+NS_NewXULPopupListener(nsIDOMElement* aElement, PRBool aIsContext,
>+                       nsIDOMEventListener** aListener);

Please document?  Esp. what aElement is here?

>Index: layout/base/nsCSSFrameConstructor.cpp

>+        else if (aTag == nsGkAtoms::menupopup || aTag == nsGkAtoms::popup) {
>+          // menu popups generate their child frames lazily only when opened.
>+          // However, generate child frames normally if the parent menu has
>+          // a sizetopopup attribute. In this case the size of the parent menu
>+          // is dependant on the size of the popup, so the frames need to
>+          // exist in order to calculate this size.

So what happens if the "sizetopopup" attribute is set dynamically?  Or if a menupopup without this attribute has a child appended via the DOM?  We'll construct a frame for that child; then what happens when we open the menupopup?  It looks like we won't construct frames for anything else, right?

Would it make more sense to move this logic into the relevant frames' IsLeaf() methods so that we're consistent about when we do want to construct child stuff for them and when we don't?

>@@ -10129,39 +10145,16 @@ nsCSSFrameConstructor::AttributeChanged(

I _like_ the changes to this method!  ;)

>+nsCSSFrameConstructor::LazyGenerateChildrenEvent::Run()

>+  mPresShell->GetDocument()->FlushPendingNotifications(Flush_OnlyReflow);

Why only reflow?  And why on the document, not on the presshell?  If we're trying to make sure that screen coordinates are up to date, we do in fact want to flush onthe document, but then we should Flush_Layout, no?

>+    if ((NS_STATIC_CAST(nsMenuPopupFrame *, frame))->HasGeneratedChildren())

I don't think you need the extra parens around NS_STATIC_CAST.

>+    nsFrameConstructorState state(mPresShell, nsnull, nsnull, nsnull);

So this will give inconsistent handling of positioned and floating kids with popups that construct eagerly, no?  I guess we might not care (or only care in a followup bug)...

>Index: layout/base/nsCSSFrameConstructor.h
>+  // aCallback will be called with the argument aArg when the children
>+  // have been added.

Document what the aContent and aFrame args passed to aCallback will be?

Do you want this method to process the XBL constructors for the kids?  If so, will that happen before or after the callback is called?  I would suggest "yes" and "after".

>Index: layout/base/nsDocumentViewer.cpp

I'd like to see these changes after merging to bug 374570.

>Index: layout/build/nsLayoutCID.h
>+// {14632191-AC21-4BDF-83E7-2363AD17E838}
>+#define NS_XULPOPUPMANAGER_CID \
>+{ 0x14632191, 0xac21, 0x4bdf, { 0x83, 0xe7, 0x23, 0x63, 0xad, 0x17, 0xe8, 0x38 } }
>+

Why?  This doesn't really seem to be used...

>Index: layout/build/nsLayoutModule.cpp

>+  { "XUL Popup Manager",
>+    NS_XULPOPUPMANAGER_CID,
>+    "@mozilla.org/xul/xul-popup-manager;1",
>+    NS_NewXULPopupManager },

Again, doesn't seem to be used.  What would be the point?

>Index: layout/generic/nsContainerFrame.cpp
>+    do {
>+      childListName = aFrame->GetAdditionalChildListName(childListIndex++);
>+    } while (childListName == nsGkAtoms::popupList);

I'm not sure I follow this change.  Please document very clearly (and probably run it by roc).

>Index: layout/xul/base/public/nsIPopupBoxObject.idl

>+   *  This method is deprecated. Use openPopup or openPopupAtScreen instead.
>   void showPopup(in nsIDOMElement srcContent, in nsIDOMElement popupContent,

Do you want to document the args?  Or do you plan to remove it soon anyway?  If so, don't worry about it.

>+   * behaviour and will install capturing key event listeners to the popup

"on the popup"?

>@@ -84,15 +97,61 @@ interface nsIPopupBoxObject : nsISupport

>+   * argument. If aAttributesOverride is false, the attributes are only used
>+   * if the values passed to this method are empty.

"the values"?  You mean if |position| is empty?

Also, is it aAttributesOverride or attributesOverride?

>+   * For an anchored popup, the x and y arguments may be used to offset the 
>+   * popup from its anchored position by some number, measured in pixels.

CSS pixels, right?

>+   * Unanchored popups may be created by supplying null as the anchor node.
>+   * An unanchored popup appears at the position specified by x and y,
>+   * relative to the viewport of the document containing the popup node.

In this case |position| and |attributesOverride| are ignored, right?  Say that explicitly?

>+                 in PRBool isContextMenu,
>+                 in PRBool attributesOverride);

s/PRBool/boolean/ ?

>+   * Open the popup at a specific screen position specified by x and y.

In CSS pixels, right?

Which screen is used on a multi-monitor system?

>+  void openPopupAtScreen(in long x, in long y, in PRBool isContextMenu);

Again, "boolean".

I really like the docs here!

>Index: layout/xul/base/public/nsXULPopupManager.h

>+/**
>+ * The XUL Popup Manager keeps track of all open popups. There are two types

Maybe put part of this comment (a short one-sentence description) before the include guard so LXR will pick it up?

>+ *   - panels, which stay open until a request is made to close them. This
>+ *     type is used by tooltips.

So how do "panel" and "tooltip" differ exactly?  We seem to have both now... is the plan to eliminate <tooltip>?

As an uber-nit, file a followup bug to get documentation about <panel> up somewhere like devmo?

Just to make sure I understand... does this mean that clicks no longer close tooltips?  Or that it's the responsibility of whoever poses the tooltip to deal with clicks?  Or something else?

Someone who's more familiar than I with the UI code should review the s/panel/tooltip/ changes.  Maybe gavin or Neil Rashbrook?

>+ * When a new menu is opened, it is appended to the popup chain, stored in a

s/menu/popup/, right?

>+#ifdef IBMBIDI

I know you just moved this, but don't bother with that define.  This is always defined; we're only keeping the old ifdefs because old code in them is known to need review, and removing them as we review it.  This code looks fine, or will once we're done with it.  ;)

>+#define NS_DIRECTION_FROM_KEY_CODE(frame, direction, keycode)    \
>+  NS_ASSERTION(keycode >= NS_VK_END && keycode <= NS_VK_DOWN,    \
>+               "Illegal key code");                              \

Please also add asserts like:

    NS_ASSERTION(NS_VK_HOME == NS_VK_END + 1, "Broken ordering");

for all five of the assumptions you're making here?  I really wish we could make it an #error if that condition fails, but I'm not sure the preprocessor can do math on class members... :(

>+  const nsStyleVisibility* vis = frame->GetStyleVisibility();    \
>+  if (vis->mDirection == NS_STYLE_DIRECTION_RTL)                 \
>+    direction = DirectionFromKeyCode_rl_tb[keycode - NS_VK_END]; \
>+  else                                                           \
>+    direction = DirectionFromKeyCode_lr_tb[keycode - NS_VK_END];

I was thinking... it might be somewhat faster and clearer like:

#if NS_STYLE_DIRECTION_LTR != 0 || NS_STYLE_DIRECTION_RTL != 1
#error "Can't deal with that"
#endif

static const nsNavigationDirection DirectionFromKeyCodeTable[2][6] = {
  {
    eNavigationDirection_Last,   // NS_VK_END
    eNavigationDirection_First,  // NS_VK_HOME
    eNavigationDirection_Start,  // NS_VK_LEFT
    eNavigationDirection_Before, // NS_VK_UP
    eNavigationDirection_End,    // NS_VK_RIGHT
    eNavigationDirection_After   // NS_VK_DOWN
  },
  {
    // RTL stuff here
  }
};

And then:

#define NS_DIRECTION_FROM_KEY_CODE(frame, keycode) \
  (DirectionFromKeyCodeTable[frame->GetStyleVisibility()->mDirection] \
                            [keycode - NS_VK_END])

If you'd prefer to keep the old tables I'm ok with that too, I guess; I really want to eliminate the hidden assignement here by changing to a macro that returns the thing to assign.  That could be done with the old tables and ?:, but the multi-dimensional table setup scales better to the future CSS world, perhaps.

Feel free to do this all as a followup bug, by the way.  I know you just copied this stuff.  ;)

>+  nsMenuChainItem(nsMenuPopupFrame* aFrame, PRBool aIsContext, PRBool aIsMenu)
>+      mIgnoreKeys(PR_FALSE),
...
>+    // always ignore keys on non-menus
>+    if (!aIsMenu)
>+      mIgnoreKeys = PR_TRUE;

Just do:

        mIgnoreKeys(!aIsMenu),

in the initializer?  That's equivalent, and faster.

Should the constructor assert that |aFrame| is not null?

>+  nsMenuChainItem* Parent() { return mParent; }
>+  nsMenuChainItem* Child() { return mChild; }

I'd call those GetParent() and GetChild(), since they can be null...

>+  void SetParent(nsMenuChainItem* aParent) { mParent = aParent; }
>+  void SetChild(nsMenuChainItem* aChild) { mChild = aChild; }

Do we really want the two separate setters?  Wouldn't it be the case that mParent->mChild == this always?  So just calling SetParent() on a menu should be enough, if SetParent() looks like:

  if (mParent) {
    NS_ASSERTION(mParent->mChild == this, "Unexpected");
    mParent->mChild = nsnull;
  }
  mParent = aParent;
  NS_ASSERTION(mParent->mChild == nsnull, "Unexpected");
  mParent->mChild = this;

or something.  That would make it hard to create broken doubly-linked lists.
  
>+  void Detach(nsMenuChainItem** aRoot);

What does this do?

>+  nsXULPopupShowingEvent(nsIContent *aPopup,
>+                         nsIContent *aMenu,

Assert that those content pointers are not null, if you can?

>+  nsXULPopupHidingEvent(nsIContent *aPopup,
>+                        nsIContent* aNextPopup,
>+                        nsIContent* aLastPopup,

Same here.

For what it's worth, these are transient objects, so you might as well use PRBool instead of PRPackedBool, I think.

>+  nsXULMenuCommandEvent(nsIContent *aMenu,

And here.

Comment 42

[Aaron Leventhal]

We use menugenerated in nsXULMenuAccessible, so that menus are not generated lazily when accessibility is atcive. The accessible objects for menus need to be created when they're asked for, because tools like onscreen keyboards need to be able to build alternate access to the menus.

What's the new way of doing this. I don't see anything in the patch that removes our menugenerated usage. Is there some new way to handle this in nsXULMenuAccessible?

Comment 43

[Aaron Leventhal]

I should note, we still have problems with this, which I haven't investigated deeply (bug 332663).

Comment 44

[Neil Deakin]

What does acessibility need the menu contents generated for? So that XBL is available for menuitems?

Is is sufficent to call mPresShell->IsAccessibilityActive() and not create menus lazily when true?

Comment 45

[Aaron Leventhal]

> What does acessibility need the menu contents generated for?
An onscreen keyboard for switch users like Stephen Hawking uses it to build a special keyboard with choices. A lightbar goes through the choice one item at a time until a user activates the switch.

> Is is sufficent to call mPresShell->IsAccessibilityActive() and not create
> menus lazily when true?
I'd prefer that we have control over it, because this isn't necessary for screen readers, screen magnifiers etc. I'd prefer to take the perf hit only the accessible objects are requested.

Comment 46

[Neil Deakin]

(In reply to comment #45)
> > What does acessibility need the menu contents generated for?
> An onscreen keyboard for switch users like Stephen Hawking uses it to build a
> special keyboard with choices. A lightbar goes through the choice one item at a
> time until a user activates the switch.
> 

I meant why do the frames need to be generated for this?

Comment 47

[Aaron Leventhal]

We don't generate accessible objects unless there is a frame. A lot of a11y getters require that info.

I suppose one option is to have us to just fail on getters that require layout info, when the frame is not available. However that would be a lot of careful work.

Comment 48

[Neil Deakin]

(In reply to comment #41)
> This change should get OKed by an owner for this code.

I plan on getting the browser/toolkit parts reviewed by someelse.

> And if so, would it make sense to only register for the event we really care about, based on isContext?

Yes, it would.

> >+    if (pm)
> >+      pm->HidePopup(mPopupContent, PR_FALSE, PR_TRUE);
> 
> Document why this is synchronous?

Don't think it needs to be. It's probably safer asynchronously as it is called in a destructor.

> 
> Please document these too.  In particular, what exact coordinate transformation
> AdjustClientXYForNestedDocuments is performing and what the arguments of
> LaunchPopup mean.

AdjustClientXYForNestedDocuments shouldn't even be there.

> >+    static nsIView* GetRootViewForPopup(nsIFrame* aStartFrame);
> 
> I don't see this implemented anywhere.  Am I missing something?

Nope, it is leftover from an earlier version of the patch.

> It looks like nsGenericElement::Traverse already traverses the popup and
> context menu listeners; would it make sense to make this class participate in
> cycle collection and just use strong refs here?

I think this would be better.

> 
> Why only reflow?  And why on the document, not on the presshell?  If we're
> trying to make sure that screen coordinates are up to date, we do in fact want
> to flush onthe document, but then we should Flush_Layout, no?

I think Flush_Layout perhaps is ok

> 
> >+    nsFrameConstructorState state(mPresShell, nsnull, nsnull, nsnull);
> 
> So this will give inconsistent handling of positioned and floating kids with
> popups that construct eagerly, no?  I guess we might not care (or only care in
> a followup bug)...

A followup bug would be good. Positioned content doesn't really work properly in xul documents anyway.

> 
> Do you want this method to process the XBL constructors for the kids?  If so,
> will that happen before or after the callback is called?  I would suggest "yes"
> and "after".

Do you mean I should be calling ProcessAttachedQueue() here after the callback?

> >Index: layout/generic/nsContainerFrame.cpp
> >+    do {
> >+      childListName = aFrame->GetAdditionalChildListName(childListIndex++);
> >+    } while (childListName == nsGkAtoms::popupList);
> 
> I'm not sure I follow this change.  Please document very clearly (and probably
> run it by roc).

I wish I had documented it when I wrote it, but I'm pretty sure that this is because the menu frame positions the view itself.
 
> >+   *  This method is deprecated. Use openPopup or openPopupAtScreen instead.
> >   void showPopup(in nsIDOMElement srcContent, in nsIDOMElement popupContent,
> 
> Do you want to document the args?  Or do you plan to remove it soon anyway?  If
> so, don't worry about it.

I will remove it in 2.0. I can add some documentation but don't really want to encourage it's use.

> >+   * Open the popup at a specific screen position specified by x and y.
> 
> In CSS pixels, right?

Not currently, but it should be using css pixels, so I'll make that fix.

> 
> Which screen is used on a multi-monitor system?

> So how do "panel" and "tooltip" differ exactly?  We seem to have both now... is
> the plan to eliminate <tooltip>?

<panel> is intended as a popup that contains content that isn't a menu, like a popup search field or such. It doesn't get the menu key listeners attached and doesn't close up a chain of menus when closed.

> 
> As an uber-nit, file a followup bug to get documentation about <panel> up
> somewhere like devmo?

Yes, I have some documentation I plan to upload once this patch goes in.
 
> 
> Just to make sure I understand... does this mean that clicks no longer close
> tooltips?

No, that still occurs in the widget code as with other popups.

> 
> >+  void Detach(nsMenuChainItem** aRoot);
> 
> What does this do?

Will add documentation that this removes an item from a chain, such that with a->b->c, b->Detach(&top) results in a->c.

Comment 49

[Neil Deakin]

aaronlev: I can add a method to force the popup to generate it's frames. It will occur asynchronously though.

Comment 50

[[e-mail]]

(In reply to comment #48)
> <panel> is intended as a popup that contains content that isn't a menu, like a
> popup search field or such. It doesn't get the menu key listeners attached and
> doesn't close up a chain of menus when closed.
So <xul:menupopup class="folderTargetPopup"> should become a panel?

Comment 51

[Aaron Leventhal]

Neil D, thanks.

Comment 52

[Neil Deakin]

(In reply to comment #50)
> So <xul:menupopup class="folderTargetPopup"> should become a panel?
> 

Don't know. What is it?

Comment 53

[[e-mail]]

(In reply to comment #52)
>(In reply to comment #50)
>>So <xul:menupopup class="folderTargetPopup"> should become a panel?
>Don't know. What is it?
It's an emulation of a non-editable menulist using a tree
(so somewhat similar to an autocomplete widget in that respect).

Comment 54

[Boris Zbarsky [:bzbarsky]]

> Do you mean I should be calling ProcessAttachedQueue() here after the callback?

If you want to run XBL constructors immediately, then yes.

> but I'm pretty sure that this is because the menu frame positions the view
> itself.

That makes some sense... please do add a comment.

> Not currently, but it should be using css pixels, so I'll make that fix.

I assume that the other relevant APIs (e.g. the screenX/screenY on events) use CSS pixels?  We probably want to do the same thing those APIs are doing.

> <panel> is intended as a popup that contains content that isn't a menu,

That seems to include tooltips.  So again, is the plan to make <tooltip> just a synonym for <panel>?

Comment 55

[Neil Deakin]

(In reply to comment #54)
Boris:
> 
> That seems to include tooltips.  So again, is the plan to make <tooltip> just a
> synonym for <panel>?

They would behave the same internally yes. The <tooltip> is styled differently though (tooltip coloured background, etc) and one would naturally use it for things that are tooltips. <menupopup> for menus, and <panel> for anything else.

Neil:
> It's an emulation of a non-editable menulist using a tree (so somewhat similar to an autocomplete widget in that respect).

Is it disabling the key handlers currently (ignorekeys or otherwise)? It should work as is, but <panel> might be more correct.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> They would behave the same internally yes. 

Great.  Thanks for clarifying that!

Comment 57

[Eli Friedman]

(In reply to comment #54)
> I assume that the other relevant APIs (e.g. the screenX/screenY on events) use
> CSS pixels?  We probably want to do the same thing those APIs are doing.

All the DOM APIs use CSS pixels.

Comment 58

[Steve Won (stevewon on irc)]

Hi. I am having a weird popup bug, and I think it might be related to this.

Having a richlistbox or any other box with style set to overflow:auto inserted in a popup seems to cause a weird behavior. Some of the other elements (e.g, description element) in the popup get duplicated within the box.

Thanks.

Comment 59

[Eli Friedman]

(In reply to comment #58)
> Hi. I am having a weird popup bug, and I think it might be related to this.
> 
> Having a richlistbox or any other box with style set to overflow:auto inserted
> in a popup seems to cause a weird behavior. Some of the other elements (e.g,
> description element) in the popup get duplicated within the box.
> 
> Thanks.

Please file a new bug with the problem and a reduced testcase; random comments like this are likely to get lost.

Comment 60

[Boris Zbarsky [:bzbarsky]]

>+class nsXULPopupManager : public nsIDOMKeyListener,

>+  // returns a weak reference to the popup manager instance
>+  static nsXULPopupManager* GetInstance();

Document that this can return null on OOM or whatnot?  But see more on this below.

>+  // given a menu frame, find the prevous or next menu frame. This is
>+  // used for keyboard navigation between items in a popup menu, if aIsPopup
>+  // is true, or a menubar if aIsPopup is false

I'm not sure I follow....  For the menubar this is hitting right/left and going from, say, "File" to "Edit", right?  What's the equivalent for popups?  Might want to just give an example...

>+  static nsMenuFrame* GetPreviousMenuItem(nsIFrame* aParent,
>+                                          nsMenuFrame* aStart,
>+                                          PRBool aIsPopup);

So if aIsPopup is false, this finds a <menu>, not a <menuitem>, right?  And if it's true, it still finds a <menu>?   Would it make more sense to call this GetPreviousMenu?  Document that aStart can be null and what this means?

Same for the GetNextMenuItem method.

>+  static PRBool IsValidMenuItem(nsPresContext* aPresContext,
>+                                nsIContent* aContent,
>+                                PRBool aOnPopup);

What does the aOnPopup argument mean?  Is it like aParentIsPopup or something, basically?

>+  // inform the popup manager that a menu bar has been activated or deactivated,
>+  // either because one of its menus has opened or closed, or that the menubar
>+  // has been focused such that its menus may be navigated with the keyboard. 
>+  void SetActiveMenuBar(nsMenuBarFrame* aMenuBar, PRBool aActivate);

What should aActivate be when focusing?  Document?

>+  // retrieve the node and offset of the last mouse event used to open a
>+  // context menu.
>+  void GetMouseLocation(nsIDOMNode** aNode, PRInt32* aOffset);

What does "offset" mean here?  For that matter, what does aNode mean?  Document?  Is this basically a range endpoint, effectively?  Saying that would make it clear what's going on.

>+  // set the mouse event that was used to activate the next popup to be opened.
>+  void SetMouseLocation(nsIDOMEvent* aEvent);

This should be called before ShowPopup()?  Always?  Or just when the popup is opened via a mouse event?  Would it make sense to pass an event to ShowPopup or ShowPopupAtScreen (and allow passing null in the cases when we don't need to call SetMouseLocation)?

>+   * Open a popup at a specific screen position specified by aXPos and aYPos.

Which are CSS pixels, right?

>+   * Hide a popup. If the popup is in a <menu>, then also inform the menu that
>+   * it is being hidden.

"it" being the popup?  I'd just make that explicit: "inform the menu that the popup is being hidden".

>+   * aDeselectMenu - true if the <menu> should be deselected.

That's the parent <menu> that the popup is a child of, if any, right?

>+   * aAsynchronous - true if the first popuphiding event should be sent
>+   *                 asynchrously. This should be true if HidePopup is called
>+   *                 from a frame.

Please either make this a required argument or at least default it to PR_TRUE so people can't accidentally add unsafe code to frames?

>+   * Execute a menu command from the triggering event aEvent.
>+   */
>+  void ExecuteMenu(nsIContent* aMenu, nsEvent* aEvent);

Is aMenu expected to be a particular kind of node?  Is aEvent expected to be a particular kind of event?  Document, please?

>+   * Return true if a popup may not be opened. This will return true if the
>+   * popup is already open, the popup is in a content shell that is not

s/open, the popup/open, if the popup/

>+   * focused, or if it is a submenu of another menu that isn't open.
>+   */
>+  PRBool MayNotShowPopup(nsMenuPopupFrame* aFrame);

I'd reverse the sense of this and call it MayShowPopup(); that seems to be a lot clearer to me.

>+   * Hide the chain of popups, but don't remove them yet
...
>+  void HidePopupViews();

Does this hide mCurrentMenu?  Or mPanels?  Or both?  Document?

>+   * Returns true if there is a context menu open. If aPopup is specified,
>+   * then the context menu must be later in the chain than aPopup.
>+   */
>+  PRBool HasContextMenu(nsMenuPopupFrame* aPopup);

Add "If aPopup is null, returns true if any context menu at all is open" to the comment?

>+   * Update the commands for the menus within the menu popup for a given
>+   * content node.
>+  void UpdateMenuItems(nsIContent* aPopup);

Is aPopup expected to be a particular kind of node or to have a particular kind of frame or anything?  e.g., can I pass in the root <window> here?  Document the expectations?

>+   * Stop the timer which hides popups after a delay.
>+  void KillMenuTimer();

Does that effectively cancel a previous HidePopupAfterDelay call?  If so, say so here and document the relationship in the HidePopupAfterDelay docs too?

>+   * Handles navigation for menu accelkeys
>+  PRBool ShortcutNavigation(nsIDOMKeyEvent* aKeyEvent);

Document what the return value means?  Maybe this should be called HandleShortcutNavigation?

Similar for KeyboardNavigation.

>+  // handle keyboard navigation within a menu popup
>+  PRBool KeyboardNavigationInPopup(nsMenuChainItem* item,

Document the retun value?  And maybe call this HandlKeyboardNavigationInPopup?

>+  // set to the currently active menu bar

"if any"

>Index: layout/xul/base/src/nsIMenuParent.h

Do we want to call this nsMenuParent so we don't confuse people about whether it inherits from nsISupports?  Followup, I guess.

>+  // deselects the current item and closes its popup if any, then selects the
>+  // new item aMenuItem, opening its popup if necessary. For menubars, if
>+  // aSelectFirstItem is true, and the new item has a submenu, open the menu
>+  // and select the first item in it.

For menupopups, is aSelectFirstItem just ignored?

>+  // returns true if the menupopup is open
>+  virtual PRBool IsOpen() = 0;

What does it return for menubars?

>+  // returns true if the menubar is currently active
>+  virtual PRBool IsActive() = 0;

What does it returns for menupopups?

>+  // returns true if this is a menu. If false, it is a menuitem
>+  virtual PRBool IsMenu() = 0;

I'm not sure I follow this comment.  |this| is either a nsMenuBarFrame or an nsMenuPopupFrame, right?  And we parent nsMenuFrames.  So where do menuitems come in?

>+  // notify that the menu has been closed and any active state should be cleared
>+  virtual PRBool MenuClosed() = 0;

What does the return value mean here?  Document?  Hard to review code that uses this..

>Index: layout/xul/base/src/nsMenuBarFrame.cpp

> nsMenuBarFrame::SetActive(PRBool aActiveFlag)

>+    // if there is a request to deactivate the menu bar, check to see whether
>+    // there is another menu popup open for the menu bar.

s/another/a/ ?  I don't see what the "first" popup would be in this case...

> nsMenuBarFrame::ToggleMenuActiveState()
>     // Deactivate the menu bar
>     SetActive(PR_FALSE);

That fires a DOM event synchronously and can destroy |this|, right?  This code probably needs to deal...

>+      closeframe->SelectMenu(PR_FALSE);

That fires a DOM event synchronously and can destroy |this| and |closeFrame|, right?  Again, need to deal.

>     SetActive(PR_TRUE);

Same here.

>       firstFrame->SelectMenu(PR_TRUE);

Same here.

>@@ -299,516 +262,172 @@ nsMenuBarFrame::FindMenuWithShortcut(nsI

>+          return (currFrame->GetType() == nsGkAtoms::menuFrame) ?
>+                 (nsMenuFrame *)currFrame : nsnull;

NS_STATIC_CAST please.

>+NS_IMETHODIMP nsMenuBarFrame::SetCurrentMenuItem(nsMenuFrame* aMenuItem)

Why all on one line?

>+  if (mCurrentMenu)
>+    mCurrentMenu->SelectMenu(PR_FALSE);

This can wipe out arbitrary frames including aMenuItem, right?

>+  if (aMenuItem)
>+    aMenuItem->SelectMenu(PR_TRUE);

And then this would crash...

I'm not going to be able to catch all the frame-code callers of SelectMenu, I don't think.  They seem to be all over the place around here.  We need a followup bug to audit every one of them and make sure that the caller bails out as needed out of the whole frame-code callstack if the event SelectMenu fires destroys things.

Similar for SetActive.  Lots of those as well.  I'll comment when I remember to, but we just need to audit them all, including all the things that can end up calling them...

>+nsMenuBarFrame::MenuClosed()
>+  SetActive(PR_FALSE);

That can kill the frame,

>+  if (!mIsActive && mCurrentMenu) {

And then this would crash or whatnot....

>Index: layout/xul/base/src/nsMenuBarFrame.h
>+  virtual PRBool MenuClosed();

Document?

>   // Called to execute a menu item.
>-  NS_IMETHOD Enter();
>+  nsMenuFrame* Enter();

I'm not sure I follow this comment...  Perhaps the comment from the C++ should move here instead?

>+  nsMenuFrame* FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent);

So this method can destroy arbitrary frames, because it calls SetActive, right?  Maybe we should document that if null is returned the whole frame tree might be gone and shouldn't be accessed?  Or something along those lines?  And audit all call chains?

>+  nsCOMPtr<nsIDOMKeyListener> mKeyboardNavigator;

Is this even used anymore?  It looks like nsXULPopupManager handles that now.

>+  nsMenuFrame* mCurrentMenu; // The current menu that is active.

So it might be active but not open, right?  And might be null if none are active?  Or is there always one active?  Document?

>Index: layout/xul/base/src/nsMenuBarListener.cpp

>+nsMenuBarListener::ToggleMenuActiveState()
>+  nsMenuFrame* closemenu = mMenuBarFrame->ToggleMenuActiveState();

Document that this is only called from our event listener methods, which is why this call cannot destroy |this|, since event dispatch itself is holding a ref?  Because this call _can_ kill mMenuBarFrame and make it drop all the non-transient refs to |this|.

>@@ -224,42 +235,42 @@ nsMenuBarListener::KeyPress(nsIDOMEvent*
>+        nsMenuFrame* result = mMenuBarFrame->FindMenuWithShortcut(keyEvent);
>+        if (result) {
>+          mMenuBarFrame->SetActive(PR_TRUE);
>+          result->OpenMenu(PR_TRUE);

That first call can destroy |result|, no?

>Index: layout/xul/base/src/nsMenuBoxObject.cpp

> NS_IMETHODIMP nsMenuBoxObject::OpenMenu(PRBool aOpenFlag)

>+  if (pm && mContent) {
>+    nsIFrame* frame = GetFrame(PR_FALSE);
>+    if (frame) {

GetFrame() will return null if mContent is null, so no need to explicitly check it here.

>+      if (aOpenFlag) {
>+        pm->ShowMenu(mContent, PR_FALSE, PR_FALSE);

So that just posts an event to show the menu... in particular, it does not add it to the mCurrentMenu or mPanels chain in the popup manager, right?

>+            pm->HidePopup(popupFrame->GetContent(), PR_FALSE, PR_TRUE, PR_FALSE);

This, on the other hand, immediately looks at the popup lists.  So if I call openMenu(true) followed immediately by openMenu(false), it sounds like the menu will be opened but then not closed?  Perhaps making a HidePopup call should cancel any pending show events and vice versa?  Or some other way of resolving this problem?

> NS_IMETHODIMP nsMenuBoxObject::GetActiveChild(nsIDOMElement** aResult)
>+    return ((nsMenuFrame *)frame)->GetActiveChild(aResult);

NS_STATIC_CAST, please.

> NS_IMETHODIMP nsMenuBoxObject::SetActiveChild(nsIDOMElement* aResult)
>+    return ((nsMenuFrame *)frame)->SetActiveChild(aResult);

NS_STATIC_CAST.

> NS_IMETHODIMP nsMenuBoxObject::HandleKeyPress(nsIDOMKeyEvent* aKeyEvent, PRBool* aHandledFlag)
>+  nsMenuPopupFrame* popupFrame = ((nsMenuFrame *)frame)->Popup();

NS_STATIC_CAST.

>Index: layout/xul/base/src/nsMenuFrame.cpp
> NS_INTERFACE_MAP_BEGIN(nsMenuFrame)
>-  NS_INTERFACE_MAP_ENTRY(nsIMenuFrame)
>   NS_INTERFACE_MAP_ENTRY(nsIScrollableViewProvider)
>+  NS_INTERFACE_MAP_ENTRY(nsIMenuFrame)

Why?

> nsMenuFrame::SetParent(const nsIFrame* aParent)
>+      mMenuParent = (nsIMenuParent *)NS_STATIC_CAST(nsMenuPopupFrame *, currFrame);

No need for the cast to nsIMenuParent*, right?

>+      mMenuParent = (nsIMenuParent *)NS_STATIC_CAST(nsMenuBarFrame *, currFrame);

Same.

Perhaps factor this loop out into a method, since it's repeated in nsMenuFrame::Init?  Otherwise, the same comments apply there.

> nsMenuFrame::GetFirstChild(nsIAtom* aListName) const
>+  if (nsGkAtoms::popupList == aListName)
>+    return mPopupFrame;

Please leave the curlies that used to be there.

> nsMenuFrame::GetAdditionalChildListName(PRInt32 aIndex) const
>-  if (NS_MENU_POPUP_LIST_INDEX == aIndex) {
>+  if (NS_MENU_POPUP_LIST_INDEX == aIndex)

And definitely leave them here.

>-nsMenuFrame::DestroyPopupFrames(nsPresContext* aPresContext)
>-    frameConstructor->RemoveMappingsForFrameSubtree(curFrame);

Why is removing this ok?  I don't see corresponding changes to the frame constructor...  Or is the point that now GetAdditionalChildListName() returns the popup list so this is ok?

> nsMenuFrame::Destroy()
>+  if (mMenuParent && mMenuParent->GetCurrentMenuItem() == this) {
>+    // yes; tell it that we're going away
>+    mMenuParent->ClearCurrentMenuItem();

Doesn't this have the potential to run script?  Doing that inside a Destroy() method is not good.  In fact, doing anything that might destroy frames insisde Destroy() is not good -- Destroy() is not reentrant, for example.

I know this hack was already here, but we want to fix it for 1.9, imo.  Followup bug, if you want.

>+nsMenuFrame::PopupOpened()
>+    mMenuParent->SetActive(PR_TRUE);
>     // Make sure the current menu which is being toggled on
>     // the menubar is highlighted
>     mMenuParent->SetCurrentMenuItem(this);

After the SetActive() call, |this| might be dead.  So this is unsafe.

> nsMenuFrame::SelectMenu(PRBool aActivateFlag)
>-  if (!mContent) {
>+  if (!mContent)

Please leave the curlies as they were.

>+nsMenuFrame::OpenMenu(PRBool aSelectFirstItem)

>+    // This opens the menu asynchronously
>+    pm->ShowMenu(mContent, aSelectFirstItem, PR_TRUE);

You mean "synchronously", right?

>+nsMenuFrame::CloseMenu(PRBool aDeselectMenu)
>+  // Close the menu synchronously

So this runs script synchronously, right?  But we call it in the middle of a frae tree walk from WalkFramesThroughPlaceholders in the presshell...  That combination sounds bad.

> nsMenuFrame::IsSizedToPopup(nsIContent* aContent, PRBool aRequireAlways)
>-  
>+

For what it's worth, it's better to avoid random whitespace changes like this; otherwise we basically end up with dueling editors.  There have been some others in this diff as well; look through and nix them?

> nsMenuFrame::DoLayout(nsBoxLayoutState& aState)

>+//    if (mLastPref != prefSize) {
...
>+//    }

What's up with that change?  Please put it back the way it used to be, unless there's a really good reason for it.  If there's a reason, take the conditional out entirely, and I'd like to know the reason.

> nsMenuFrame::Notify(nsITimer* aTimer)

>+    // cancel the timer first, as opening the menu below may modify it
>+    mOpenTimer->Cancel();

Why do we need to cancel at all?  Document that part clearly?

>@@ -1433,53 +875,38 @@ nsMenuFrame::UpdateMenuSpecialState(nsPr
>+      if ((menu->GetMenuType() == eMenuType_Radio) &&
>+          (menu->IsChecked()) &&
>+          (menu->GetRadioGroupName(sibGroup), sibGroup == mGroupName)) {      

The first two of those conditions are overparenthesized.

> nsMenuFrame::InsertFrames(nsIAtom*        aListName,
>+    mPopupFrame = (nsMenuPopupFrame *)aFrameList;

NS_STATIC_CAST.

Should this assert that aFrameList->GetNextSibling() is null or something?

> nsMenuFrame::AppendFrames(nsIAtom*        aListName,

Same comments here.

> nsMenuFrame::SetActiveChild(nsIDOMElement* aChild)
>+    mPopupFrame->ChangeMenuItem((nsMenuFrame *)kid, PR_FALSE);

NS_STATIC_CAST.

>@@ -2162,16 +1254,17 @@ NS_IMPL_ISUPPORTS1(nsMenuTimerMediator, 
> nsMenuTimerMediator::~nsMenuTimerMediator()
> {
>+

Why?  I'd leave this as it was.

>Index: layout/xul/base/src/nsMenuFrame.h
>+enum nsMenuType {
>+  eMenuType_Normal = 0,
>+  eMenuType_Checkbox = 1,
>+  eMenuType_Radio = 2

Want to document what these mean, as a followup?

>-  virtual nsIAtom* GetAdditionalChildListName(PRInt32 aIndex) const;
>+  nsIAtom* GetAdditionalChildListName(PRInt32 aIndex) const;

Please leave the "virtual" the way it was, since this _is_ a virtual method.

>+   * NOTE: OpenMenu will open the menu synchronously. Don't call this if a frame
>+   *       is manipulated afterwards without checking to make sure it is still alive.
>+   *       All current calls to OpenMenu do not adjust the frame.
>+   */
>+  void OpenMenu(PRBool aSelectFirstItem);
>+  void CloseMenu(PRBool aDeselectMenu);

That comment applies to CloseMenu too, no?  And you need to make sure none of the callers of your code manipulate frames either.  Probably a followup bug on auditing this...

>+  NS_IMETHOD SetActiveChild(nsIDOMElement* aChild); // @see comment ***

Which comment?

>+  nsMenuFrame* Enter();

What does this do?

>+  void GetRadioGroupName(nsString &aName) { aName = mGroupName; };

Does anyone really need to edit the result?  Could this just have no args, a |const nsString&| return value, and return mGroupName?  It looks like that would also simplify callers...

>+  nsMenuPopupFrame* Popup() { return mPopupFrame; }

Is this guaranteed to not be null?  If not, please call it GetPopup.

>+  void ToggleMenuState();

Document that this can destroy the frame tree?

>+  void PopupOpened();
>+  void PopupClosed(PRBool aDeselectMenu);

What do these methods do?  When should they be called?  By whom?  Document that they can destroy the frame tree?

>+  void MenuStateChanged(PRBool aOpen);

Who should be calling this, and when?  How is this different from PopupOpened/PopupClosed?

>+  // returns true if the menu is in another menu 
>+  // a menu is a submenu if it has a parent menubar, popup or menupopup
>+  PRBool IsOnMenu() { return mMenuParent && mMenuParent->IsMenu(); }

But nsMenuBarFrame::IsMenu() returns false...  So I'm not sure how this code matches the comment.

>Index: layout/xul/base/src/nsMenuPopupFrame.cpp

>+class nsXULPopupShownEvent : public nsRunnable
>+  NS_IMETHOD Run()
>+    nsMouseEvent *event = new nsMouseEvent(PR_TRUE, NS_XUL_POPUP_SHOWN,
>+                                           nsnull, nsMouseEvent::eReal);
>+    nsEventDispatcher::Dispatch(mPopup, mPresContext, event);                 

That leaks the event.  You want to stack-allocate it.

>+nsMenuPopupFrame::AdjustView()
>+    viewManager->UpdateView(view, rect, NS_VMREFRESH_IMMEDIATE);

Do we really need that?  If so, why are we doing it before the SetViewVisibility() call?  I'd suspect we can remove this call altogether, frankly.

>+LazyGeneratePopupDone(nsIContent* aPopup, nsIFrame* aFrame, void* aArg)

So by this point the child frames have been created but their XBL binding constructors have not run yet.  Is this a problem?

>+    if (weakFrame.IsAlive()) {
>+      // XXXndeakin fix this

Followup bug, cite the number in the comment?

>+nsMenuPopupFrame::ShowPopup(PRBool aIsContextMenu, PRBool aSelectFirstItem)

>+      (NS_STATIC_CAST(nsMenuFrame*, parent))->PopupOpened();
>+      PresContext()->RootPresContext()->NotifyAddedActivePopupToTop(this);

After the PopupOpened call, |this| might be dead.  So this is unsafe.

>+      // XXXndeakin fix this
>+      AddStateBits(NS_FRAME_HAS_DIRTY_CHILDREN);

Again, cite the bug#.

>+nsMenuPopupFrame::HidePopup(PRBool aDeselectMenu)
>+    (NS_STATIC_CAST(nsMenuFrame*, parent))->PopupClosed(aDeselectMenu);
>+    PresContext()->RootPresContext()->NotifyRemovedActivePopup(this);

After the PopupClosed call, |this| might be dead.  So this is unsafe.

> nsMenuPopupFrame::GetRootViewForPopup(nsIFrame* aStartFrame,
>+        if (wtype == eWindowType_popup)
>+          return view;

Please leave the curlies here.

>+      if (aStopAtViewManagerRoot && view == rootView)
>+        return view;

Same.

>+// XXXndeakin this function will be reworked in a followup bug such that positioning

Cite the bug number here, please.

>+nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
>+      nsIPresShell *shell = document->GetShellAt(0);

Why not use the presshell for this->PresContext() ?  It'll be guaranteed non-null too, right?

>+    sizedToPopup = (nsMenuFrame::IsSizedToPopup(aAnchorFrame->GetContent(), PR_FALSE));

Take out those outermost parens?  They just confuse things, imo.

>-  if (sizedToPopup) {
>+  if (sizedToPopup)

Please leave the curlies.

I'm assuming all the coordinate system stuff in this method got moved with no changes.  If I'm wrong and it needs to be reviewed, please let me know.

>@@ -816,29 +1009,29 @@ nsMenuPopupFrame::SyncViewWithFrame(nsPr
>-    
>+

Please leave out whitespace-only changes like this if possible?  Similar in other places in this patch where it looks like your editor settings just disagree with someone else's editor settings.

>-    if(mRect.width > screenWidthTwips) 
...
>+    if (mRect.width > screenWidthTwips)
...

And here.

>+NS_IMETHODIMP nsMenuPopupFrame::SetCurrentMenuItem(nsMenuFrame* aMenuItem)

>+  if (mCurrentMenu)
>     mCurrentMenu->SelectMenu(PR_FALSE);

Please put the pre-existing curlies back.  And this call can kill |this|, and more importantly kill aMenuItem, which leads to this function being exploitable.

>     aMenuItem->SelectMenu(PR_TRUE);

That can kill |this|.

>   mCurrentMenu = aMenuItem;

In which case this line crashes.

>+NS_IMETHODIMP nsMenuPopupFrame::ChangeMenuItem(nsMenuFrame* aMenuItem,
>+    mCurrentMenu->SelectMenu(PR_FALSE);

After which the frame tree might be gone.

>+    nsMenuPopupFrame* popup = mCurrentMenu->Popup();

So this crashes.

>+    aMenuItem->SelectMenu(PR_TRUE);

This can kill the frame tree too.

>+  mCurrentMenu = aMenuItem;

And then this crashes.

> nsMenuPopupFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent, PRBool& doAction)
>+              frameBefore = (nsMenuFrame *)currFrame;

NS_STATIC_CAST.

>+              frameAfter = (nsMenuFrame *)currFrame;

Same.

>Index: layout/xul/base/src/nsMenuPopupFrame.h

>+// values are selected so that they can be flipped by direction just by

"...so that the direction can be flipped just by..."

>+  virtual void ClearCurrentMenuItem()
>+  {
>+    mCurrentMenu->SelectMenu(PR_FALSE);
>+    mCurrentMenu = nsnull;

Crash, if SelectMenu destroyed |this|.  And of course callers need to deal too...

In general, there's no benefit in putting the implementation of virtual methods in the header like this; it just creates the false impression that they're inlined.  I'd move this to the .cpp; this applies to some other methods in this class too.

Document that SetCurrentMenuItem and ChangeMenuItem can wipe out the frame tree?  We'll need to audit callers of those too...

>+  PRBool ConsumeOutsideClicks();

Document?

>+  nsresult PopupNeedsRelayout(nsBoxLayoutState& aState);

Document?  Though I don't see it anywhere else in this patch; perhaps this line should just go away?

>+  // set the position of the popup either relative to the anchor or at a
>+  // specific point. It will be adjusted so that it is on screen.
>+  nsresult SetPopupPosition(nsIFrame* aFrame);

What's aFrame?

>+  nsMenuFrame* Enter();

What does this do?

>+  nsMenuFrame* FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent, PRBool& doAction);

What does the last arg mean?

>+  // the position of the popup. The screen coordinates, if set override

So being "not set" means "set to -1"?  Document that?

>Index: layout/xul/base/src/nsPopupBoxObject.cpp
> nsPopupBoxObject::MoveTo(PRInt32 aLeft, PRInt32 aTop)
>-  if (menuPopupFrame) {
>+  if (menuPopupFrame)

Please leave the curlies.

> nsPopupBoxObject::EnableRollup(PRBool aShouldRollup)
>+  // this does nothing any more

s/any more/now/

>Index: layout/xul/base/src/nsPopupSetFrame.cpp

> nsPopupSetFrame::Destroy()

>+  // remove each popup from the list as we go. This
>+  // keeps things consistent so reentering won't crash us

Reentering what?  Reentering nsIFrame::Destroy() is generally bad.  In particular, nsFrame::Destroy does some stuff with members of the frame, then deallocates it.  If this runs twice, life is bad.  Very bad.  Did I mention bad?  ;)

In particular, the places in this code that have weakFrame stuff in Destroy() methods (I'm looking at nsMenuFrame::Destroy) are pretty suspect unless they bail out of all the Destroy() methods that need to be aborted...

> nsPopupSetFrame::AddPopupFrame(nsIFrame* aPopup)
>+  if (aPopup->GetType() != nsGkAtoms::menuPopupFrame)
>+    return NS_ERROR_UNEXPECTED;

Can that really ever happen?  If not, please assert here in addition to returning?

>+  entry->mPopupFrame = (nsMenuPopupFrame *)aPopup;

NS_STATIC_CAST.

>Index: layout/xul/base/src/nsXULPopupManager.cpp

>+nsXULPopupManager::GetInstance()
>+{
>+  if (!sInstance) {
>+    sInstance = new nsXULPopupManager();
>+    NS_IF_ADDREF(sInstance);

So we basically leak nsXULPopupManager?  I'd really rather we avoided that.  Perhaps we could set it up during layout module init and tear it down during layout module shutdown?  Or something along those lines?

I'm up to nsXULPopupManager::GetSubmenuWidgetChain.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> +nsXULPopupManager::~nsXULPopupManager()

Could we assert that mCurrentMenu and mPanels are null here?

>+nsXULPopupManager::GetSubmenuWidgetChain(nsISupportsArray **_retval)
>+  NS_NewISupportsArray(_retval);

Check for allocation failure?

>+nsXULPopupManager::GetMenuFrameForContent(nsIContent* aContent)
>+    document->FlushPendingNotifications(Flush_OnlyReflow);

Why Flush_OnlyReflow?  I guess we don't want to flush restyles from ShowMenu() if aAsynchronous is true.  But then why bother flushing reflow?

>+nsXULPopupManager::GetPopupFrameForContent(nsIContent* aContent)

I almost wonder whether it makes sense to have a helper method that returns the nsIFrame and share that code between GetMenuFrameForContent and GetPopupFrameForContent.

>+nsXULPopupManager::SetMouseLocation(nsIDOMEvent* aEvent)
>+  if (uiEvent) {

Should we set them to null and 0 respectively if not?  And probably assert or something?

>+nsXULPopupManager::ShowPopupCallback(nsIContent* aPopup,
>+      nsMenuFrame* menuFrame = (nsMenuFrame *)parent;

NS_STATIC_CAST.

>+nsXULPopupManager::HidePopup(nsIContent* aPopup,

>+    // at this point, item will be set to the found item in the list. If item
...
>+    // ... To do this, we start at mCurrentMenu and
>+    // close each popup in turn until aPopup is reached.

That's not what the code does.  Is a loop missing somewhere here?  Or is the idea that FirePopupHidingEvent takes care of this recursion?  If so, you might want to make that clear in this comment.

>+nsXULPopupManager::HidePopupCallback(nsIContent* aPopup,

>+  aPopupFrame->HidePopup(aDeselectMenu);

After this call, aPopupFrame might be dead.

>+  nsEventDispatcher::Dispatch(aPopup, aPopupFrame->PresContext(),
>+                              &event, nsnull, &status);

And then this crashes.

>+      item->Detach(&mCurrentMenu);

Detach() should probably document that it doesn't change the item's parent, just the linkage of things around the item...

>+nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)

>+      item->Frame()->HidePopup(PR_TRUE);

This call can delete |item|.

>+    item = item->Parent();

And then this is bad.

Same for the other loop in this method.

>+nsXULPopupManager::ExecuteMenu(nsIContent* aMenu, nsEvent* aEvent)

>+    nsMenuChainItem* next = item->Parent();
>+    item->Frame()->HidePopup(PR_TRUE);
>+    item = next;

The HidePopup() call can delete |next|...  Then this crashes.

>+nsXULPopupManager::FirePopupShowingEvent(nsIContent* aPopup,
>+                                         nsIContent* aMenu,
>+                                         nsMenuPopupFrame* aPopupFrame,

So we don't _really_ need aPopupFrame in this method, right?  Just the presshell and prescontext?  Would it make more sense to pass in a prescontext to this method?  Then we could eliminate getting the frame in at least one place...

>+  nsPresContext* context = aPopupFrame->PresContext();

This should probably be a strong ref.

>+  // XXXndeakin eventually, the popup events will be a different event type

Is there a bug number on this?

>+nsXULPopupManager::FirePopupHidingEvent(nsIContent* aPopup,
>+  // get frame again in case it went away

For what it's worth, in this case and similar ones... Do we still want to do HidePopupCallback?  Even though there may now be an open popup from for this popup content?  I don't really care how we do this as long as we don't crash, I guess...

>+nsXULPopupManager::MayNotShowPopup(nsMenuPopupFrame* aPopup)
>+    nsCOMPtr<nsIDocShell> shell = do_QueryInterface(dsti);
>+    if (!shell)
>+      return PR_TRUE;

Why?  You never use |shell| anywhere that I can see...

>+  // cannot open a popup that is a submenu of a menupopup that isn't open.

What about a popup that is living inside a menupopup but is not a submenu (e.g. someone stuck a menulist inside a menupopup, as people are wont to do)?

>+nsXULPopupManager::PopupDestroyed(nsMenuPopupFrame* aPopup)
>+        item->Frame()->HidePopup(PR_FALSE);

This might kill menuToDestroyFrame, after which continuing this loop will be bad...

>+nsXULPopupManager::UpdateMenuItems(nsIContent* aPopup)
>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsIContent *grandChild = aPopup->GetChildAt(i);

So the good news is that GetChildAt() handles out-of-bounds access.  The bad news is that you might miss kids if a mutation listener twiddles the DOM...  Not much I can think of to fix that, though.

But for sure you need to hold a strong ref to grandChild here.

>+nsXULPopupManager::ShortcutNavigation(nsIDOMKeyEvent* aKeyEvent)

>+    nsMenuFrame* result = currentPopup->FindMenuWithShortcut(aKeyEvent, action);

>+    if (result) {
>+      currentPopup->ChangeMenuItem(result, PR_FALSE);

This might destroy |result|.

>+      if (action)
>+        result->Enter();

And then this crashes.

>+      mActiveMenuBar->SetActive(PR_TRUE);

That can destroy |result|.

>+      result->OpenMenu(PR_TRUE);

And then this crashes.

>+nsXULPopupManager::KeyboardNavigation(PRUint32 aKeyCode)
>+      nsIMenuParent* expectedParent = (nsIMenuParent *)nextitem->Frame();

NS_STATIC_CAST.

>+nsXULPopupManager::KeyboardNavigationInPopup(nsMenuChainItem* item,
>+nsXULPopupManager::GetNextMenuItem(nsIFrame* aParent,
>+             (nsMenuFrame *)currFrame : nsnull;

NS_STATIC_CAST.

>+             (nsMenuFrame *)currFrame : nsnull;

And here.

>+nsXULPopupManager::GetPreviousMenuItem(nsIFrame* aParent,
>+             (nsMenuFrame *)currFrame : nsnull;

And here

>+             (nsMenuFrame *)currFrame : nsnull;

And here.

>+nsXULPopupManager::IsValidMenuItem(nsPresContext* aPresContext,

>+  nsIAtom *tag = aContent->Tag();

Do we care to verify that aContent is XUL?  The old code didn't, but is that correct?

>+  if (!((tag == nsGkAtoms::menu ||
>+         tag == nsGkAtoms::menuitem) ||
>+        (aOnPopup && tag == nsGkAtoms::option)))

This might be easier to read (fewer parens and logical operations, sorta) as:

  if (tag != nsGkAtoms::menu &&
      tag != nsGkAtoms::menuitem &&
      (!aOnPopup || tag != nsGkAtoms::option))

>+  return (!skipNavigatingDisabledMenuItem ||
>+          !aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::disabled,
>+                                 nsGkAtoms::_true, eCaseMatters));

Whereas this _might_ be clearer as:

  return !(skipNavigatingDisabledMenuItem &&
           aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::disabled,
                                 nsGkAtoms::_true, eCaseMatters));

but either way.


>+nsXULPopupManager::KeyUp(nsIDOMEvent* aKeyEvent)
>+  return NS_ERROR_BASE; // I am consuming event

Ugh.  I know this is what the code used to do, but file a followup for a somewhat sane rv here?

>+nsXULPopupManager::KeyPress(nsIDOMEvent* aKeyEvent)
>+      ShowMenu(menuToOpen->GetContent(), PR_TRUE, PR_FALSE);

This probably needs to hold a strong ref to the content, as do other sync callers of ShowMenu()...

>+NS_NewXULPopupManager(nsISupports* aOuter, REFNSIID aIID, void **aResult)

This is unused if we remove the nsLayoutModule stuff.

>+nsXULPopupShowingEvent::Run()

The "get the frame" code here that's duplicated in so many places should really be shared somehow.

>+nsXULPopupHidingEvent::Run()

Same.

>+nsXULMenuCommandEvent::Run()

>+  pm->Rollup();

This might roll up the wrong menu if the event killed some menus, but I guess that's ok, right?

>Index: layout/xul/base/src/nsXULTooltipListener.cpp
>@@ -155,16 +155,21 @@ nsXULTooltipListener::MouseOut(nsIDOMEve
>+    nsCOMPtr<nsIDOMMouseEvent> me = do_QueryInterface(aMouseEvent);
>+    nsCOMPtr<nsIDOMEventTarget> related;
>+    me->GetRelatedTarget(getter_AddRefs(related));
>+
>+    nsCOMPtr<nsIContent> targetContent = do_QueryInterface(related);

What's the point of this chunk of code?  It seems to eb a complicate no-op.

>@@ -651,21 +626,18 @@ nsXULTooltipListener::GetTooltipFor(nsIC
>+    if (frame && frame->GetType() == nsGkAtoms::menuFrame)
>+      return NS_ERROR_FAILURE;

Might still want to warn....

Again, I'm skipping the toolkit and xpfe changes on the assumption that domain experts will review them.

I'm also not verifying that "panel" got added to all the places it should have been, in CSS/JS/C++/whatever.  I'm assuming you already did that (checked all hits for ::popup/::menupopup/::tooltip, "popup"/"menupopup"/"tooltip", <popup/<xul:popup etc in LXR).

That's all over here.  I'm really sorry it took so long; sorting through all the events and attr changes just took forever, and I had to do it anew every time I sat down to review more of the patch.  :(

Comment 62

[Neil Deakin]

> 
> And why not just nsIDOMEventListener* here?  I guess this simplifies the code
> below?  If so, leaving this as an nsCOMPtr sounds fine by me.

Makes it simpler yes

> >+    // |mElement| is the node to which this listener is attached.
> >+    nsIDOMElement* mElement;               // Weak ref. The element will go away first.
> >+
> >+    // The popup that is getting shown on top of mElement.
> >+    nsCOMPtr<nsIContent> mPopupContent; 
> 
> Could those end up pointing to the same element?  If so, would that leak?  Or
> could we have two listeners, set as event listeners on two different elements
> and pointing to each other's targets via mPopupContent?
> 
> It looks like nsGenericElement::Traverse already traverses the popup and
> context menu listeners; would it make sense to make this class participate in
> cycle collection and just use strong refs here?

yes it would, as a cycle is possible if two listeners point to each other

> So what happens if the "sizetopopup" attribute is set dynamically?  Or if a
> menupopup without this attribute has a child appended via the DOM?  We'll
> construct a frame for that child; then what happens when we open the menupopup?
>  It looks like we won't construct frames for anything else, right?
> 
> Would it make more sense to move this logic into the relevant frames' IsLeaf()
> methods so that we're consistent about when we do want to construct child stuff
> for them and when we don't?

Currently, this works because the frame construction stops after finding no frame for the insertion point. This doesn't seem like a good assumption so I'll move this to use IsLeaf.

> >+    nsFrameConstructorState state(mPresShell, nsnull, nsnull, nsnull);
> 
> So this will give inconsistent handling of positioned and floating kids with
> popups that construct eagerly, no?  I guess we might not care (or only care in
> a followup bug)...
> 

Yeah, positioned content does work in in XUL, and floats aren't really used, so a follow up bug would be suitable here

> Which screen is used on a multi-monitor system?

This is a underlying widget issue, but on my system it means on the monitor that contains the largest portion of the containing window.


> 
> Do we really want the two separate setters?  Wouldn't it be the case that
> mParent->mChild == this always?  So just calling SetParent() on a menu should
> be enough, if SetParent() looks like:

Will move into a single setter

> >+   * Hide the chain of popups, but don't remove them yet
> ...
> >+  void HidePopupViews();
> 
> Does this hide mCurrentMenu?  Or mPanels?  Or both?  Document?
> 

Isn't actually used, so it should be removed

> >+  void MenuStateChanged(PRBool aOpen);
> 
> Who should be calling this, and when?  How is this different from
> PopupOpened/PopupClosed?

Also not needed

>+nsXULMenuCommandEvent::Run()
>+  pm->Rollup();
> 
> This might roll up the wrong menu if the event killed some menus, but I guess
> that's ok, right?

That's OK, the old code did this too. It also isn't a big deal if someone messed with the menus during a command event.

> >+    // This opens the menu asynchronously
> >+    pm->ShowMenu(mContent, aSelectFirstItem, PR_TRUE);
> 
> You mean "synchronously", right?

No, true means asynchronous

> >+nsXULPopupManager::FirePopupHidingEvent(nsIContent* aPopup,
> >+  // get frame again in case it went away
> 
> For what it's worth, in this case and similar ones... Do we still want to do
> HidePopupCallback?  Even though there may now be an open popup from for this
> popup content?  I don't really care how we do this as long as we don't crash, > I guess...

Not sure what is meant here.

> >+nsXULPopupManager::MayNotShowPopup(nsMenuPopupFrame* aPopup)
> >+    nsCOMPtr<nsIDocShell> shell = do_QueryInterface(dsti);
> >+    if (!shell)
> >+      return PR_TRUE;
> 
> Why?  You never use |shell| anywhere that I can see...

It's used in the next line:
  nsCOMPtr<nsPIDOMWindow> win = do_GetInterface(shell);

> >+  // cannot open a popup that is a submenu of a menupopup that isn't open.
> 
> What about a popup that is living inside a menupopup but is not a submenu (e.g.
> someone stuck a menulist inside a menupopup, as people are wont to do)?

The popup won't be opened either, but the code doesn't need to do anything specific for this here, or am I missing something?

> >+LazyGeneratePopupDone(nsIContent* aPopup, nsIFrame* aFrame, void* aArg)
> 
> So by this point the child frames have been created but their XBL binding
> constructors have not run yet.  Is this a problem?

Don't think so, in fact I think we want the initialization this code does to happen before XBL gets applied.

> >+  // set the mouse event that was used to activate the next popup to be opened.
> >+  void SetMouseLocation(nsIDOMEvent* aEvent);
> 
> This should be called before ShowPopup()?  Always?  Or just when the popup is
> opened via a mouse event?  Would it make sense to pass an event to ShowPopup or
> ShowPopupAtScreen (and allow passing null in the cases when we don't need to
> call SetMouseLocation)?

It's a workaround for the spellcheck code needing to get the node and offset of the mouse event. I hope to remove it or change it later so I don't want to add any additional arguments until I've worked this out. This would be part of bug 383930. Added a comment to that effect.

> >Index: layout/xul/base/src/nsIMenuParent.h
> 
> Do we want to call this nsMenuParent so we don't confuse people about whether
> it inherits from nsISupports?  Followup, I guess.

Will file a followup bug

> >+  NS_IMETHOD SetActiveChild(nsIDOMElement* aChild); // @see comment ***
> 
> Which comment?

There's a comment at the top of the class declaration about this. I'll add an 'above' qualifier

> I'm assuming all the coordinate system stuff in this method got moved with no
> changes. If I'm wrong and it needs to be reviewed, please let me know.

The only changes were in the the if (mScreenXPos == -1 && mScreenYPos == -1) block and the else clause.

> >+nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame)
> >+      nsIPresShell *shell = document->GetShellAt(0);
> 
> Why not use the presshell for this->PresContext() ?  It'll be guaranteed
> non-null too, right?

mAnchorContent may be in a different document though.

> >+  // remove each popup from the list as we go. This
> >+  // keeps things consistent so reentering won't crash us
> 
> Reentering what?  Reentering nsIFrame::Destroy() is generally bad.  In
> particular, nsFrame::Destroy does some stuff with members of the frame, then
> deallocates it.  If this runs twice, life is bad.  Very bad.  Did I mention
> bad?  ;)

I don't think this comment applies any more, so i'll remove it

> In particular, the places in this code that have weakFrame stuff in Destroy()
> methods (I'm looking at nsMenuFrame::Destroy) are pretty suspect unless they
> bail out of all the Destroy() methods that need to be aborted...

I will remove the call to SelectMenu here

> >-nsMenuFrame::DestroyPopupFrames(nsPresContext* aPresContext)
> >-    frameConstructor->RemoveMappingsForFrameSubtree(curFrame);
> 
> Why is removing this ok?  I don't see corresponding changes to the frame
> constructor...  Or is the point that now GetAdditionalChildListName() returns
> the popup list so this is ok?

Yes, I verified that without this change the popup's frame tree gets traversed twice to remove the mappings

I have addressed all the other comments for the next patch except the crashing after dom changes crashes. I had originally planned on finding a way to get accessibility to work without needing menu events, but I now see that that won't work, (also other places seem to use the events). I'll spend some time trying to think of a decent solution without having to use a pile of weakframe objects.

Comment 63

[Boris Zbarsky [:bzbarsky]]

> This is a underlying widget issue

Document that the screen for the window in question (however it's related to this API) is used?  Which screen that is is a widget issue, as you say.

> It also isn't a big deal if someone messed with the menus during a command
> event.

Agreed.

> No, true means asynchronous

Indeed.  Sorry about that.  :(

> Not sure what is meant here.

If while the runnable to fire pophiding is pending the frame goes away, so we close the popup, do we still want to fire the HidePopupCallback?  Like I said, I think it's fine either way, so the current code is fine.

> It's used in the next line:
>  nsCOMPtr<nsPIDOMWindow> win = do_GetInterface(shell);

That could just as easily be:

  nsCOMPtr<nsPIDOMWindow> win = do_GetInterface(dsti);

was my point.

> The popup won't be opened either

Why not?  It won't have a menu parent, will it?  Or will the menu parent be this random menu that it's not really a popup for?

> I hope to remove it or change it later so I don't want to add any additional
> arguments

Sounds good.  Thanks for adding the relevant bug # to the comment.

> There's a comment at the top of the class declaration about this.

I'm being really blind here... I don't see it being added to the patch, and it's not already in nsMenuFrame.h, is it?

> mAnchorContent may be in a different document though.

Oh, good point.  Document this, please?

And perhaps in general document in the relevant APIs that the two content nodes might not be in the same document?

As for the accessibility events... Is firing them when we fire popupshowing/popuphiding too late?

Comment 64

[Neil Deakin]

(In reply to comment #63)
> Why not?  It won't have a menu parent, will it?  Or will the menu parent be
> this random menu that it's not really a popup for?

A menulist inside a popup will have the popup as a menuparent.

> I'm being really blind here... I don't see it being added to the patch, and
> it's not already in nsMenuFrame.h, is it?

It is already there in nsMenuFrame.h The lines with the 'see comment' haven't changed, just moved. They were added in bug 348304.

> As for the accessibility events... Is firing them when we fire
> popupshowing/popuphiding too late?

The DOMMenuItemActive/Inactive events need to fire as the items are rolled over, so the popupshowing/popuphiding events won't work here.

Not sure, but maybe firing events asynchronously is possible, or at least queuing them and firing them at a later point (such as within a caller). I suspect that at least some of the calls can be done this way.

Aaronlev, what are the reasons for using synchronous DOMMenuItemActive/Inactive events here?

Comment 65

[Neil Deakin]

I think the main reason for synchronous events is to fire the accessibility events before a menu opens a modal dialog, otherwise screenreaders get confused thinking that the menu is still open. That isn't an issue with this patch as the menu's command event is changed to fire asynchronously. So maybe we can just change the DOMMenuItemActive/Inactive events to be asynchronous as well.

Comment 66

[Boris Zbarsky [:bzbarsky]]

> They were added in bug 348304.

You mean the @note thing?  It wasn't at all clear to me that this comment was referring to this.  We need a better way to make it clear.  How about just explicitly commenting before all such methods: "This method can execute arbitrary script"?

Going to wait on Aaron's response to the rest.  Thanks for digging into it!

Comment 67

[Neil Deakin]

Attachment: Updated patch

I'll add the cycle collection mentioned above in a followup bug

Comment 68

[Neil Deakin]

Comment on attachment 269055 [details] [diff] [review]
Updated patch

Gavin could review the browser/toolkit autocomplete stuff I'm assuming, or maybe Neil would like to take a look

Comment 69

[Neil Deakin]

Comment on attachment 269055 [details] [diff] [review]
Updated patch

Aarin, could you review the accessibility changes here?

Comment 70

[Asaf Romano (gone)]

1) The last patch doesn't build on windows with a11y enabled
2) Autocomplete in the location bar is broken (at least on mac)

Comment 71

[Asaf Romano (gone)]

3) tooltips are in some sort of one-word-per-line mode.

Comment 72

[Aaron Leventhal]

Comment on attachment 269055 [details] [diff] [review]
Updated patch

Ginn, can you review the accessibility changes here?

Comment 73

[Neil Deakin]

Comment on attachment 269055 [details] [diff] [review]
Updated patch

Boris won't be able to get to this, but says that after the xpfe/toolkit/accesibility parts are reviewed, and if roc has time to take a look, that would be great, otherwise we can land this and he can do a more complete review afterwards.

Unfortunately, I'm at the Paris Developer Day until Monday and I'm going through some personal issues, so I'm not sure how much time I'll have to work on this until then.

Comment 74

[Neil Deakin]

Mano: I don't see any issues with autocomplete on Mac.

Comment 75

[Ginn Chen]

Comment on attachment 269055 [details] [diff] [review]
Updated patch

r=me for accessible/src/base/nsRootAccessible.cpp

I think it's needed.
+      NS_ENSURE_TRUE(menuFrame, NS_ERROR_FAILURE); // XXXndeakin is this line needed?

Comment 76

[Neil Deakin]

Mano: Seems that removing the hack added in bug 262651 to browser.js causes it to work but then the box size is too large when wrapping a tooltip.

What is the build error on Windows?

Comment 77

[Neil Deakin]

Neil (Rashbrook), are you OK with reviewing the browser/toolkit/autocomplete parts here?

Comment 78

[[e-mail]]

(In reply to comment #77)
>Neil are you OK with reviewing the browser/toolkit/autocomplete parts here?
Yeah, they look fine to me.

Comment 79

[Asaf Romano (gone)]

@Neil Deakin:

# Do you still set the |open| attribute? I don't see it set for my panel. Note various front-end code rely on this attribute for checking whether or not a popup/menupopup is opened. I also couldn't find a reliable replacement for it.

# I cannot reproduce the mac autocomplete issues.

# Unfortunately I don't have the build log.

Comment 80

[Asaf Romano (gone)]

At least on windows, setting the noautohide attribute on a panel makes its contents not focusable.

Comment 81

[Ginn Chen]

(In reply to comment #75)
> (From update of attachment 269055 [details] [diff] [review])
> r=me for accessible/src/base/nsRootAccessible.cpp

And
virtual PRBool IsOnActiveMenuBar should be defined in layout/xul/base/public/nsIMenuFrame.h
nsRootAccessible.cpp should include "nsIMenuFrame.h"

Comment 82

[Neil Deakin]

(In reply to comment #79)
> @Neil Deakin:
> 
> # Do you still set the |open| attribute? I don't see it set for my panel. Note
> various front-end code rely on this attribute for checking whether or not a
> popup/menupopup is opened. I also couldn't find a reliable replacement for it.
> 

The open attribute is still set on the <menu> when its popup is open, as before. I'm not clear what this has to do with panels.

Comment 83

[Neil Deakin]

Attachment: Patch for checkin

This is the patch to checkin. I fixed the accessibility build issue. Also, I'm working on some tests for menus and popups. Not sure if I'm brave enough to check in just a short time before alpha6, or whether it would be better to wait until after.

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you'd better check it in after alpha6.

Comment 85

[Neil Deakin]

Will check this in just after alpha6.

Comment 86

[Mike Schroepfer]

Neil - is there any automated test coverage of this patch?

Comment 87

[Neil Deakin]

(In reply to comment #86)
> Neil - is there any automated test coverage of this patch?
> 

Yes, I have automated tests ready, but still need to test them on Linux.
 

Comment 88

[Neil Deakin]

Attachment: work in progress: testcases

Still some work to be done on Windows/Linux but the tests are mostly passing

Comment 89

[Neil Deakin]

Checked in!

Comment 90

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix bustage on x86_64 Linux

Comment 91

[Alfred Kayser]

So, why is <popup> renamed to <panel>?
This breaks compatibility with older versions of mozilla/firefox/etc...

Looking at the comments about <panel> versus <popup> versus <toolkit>, I can only conclude that <panel> is a <popup> without menu, but we have already have <menupopup>. Why not just making <popup> to be anything but <menupopup>

Furthermore we have the discussion of tooltip versus panel. Tooltip clearly has different styling, but same behaviour as panel.

Is there may some hierarchy to this?

<popup>:
  <tooltip>: a tooltip styled popup
  <menupopup>: a popup with menu contents
  <panel>: any other popup 'panel'
Or just:
<popup>: any popup (incl. normal panels...)
  <tooltip>: a tooltip styled popup
  <menupopup>: a popup with menu contents

Comment 92

[Neil Deakin]

<popup> and <menupopup> are equivalent and designed for popups that contain menus. <popup> is deprecated and has always been so (see xul.css) <panel> is a new element designed for popups which are not menus. This type of popup was not supported before this patch. A new element is used to avoid compatibility issues.
 

Comment 93

[Boris Zbarsky [:bzbarsky]]

Attachment: Include the stuff we use (checked in)

Without this, I get build bustage because of local changes to not #include nsStyleSet.h in nsLayoutUtils.h.  If we're using the binding manager, we should just include the header.

Comment 94

[[e-mail]]

Attachment: Fix shortcut navigation opening a submenu

Comment 95

[Alfred Kayser]

Theme authors could have used some warning that <panel> was coming, replacing the depreciated <popup>.

And a small issue report:
In Tools/Options/Contents/Colors, click on a colorpicker to open the colors panel (with updated Walnut theme this looks nice!), and the click again on the same colorpicker button to close it again. So far so good. Click again, and you would expect to pop it again, but no ...  For other popups this works, so may be the colorpicker needs some specific attention?

Comment 96

[Ryan VanderMeulen [:RyanVM]]

Alfred, I filed bug 387097 for the color picker issue

Comment 97

[Neil Deakin]

> Theme authors could have used some warning that <panel> was coming,
> replacing the depreciated <popup>.

<panel> is a new element. It does not replace <popup>.

To others: please file any new patches as different bugs.

Comment 98

[Doug Wright]

Neil - could this have caused bug 387111?

Comment 99

[Wayne Woods]

It seems like this might have caused bug 389542 on Mac.

Comment 100

[Boris Zbarsky [:bzbarsky]]

Doing the followup review would have been a lot easier if the replies to the comments in the bug matched the original comment order (and the patch order).  :(

Anyway, here are the remaining issues (ignoring the stuff I think you've fixed since):

>Index: content/xul/content/src/nsXULPopupListener.cpp
>@@ -245,17 +166,17 @@ XULPopupListenerImpl::PreLaunchPopup(nsI

>-  if (!xulDocument) {
>-    NS_ERROR("Popup attached to an element that isn't in XUL!");
>+  if (!xulDocument)
>     return NS_ERROR_FAILURE;
>-  }

Please leave the curlies the way they were.

> // LaunchPopup
...
>+// aTargetContent is the target of the mouse event aEvent that triggered the
>+// popup. mElement is the element that the popup menu is attached to. The
>+// former may be equal to mElement or it may be a descendant.

Perhaps "aTargetContent may be equal ..."?

>Index: content/xul/content/src/nsXULPopupListener.h

It looks like nsXULPopupListener still doesn't participate in cycle collection.  Please file a followup bug on getting that done for 1.9.

>Index: layout/xul/base/public/nsIPopupBoxObject.idl
>+   * For an anchored popup, the x and y arguments may be used to offset the 
>+   * popup from its anchored position by some number, measured in CSS pixels.

s/number/distance/, please.  And perhaps say that x increases to the right and y increases down?

>Index: layout/xul/base/public/nsXULPopupManager.h
>+ * nsNavigationDirection: an enum expressing navigation through the menus in

Did a followup bug to improve this stuff a bit ever get filed?

>+class nsXULPopupManager : public nsIDOMKeyListener,

>+  // Items that not valid, such as non-menu or menuitem elements are skipped,

"that are not valid, such as non-menu or non-menuitem elements"

>+  // If aStart is null, the first valid item is retrieved for GetNextMenuItem
>+  // or the last valid item for GetPreviousMenuItem is used.

Maybe more like:  "..., the first valid item is retrieved by GetNextMenuItem and the last valid item is retrieved by GetPreviousMenuItem".

>+  // aStart - the menu/menuitem to start navigation from. GetPreviousMenuItem
>+  //          returns the item before it, while GetNextMenuItem returns the
>+  //          next item.

"the item after it".

This should document that both methods loop around as needed.

>+  // set the mouse event that was used to activate the next popup to be opened.
>+  void SetMouseLocation(nsIDOMEvent* aEvent);

This could still use answers to the questions I asked in comment 60.

>+  // handle keyboard navigation within a menu popup. Returns true if the
>+  // key was handled and that other default handling should not occur.

s/that other/other/ please.

>Index: layout/xul/base/src/nsIMenuParent.h

Did the followup about renaming this to nsMenuParent ever get filed?

>+  // select the first item in it. For menupoups, the menu is not opened and

s/menupoups/menupopups/

>Index: layout/xul/base/src/nsMenuBarFrame.cpp
>+nsMenuBarFrame::SetCurrentMenuItem(nsMenuFrame* aMenuItem)

>  nsWeakFrame weakFrame(this);

Why?  SelectMenu() is safe now, no?  What else could destroy |this|?

>Index: layout/xul/base/src/nsMenuBoxObject.cpp
> NS_IMETHODIMP nsMenuBoxObject::OpenMenu(PRBool aOpenFlag)

Did my question about this code ever get addressed?  Search this bug for "opened but then not closed".  I don't see an answer, but maybe I missed it?

>Index: layout/xul/base/src/nsMenuFrame.cpp
> nsMenuFrame::InsertFrames(nsIAtom*        aListName,

Did I miss the answer about the next-sibling of aFrameList in the popup case?  What if aFrameList has both popups and non-popups on it?  What if it has popups and we already have a popup?

Same thing for ContentAppended.

>Index: layout/xul/base/src/nsMenuFrame.h
>+  NS_IMETHOD SelectMenu(PRBool aActivateFlag); // @see comment above ***

Really?  I thought you changed it to not destroy the frame...

>+   * NOTE: OpenMenu will open the menu synchronously. Don't call this if a frame

Did a followup bug get filed on auditing this?  That said, isn't OpenMenu async now?  I'm not sure what this comment is talking about if so, and the comments in OpenMenu do claim that it's async.

>+   *       All current calls to OpenMenu do not adjust the frame.

What does that mean?

>+  // called when the Enter key is pressed while the menuitem is the current
>+  // one in its parent popup. This will carry out the command attached to
>+  // the menuitem.
>+  nsMenuFrame* Enter();

Yes, but what does the return value mean?

>+  // indiciate that the menu's popup has just been opened, so that the menu
>+  // can update its open state. This method modifies the open attribute on
>+  // the menu, so the frames could be gone after this call

So why are some methods using the "****" thing and some not?  Frankly, I would just remove the "****" stuff and explicitly document on each method that can destroy the frame tree that it can do so.  Especially since so few things are using "****" at this point.

>Index: layout/xul/base/src/nsMenuPopupFrame.cpp

>+nsMenuPopupFrame::HidePopup(PRBool aDeselectMenu)
>+    FireDOMEvent(NS_LITERAL_STRING("DOMMenuInactive"), mContent);

After this we could be dead.

>+  nsIEventStateManager *esm = PresContext()->EventStateManager();

And this would be a crash at best.

>+    (NS_STATIC_CAST(nsMenuFrame*, parent))->PopupClosed(aDeselectMenu);
>+    PresContext()->RootPresContext()->NotifyRemovedActivePopup(this);

Same issue here.  I think I mentioned that on the first patch I reviewed, no?

>Index: layout/xul/base/src/nsMenuPopupFrame.h

>+  // called when the Enter key is pressed while the popup is open. This will

Again, what's the return value?

>Index: layout/xul/base/src/nsPopupSetFrame.cpp
>Index: layout/xul/base/src/nsXULPopupManager.cpp
>+nsXULPopupManager::HidePopup(nsIContent* aPopup,
>+    // called asynchrounsly. In either case, nextPopup is set to the content

"asynchronously"

>+nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)

>+    if (item->Content()->GetOwnerDoc() == aDocument)
>+      item->Frame()->HidePopup(PR_TRUE);
>+    item = item->GetParent();

The HidePopup() call can delete |item|, no?  Or do we protect against that somehow?

Same for the other loop.

>+nsXULPopupManager::FirePopupHidingEvent(nsIContent* aPopup,
>Not sure what is meant here.

What I meant was...  Is it possible for a complete popup open to happen between this event being posted and firing?  In other words, could it be that by this point the "right" state for aPopup is open?

This wouldn't really happen unless someone screws up, so as long as we don't crash, it's fine.

>+nsXULPopupManager::PopupDestroyed(nsMenuPopupFrame* aPopup)

>+        item->Frame()->HidePopup(PR_FALSE);

Again, this might kill menuToDestroyFrame, and then we have a major problem.

>+nsXULPopupManager::IsValidMenuItem(nsPresContext* aPresContext,

>+  if (ns == kNameSpaceID_XUL &&
>+       tag != nsGkAtoms::menu &&
>+       tag != nsGkAtoms::menuitem)
>+    return PR_FALSE;
>+
>+  if (ns == kNameSpaceID_XHTML && (!aOnPopup || tag != nsGkAtoms::option))
>+    return PR_FALSE;

So anything that's not XUL or HTML is a valid menu item?  Is that really right?  Feels fishy to me.

Comment 101

[Neil Deakin]

> It looks like nsXULPopupListener still doesn't participate in cycle collection.
>  Please file a followup bug on getting that done for 1.9.
> 

Bug 393566

> >+ * nsNavigationDirection: an enum expressing navigation through the menus in
> 
> Did a followup bug to improve this stuff a bit ever get filed?

Bug 393582

> >+  void SetMouseLocation(nsIDOMEvent* aEvent);
> 
> This could still use answers to the questions I asked in comment 60.
> 

I'll move this to just use an argument to ShowPopup/ShowPopupAtScreen

> >Index: layout/xul/base/src/nsIMenuParent.h
> Did the followup about renaming this to nsMenuParent ever get filed?

Bug 393575

> >  nsWeakFrame weakFrame(this);
> Why?  SelectMenu() is safe now, no?  What else could destroy |this|?

I'll remove the weak frame stuff here

> Did my question about this code ever get addressed?  Search this bug for
> "opened but then not closed".  I don't see an answer, but maybe I missed it?

Bug 388995 should have addressed issues with popups opening and closing in sequence. I tested this case specifically and it worked OK.

> Did I miss the answer about the next-sibling of aFrameList in the popup case? 
> What if aFrameList has both popups and non-popups on it?  What if it has popups
> and we already have a popup?
> 
> Same thing for ContentAppended.

I'll add an assertion to check that the next sibling is blank. In what situations would a sibling be set? Would it better to do something like what nsMenuFrame::SetInitialChildList does and extract out a popup?

> >+nsMenuPopupFrame::HidePopup(PRBool aDeselectMenu)
> >+    FireDOMEvent(NS_LITERAL_STRING("DOMMenuInactive"), mContent);
> 
> After this we could be dead.

FireDOMEvent is asynchronous, no?

> >+    (NS_STATIC_CAST(nsMenuFrame*, parent))->PopupClosed(aDeselectMenu);
> >+    PresContext()->RootPresContext()->NotifyRemovedActivePopup(this);
> 
> Same issue here.  I think I mentioned that on the first patch I reviewed, no?

This line was removed, so no issue any more.

> What I meant was...  Is it possible for a complete popup open to happen between
> this event being posted and firing?  In other words, could it be that by this
> point the "right" state for aPopup is open?
> 
> This wouldn't really happen unless someone screws up, so as long as we don't
> crash, it's fine.
> 

I'm not too concerned if someone opens/closes a popup in an unusual way. I don't
see a way for it to crash though.

Comment 102

[Boris Zbarsky [:bzbarsky]]

> I'll move this to just use an argument to ShowPopup/ShowPopupAtScreen

Is there a bug covering this?

> I'll remove the weak frame stuff here

Same.

> In what situations would a sibling be set?

Any time ContentAppended happens with more than one node, no?  For example, appending a DocumentFragment with a popup and some other stuff in it.

> Would it better to do something like what nsMenuFrame::SetInitialChildList
> does and extract out a popup?

Might be, yes.

> FireDOMEvent is asynchronous, no?

Ah, yes.  Ok.

Comment 103

[Neil Deakin]

(In reply to comment #102)
> > I'll move this to just use an argument to ShowPopup/ShowPopupAtScreen
> 
> Is there a bug covering this?
> 

I'm creating a patch addressing all of these comments, but I can post it in a follow up bug.

Comment 104

[Boris Zbarsky [:bzbarsky]]

Whatever works best for you.  Here is fine too.

Comment 105

[Gomita]

Attachment: ignorekeys and enableKeyboardNavigator testcase

It seems that the effect of nsIPopupBoxObject#enableKeyboardNavigator is reversed.
enableKeyboardNavigator(false) works same as ignorekeys="true" on Firefox/2.0.0.6 but ignorekeys="false" on Minefield/3.0a8pre.

Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9a8pre) Gecko/2007091604 Minefield/3.0a8pre

Comment 106

[Boris Zbarsky [:bzbarsky]]

Filed bug 396517 on that.  In general, you want to file bugs on regressions (and request blocking1.9) and then point to the new bug in the regressing bug instead of just making comments that easily get lost.

Comment 107

[Aaron Leventhal]

This caused bug 397606.

Comment 108

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9a8ca53c8fe0
rework XUL popups to use asynchronous opening, plus many other fixes, r=bz,neil,sr=bz
https://hg.mozilla.org/comm-central/rev/6009e09cf861
backing out the popup changes due to performance regressions. sigh.
https://hg.mozilla.org/comm-central/rev/8e44615e3bd2
rework XUL popups to use asynchronous opening, plus many other fixes, attempt 2 with creating widgets later, r=bz,neil,roc,sr=bz