Need to redesign how XUL popups work

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
XUL
--
critical
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: bz, Assigned: Neil Deakin (mostly unavailable until September))

Tracking

(Depends on: 6 bugs, Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 6 obsolete attachments)

494.77 KB, patch
Ginn Chen
: review+
Details | Diff | Splinter Review
495.38 KB, patch
Details | Diff | Splinter Review
61.63 KB, patch
Details | Diff | Splinter Review
1.08 KB, patch
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
931 bytes, patch
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
1.18 KB, patch
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
1.73 KB, application/vnd.mozilla.xul+xml
Details
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

13 years ago
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.
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

13 years ago
(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.
OK.  So just selecting popups using CSS and the state of the menu won't work....
(Reporter)

Updated

13 years ago
Blocks: 282359
(Reporter)

Updated

13 years ago
Blocks: 283679
(Reporter)

Updated

12 years ago
Blocks: 291621
(Reporter)

Updated

12 years ago
Blocks: 294183
(Reporter)

Updated

12 years ago
Blocks: 297570

Updated

12 years ago
Blocks: 310474

Comment 5

12 years ago
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.
Flags: blocking1.9a1?
There is no reasonable partial fix here.

Updated

12 years ago
Blocks: 265897

Updated

12 years ago
Blocks: 323628

Updated

12 years ago
Blocks: 323617

Updated

12 years ago
Blocks: 246712

Updated

12 years ago
Blocks: 323750

Updated

12 years ago
Blocks: 288763
(Reporter)

Updated

12 years ago
Blocks: 308030

Updated

12 years ago
Blocks: 327973

Updated

12 years ago
Blocks: 330456

Updated

12 years ago
Blocks: 331297

Updated

12 years ago
No longer blocks: 308030
(Reporter)

Updated

12 years ago
Blocks: 334984

Updated

12 years ago
Blocks: 336739

Updated

12 years ago
Blocks: 336744
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.
Assignee: nobody → enndeakin
Blocks: 342619

Updated

11 years ago
Blocks: 344176

Comment 8

11 years ago
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.
I doubt there's any way to create a patch here that would be safe for branch at this point....
(Reporter)

Updated

11 years ago
Blocks: 348304

Updated

11 years ago
Flags: blocking1.9a1? → blocking1.9+
Status: NEW → ASSIGNED
Created attachment 236897 [details] [diff] [review]
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.
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?
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.
(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

11 years ago
(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.
> 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.
Blocks: 286751

Updated

11 years ago
Blocks: 351938
(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.


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

Created attachment 239217 [details] [diff] [review]
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.
Attachment #236897 - Attachment is obsolete: true
Testcases are at:

http://xulplanet.com/ndeakin/tests/xts/menu/
 and
http://xulplanet.com/ndeakin/tests/xts/popup/
(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.

Updated

11 years ago
Blocks: 354300
Created attachment 240488 [details] [diff] [review]
Fix issues

This should fix the issues described above, and should also fix the 2x2 popup issue.
Attachment #239217 - Attachment is obsolete: true

Updated

11 years ago
No longer blocks: 354300
(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.
Blocks: 335162
> 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 ;)
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...
Created attachment 240964 [details] [diff] [review]
Fixed latest set of issues
Attachment #240488 - Attachment is obsolete: true
Created attachment 242019 [details] [diff] [review]
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.
Attachment #242019 - Flags: superreview?(bzbarsky)
Attachment #242019 - Flags: review?(roc)
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.  :(
If Boris is going to sr this (and I think he should) then do you really need me to review too?

Comment 29

11 years ago
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 ;-)
Blocks: 325333
Blocks: 362403

Comment 30

11 years ago
this missed bz's window. roc is it still on your agenda?
Can't bz review this next month?
> 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.
Blocks: 332845
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 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

11 years ago
(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); ?

Updated

11 years ago
Blocks: 367988
Blocks: 370673

Updated

11 years ago
Blocks: 329938
Blocks: 195031
bz, do you want me to make a more up to date patch for this?
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!
Created attachment 260959 [details] [diff] [review]
updated popup reworking patch to trunk
Attachment #240964 - Attachment is obsolete: true
Attachment #242019 - Attachment is obsolete: true
Attachment #260959 - Flags: review?(bzbarsky)
Attachment #242019 - Flags: superreview?(bzbarsky)
Attachment #242019 - Flags: review?(roc)

Comment 39

11 years ago
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

11 years ago
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.)

Updated

11 years ago
Blocks: 357337
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.
(Reporter)

Updated

10 years ago
Blocks: 375436

Comment 42

10 years ago
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

10 years ago
I should note, we still have problems with this, which I haven't investigated deeply (bug 332663).
Target Milestone: --- → mozilla1.9alpha6
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

10 years ago
> 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.
(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

10 years ago
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.
(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.
aaronlev: I can add a method to force the popup to generate it's frames. It will occur asynchronously though.

Comment 50

10 years ago
(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

10 years ago
Neil D, thanks.
(In reply to comment #50)
> So <xul:menupopup class="folderTargetPopup"> should become a panel?
> 

Don't know. What is it?

Comment 53

10 years ago
(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).
> 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>?
(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.
> They would behave the same internally yes. 

Great.  Thanks for clarifying that!

Comment 57

10 years ago
(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.
Blocks: 381689
Blocks: 374524
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

10 years ago
(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.
>+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.
> +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.  :(
(Reporter)

Updated

10 years ago
Attachment #260959 - Flags: review?(bzbarsky) → review-
Blocks: 383930
Depends on: 384062

Updated

10 years ago
Blocks: 384105

Updated

10 years ago
Blocks: 384062
No longer depends on: 384062
> 
> 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.
> 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?
(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?
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.
> 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!

Updated

10 years ago
Blocks: 384871

Updated

10 years ago
Blocks: 384877
Created attachment 269055 [details] [diff] [review]
Updated patch

I'll add the cycle collection mentioned above in a followup bug
Attachment #260959 - Attachment is obsolete: true
Attachment #269055 - Flags: superreview?(bzbarsky)
Attachment #269055 - Flags: review?(bzbarsky)
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
Attachment #269055 - Flags: review?(gavin.sharp)
Comment on attachment 269055 [details] [diff] [review]
Updated patch

Aarin, could you review the accessibility changes here?
Attachment #269055 - Flags: review?(aaronleventhal)
1) The last patch doesn't build on windows with a11y enabled
2) Autocomplete in the location bar is broken (at least on mac)
3) tooltips are in some sort of one-word-per-line mode.

Comment 72

10 years ago
Comment on attachment 269055 [details] [diff] [review]
Updated patch

Ginn, can you review the accessibility changes here?
Attachment #269055 - Flags: review?(aaronleventhal) → review?(ginn.chen)
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.
Attachment #269055 - Flags: superreview?(bzbarsky) → superreview?(roc)
Mano: I don't see any issues with autocomplete on Mac.

Comment 75

10 years ago
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?
Attachment #269055 - Flags: review?(ginn.chen) → review+
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?

Updated

10 years ago
Blocks: 332663
Neil (Rashbrook), are you OK with reviewing the browser/toolkit/autocomplete parts here?

Comment 78

10 years ago
(In reply to comment #77)
>Neil are you OK with reviewing the browser/toolkit/autocomplete parts here?
Yeah, they look fine to me.
@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.
At least on windows, setting the noautohide attribute on a panel makes its contents not focusable.

Comment 81

10 years ago
(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"
(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.
Created attachment 269745 [details] [diff] [review]
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.
I think you'd better check it in after alpha6.
Will check this in just after alpha6.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Depends on: 386380

Updated

10 years ago
Depends on: 386390

Comment 86

10 years ago
Neil - is there any automated test coverage of this patch?
(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.
 
Created attachment 270909 [details] [diff] [review]
work in progress: testcases

Still some work to be done on Windows/Linux but the tests are mostly passing
Checked in!
Created attachment 270936 [details] [diff] [review]
fix bustage on x86_64 Linux
Attachment #270936 - Flags: review?(enndeakin)
Attachment #270936 - Flags: review?(enndeakin) → review+

Updated

10 years ago
Depends on: 386917

Comment 91

10 years ago
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
<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.
 
Created attachment 271094 [details] [diff] [review]
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.
Attachment #271094 - Flags: review?(enndeakin)
Attachment #271094 - Flags: review?(enndeakin) → review+

Comment 94

10 years ago
Created attachment 271147 [details] [diff] [review]
Fix shortcut navigation opening a submenu
Attachment #271147 - Flags: review?(enndeakin)
(Reporter)

Updated

10 years ago
Attachment #271094 - Attachment description: Include the stuff we use → Include the stuff we use (checked in)

Comment 95

10 years ago
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?
Depends on: 387097
Alfred, I filed bug 387097 for the color picker issue
> 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Attachment #271147 - Flags: review?(enndeakin) → review+

Updated

10 years ago
Depends on: 387109
Depends on: 387124

Updated

10 years ago
Blocks: 387131

Updated

10 years ago
No longer blocks: 387131
Depends on: 387142

Comment 98

10 years ago
Neil - could this have caused bug 387111?
Depends on: 387236

Updated

10 years ago
Depends on: 387266

Updated

10 years ago
Depends on: 387270

Updated

10 years ago
Depends on: 387272
Depends on: 387499
Depends on: 387533
Depends on: 387548
Depends on: 387985
Depends on: 388112

Updated

10 years ago
Depends on: 388123

Updated

10 years ago
No longer depends on: 388123

Updated

10 years ago
Depends on: 388123

Updated

10 years ago
No longer depends on: 388123

Updated

10 years ago
Depends on: 388280
Depends on: 388361

Updated

10 years ago
Depends on: 388375

Updated

10 years ago
Depends on: 388423
Depends on: 389112

Comment 99

10 years ago
It seems like this might have caused bug 389542 on Mac.
Attachment #269055 - Flags: review?(gavin.sharp)
(Reporter)

Updated

10 years ago
Depends on: 389542
(Reporter)

Updated

10 years ago
Depends on: 389573

Updated

10 years ago
Depends on: 389931

Updated

10 years ago
Depends on: 389945

Updated

10 years ago
Depends on: 389996

Updated

10 years ago
Blocks: 378691
Depends on: 390420
Depends on: 390589
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.

Updated

10 years ago
Depends on: 393398

Updated

10 years ago
Depends on: 393791
> 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.
Flags: in-testsuite-
> 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.



(Reporter)

Updated

10 years ago
Depends on: 393566
(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.
Whatever works best for you.  Here is fine too.
(Reporter)

Updated

10 years ago
Depends on: 394213

Updated

10 years ago
Depends on: 394301
(Reporter)

Updated

10 years ago
Attachment #269055 - Flags: review?(bzbarsky) → review+
Blocks: 394600

Comment 105

10 years ago
Created attachment 281149 [details]
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
Attachment #269055 - Flags: superreview?(roc)
(Reporter)

Updated

10 years ago
Depends on: 396517
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.
(Reporter)

Updated

10 years ago
Depends on: 396983

Comment 107

10 years ago
This caused bug 397606.

Updated

10 years ago
Depends on: 397606

Updated

10 years ago
Depends on: 399013

Updated

10 years ago
Depends on: 399468

Updated

10 years ago
Depends on: 400185
Depends on: 402117

Updated

10 years ago
Depends on: 405662

Updated

10 years ago
Depends on: 399325
No longer depends on: 405662

Updated

10 years ago
Depends on: 405719
Depends on: 400671

Updated

10 years ago
Depends on: 411903
Depends on: 411048

Updated

10 years ago
Depends on: 415440

Updated

10 years ago
Blocks: 418798

Updated

10 years ago
No longer blocks: 418798

Updated

10 years ago
Blocks: 422515
(Reporter)

Updated

10 years ago
No longer blocks: 422515
Depends on: 422515

Updated

9 years ago
Depends on: 434456

Updated

9 years ago
Depends on: 434458

Updated

9 years ago
Depends on: 431997

Updated

9 years ago
No longer depends on: 431997
Depends on: 447626

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.widgets
Depends on: 458789
Depends on: 406646
Depends on: 474197
Depends on: 480191

Updated

4 years ago
Depends on: 981125
You need to log in before you can comment on or make changes to this bug.