71219 - Context menu doesn't disappear when opening modal dialog

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Koike Kazuhiko]

Context menu doesn't disappear when opening "Insert Table"
dialog.

Step to reproduce:
(1) Open Composer window.
(2) Insert table by [Table]-[Insert]-[Table].
(3) Right click on the table and select [Table Insert]-[Table].
"Insert Table" dialog appears and context menu doesn't disaappear.

Build: 2001030708/Linux

Comment 1

[Kathleen :Brade]

->editor

Comment 2

[Charles Manske]

This can't be an editor bug!
I bet it would happen for any dialog that launches from a submenu of a context
menu, but there's no other examples that I could find in browser, etc.
I noticed that bug 67932 (submenus from a context menu cannot be used more
than once) seems to be fixed (though not by bug owner!), and I'm wondering if
these issues are related.

Comment 3

[Mike Pinkerton (not reading bugmail)]

icky

Comment 4

[Mike Pinkerton (not reading bugmail)]

this might be related to 71795

Comment 5

[Charles Manske]

Not sure if this is new bugzilla behavior, but it appears that a bug reference
in a comment becomes a link only if a number is preceeded by the word "bug",
such as "bug 71795", so we all should get into that habit.

Comment 6

[:deanis74]

Hasn't it always been that way?

Comment 7

[Charles Manske]

Definitely not!

Comment 8

[:deanis74]

Hmmm... watch things again when you do this.  I don't think it's that the
context menu doesn't disappear, I think it's that it disappears and then re-appears.

Comment 9

[:deanis74]

Here's some testing results.  Don't know if they help or confuse, I was just 
trying to track down if something weird was being done in xul or js.  Everything 
looks pretty normal to me.

1. Removed the observer on the menu item (EditorContextMenuOverlay.xul).  
Context menu disappeared.  Of course, the dialog didn't appear.

2. Added an ID to every item in the tableInsertMenu_cm menupopup 
(EditorContextMenuOverlay.xul).  Context menu remained.  But shouldn't each menu 
item have an ID anyway, or is that optional?

Off to the .js code for the menu command...

3. Wrapped the two lines of code in EditorInsertTable() (editor.js) with "if 
PageIsEmptyAndUntouched()", so that I could insert one table but the code 
wouldn't fire on the second insert.  The context menu disappeared, and nothing 
happened.

4. Wrapped only openDialog() in EditorInsertTable() in PIEAU(), allowing 
"window._content.focus()" to fire every time.  Context menu disappeared, nothing 
happened.

5. Wrapped only the focus() call in EditorInsertTable() in PIEAU(), so 
openDialog() fires every time.  Context menu remained.

Comment 10

[:deanis74]

More scratch notes...

· This occurs when you use either button to select the "Table..." menu item, so 
it's not related to right-click processing.

· This also occurs if you use the keyboard once the context menu is initally 
displayed.

· The fact that this doesn't happen off of the Table > Insert > Table menu item, 
which calls the exact same code, points toward the context menu.

Comment 11

[:deanis74]

Can someone possibly code a quick example of a very basic, hierarchical context 
menu?  I tried before when I implemented the context menu right-click 
selection, but I could never get it to work.  I could use something like that 
for testing this bug and also bug 71795.

Comment 12

[Charles Manske]

Good testing, Dean.
Answering some questions:
Ids are definitely optional. You really only need them if you need to
getElementById(). We tend to leave them off when we don't need to do that
to reduce XUL code bloat.
I meant to add a comment that it does look like the menu does disappear, but
immediately reappear, which to me suggests problem is mouse event processing.
The reason it only occurs when a dialog is popped up suggests that maybe the
dialog is being created too quickly, stealing the focus and interfering with
pending mouse events whose target is the content window?

Comment 13

[Mike Pinkerton (not reading bugmail)]

*** Bug 73721 has been marked as a duplicate of this bug. ***

Comment 14

[Blake Ross]

This is kinda like bug 65943, where we need to reset the content state before 
firing the event handler to handle cases like this (with modal dialogs).  Are 
we dismissing the context menu before executing the action in the event handler?

Comment 15

[:deanis74]

I don't think so.  See

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#1345

We call HideChain, then HandleDOMEventWithTarget, then DismissChain.

Comment 16

[Mike Pinkerton (not reading bugmail)]

so here's the problem:

- when we hide the chain, there's a reflow left pending that needs to run do to 
some cleanup. I'm not yet sure why.
- In the normal case, we don't get back to the main event loop until after we've 
called DismissChain() and left the whole Execute() routine. In this case, 
however, showing the modal dialog gives us an event loop with which to idly 
process reflow commands. the reflow causes us to think we need to show ourselves 
again, perhaps because we have yet to dismiss the chain (we only hid it). 
- cancelling all reflows on the presShell after calling hideChain() fixes the 
immediate problem, but then the context menu will never show up again

so we need to find out why this extra reflow command needs to be processed and 
what effects not having called DismissChain() have while processing this reflow.

Comment 17

[Mike Pinkerton (not reading bugmail)]

Attachment: [patch] flush reflows that get us into trouble

Comment 18

[David Hyatt]

sr=hyatt

Comment 19

[Mike Pinkerton (not reading bugmail)]

fixed.

Comment 20

[:deanis74]

vfy