Closed Bug 392512 Opened 12 years ago Closed 12 years ago

Opening a panel from within an oncommand handler of a menuitem fails

Categories

(Core :: XUL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mano, Assigned: enndeakin)

References

Details

Attachments

(2 files, 1 obsolete file)

Opening a panel from within an oncommand handler of a menuitem fails. It seems as the panel is auto-hidden when the menuitem's containing popup is closed.

Opening the panel asynchronously (i.e. setTimeout) works.
Flags: blocking1.9?
Attached patch fix (obsolete) — Splinter Review
This is very easy fix. The call to Rollup in nsXULMenuCommandEvent::Run should call HidePopup with the menu to close instead so that any new popups that were opened aren't rolled up as well.

Mano, does this fix your case? If so, I'll create a test for this too.
Assignee: jag → enndeakin
Status: NEW → ASSIGNED
Attached patch include testSplinter Review
This changes it so that:
 1. In HidePopup, don't close up children that aren't submenus.
 2. In nsXULCommandEvent::Run, make sure to call HidePopup with the parent popup of the menuitem that was executed, rather than Rollup, as Rollup always closes the topmost popup, and in this case, a new panel was opened above it during the preceding command event.
Attachment #277043 - Attachment is obsolete: true
Attachment #277392 - Flags: superreview?(bzbarsky)
Attachment #277392 - Flags: review?(bzbarsky)
So I'm not sure I follow...  In HidePopup, we could have menus that are on top of us but are not submenus?  And are we looking for the bottommost menu (as the comments say) or the topmost one (as the variable naming seems to indicate)?

How do |foundMenu| and |item| differ at that point in the code, if at all?
(In reply to comment #5)
> So I'm not sure I follow...  In HidePopup, we could have menus that are on top
> of us but are not submenus?

There could be popups that aren't menus.

  And are we looking for the bottommost menu (as the
> comments say) or the topmost one (as the variable naming seems to indicate)?

It should be topmost, I will fix the comment.

> 
> How do |foundMenu| and |item| differ at that point in the code, if at all?
> 

At the 'if (foundMenu)' line, foundMenu and item will be the same. I changed the code to use foundMenu consistently and just reuse 'item' as a temporary variable.
Comment on attachment 277392 [details] [diff] [review]
include test

>Index: layout/xul/base/src/nsXULPopupManager.cpp
>+    // find the bottomost menu and close that menu first. In synchronous mode,

"find the topmost menu with only menus between it and foundMenu and close ...".

>+    if (foundMenu->IsMenu()) {

Why this check?  Document.

r+sr=bzbarsky wth that
Attachment #277392 - Flags: superreview?(bzbarsky)
Attachment #277392 - Flags: superreview+
Attachment #277392 - Flags: review?(bzbarsky)
Attachment #277392 - Flags: review+
Comment on attachment 277392 [details] [diff] [review]
include test

Will fix the comments.
Attachment #277392 - Flags: approval1.9?
Comment on attachment 277392 [details] [diff] [review]
include test

a1.9=dbaron
Attachment #277392 - Flags: approval1.9? → approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: blocking1.9? → in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.