401627 - Clicking menu doesn't work after dismissing it by clicking on title bar

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Warner Young]

I think this is related to bug 397012, but wasn't fixed by bug 399350.  I'm filing this as a new bug as advised.

Steps to reproduce:
1) Click menu bar to open any menu, for example, Bookmarks.
2) Click the window's title bar, which will close the menu.
3) Now click the same menu Bookmarks) again.  The first click fails, but the second one works.

This is on "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre"

Comment 1

[Martijn Wargers (dead)]

Thanks for filing, Warner.
You can CC the relevant people (if you think they caused the bug), so the bug won't be forgotten.

Comment 2

[Elmar Ludwig]

I see this on Linux as well with a 2007103104 trunk build.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Also after clicking Cancel on the Properties dialog of a bookmark. I suppose this is the same bug.

Comment 4

[Matthew Babbs]

I see a related effect when middle-clicking.

Steps to reproduce:
1) Click menu bar to open a menu, e.g., Bookmarks.
2) Middle-click an item on the menu.
3) Click the same menu again. The first click won't open the menu, but the second one will.

FWIW, I have "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110103 Minefield/3.0a9pre ID:2007110103".

Comment 5

[AndrewM]

I am able to reproduce the problem in comment 0 and comment 4, but not comment 3.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110103 Minefield/3.0a9pre ID:2007110103

Comment 6

[Warner Young]

I can reproduce comment 3: click Bookmarks menu, find any bookmark, right-click it and choose Properties.  Click cancel to dismiss it.  Now it takes 2 clicks to open the Bookmarks menu again.

Comment 7

[AndrewM]

(In reply to comment #6)
> I can reproduce comment 3: click Bookmarks menu, find any bookmark, right-click
> it and choose Properties.  Click cancel to dismiss it.  Now it takes 2 clicks
> to open the Bookmarks menu again.

I found that I can reproduce it by doing that; I was right-clicking on bookmarks in the bookmark toolbar, and it does not happen for me when I do it that way.

Comment 8

[Ginn Chen]

I found the same issue on Solaris and Linux.
2007100904 is fine. 2007101004 is broken.

Comment 10

[Kevin Brosnan]

Here is another way to reproduce from my above bug.

If a menu is displayed then the browser is moved so that the menu would extend
past the edge of the screen the menu does not appear on the first click.

Steps to reproduce
1. click on a menu
2. reposition the browser window so the will extend off the screen
3. click on the same menu

results:
Trunk no menu is displayed till the second click on the menu
Firefox 2 menu is displayed

This would be a common issue for someone with a long bookmarks list and runs
Firefox in a non-maximized state. Any movement of the window would result in
this bug.

Comment 11

[Neil Deakin]

I already know what the problem is here, so no need for more testing when this occurs. It's caused because a mouse event not over the window area, such as over another application, doesn't get sent to the application so the flag which indicates that a menu was just closed doesn't get reset.
  
I haven't come with a good idea of how to fix this yet, it may involve resorting to implementing an extra call after the mouse event for each platform.

Comment 12

[Warner Young]

Neil, not knowing how the code actually works, maybe this is a stupid question, but: there's obviously existing code that "knows" when an on-screen menu needs to be dismissed (e.g. when you click on the title bar, the menu goes away).  Can't the necessary code to reset the "menu was closed" flag be put somewhere in there?  That is, in the menu code itself?

Comment 13

[Neil Deakin]

(In reply to comment #12)
> Can't the necessary code to reset the "menu was closed" flag be put somewhere
> in there?  That is, in the menu code itself?
> 

It needs to be reset in the platform-specific widget code (nsWindow) after a mouse event. As the event occured outside the application, it doesn't get fired at anything in the window, so the menu code doesn't know anything happened. Note that the closed state needs to be reset after dismissing the menu, not when clicking it.

 

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Would single shot 0 timer, which checks whether there is a menu open, be bad?

Comment 15

[Neil Deakin]

Attachment: work in progress

This patch reverts the change made in bug 397012, and uses a different technique. Instead, we store the popup that was just rolled up until after the event that triggered the rollup has been processed. If we attempt to reopen this popup during the event, it won't be opened. After the event, the stored popup is cleared.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 289729 [details] [diff] [review]
work in progress


>+...::Rollup(nsISupports** aLastRolledUp)
> {
>+  *aLastRolledUp = nsnull;
you're setting *aLastRolledUp even if aLastRolledUp might be null.
Shouldn't it be 
if (aLastRolledUp)
  *aLastRolledUp = nsnull;
}

> 
>+nsISupports* nsBaseWidget::mLastRollup = nsnull;
>+
You might want to document that nsAutoRollupOccured releases this.

Comment 17

[Neil Deakin]

Attachment: complete patch, as described in comment fo previous patch

Comment 18

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

+		  content \
+		  layout \

Urgh, why do we need that?

Other than that it looks OK

Comment 19

[Neil Deakin]

(In reply to comment #18)
> +                 content \
> +                 layout \
> 
> Urgh, why do we need that?

Because nsIWidget needs nsIContent defined.

Comment 20

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

And why's that? Can we break that dependency instead?

Comment 21

[Neil Deakin]

Attachment: actually, yes, we can remove the dependency by moving the nsAutoRollup constructor/destructor

Comment 22

[Martijn Wargers (dead)]

Works great! Thanks.

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120405 Minefield/3.0b2pre