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

VERIFIED FIXED

Status

()

Core
XUL
P3
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Warner Young, Assigned: Neil Deakin)

Tracking

({regression})

Trunk
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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"
Thanks for filing, Warner.
You can CC the relevant people (if you think they caused the bug), so the bug won't be forgotten.
Blocks: 397012
Component: Menus → XP Toolkit/Widgets: Menus
Keywords: regression
Product: Firefox → Core
QA Contact: menus → xptoolkit.menus

Comment 2

11 years ago
I see this on Linux as well with a 2007103104 trunk build.
OS: Windows XP → All
Also after clicking Cancel on the Properties dialog of a bookmark. I suppose this is the same bug.

Comment 4

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

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

Comment 6

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

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

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

Updated

11 years ago
Flags: blocking1.9?

Updated

11 years ago
Blocks: 402855

Updated

11 years ago
No longer blocks: 402855
Duplicate of this bug: 402855

Comment 10

11 years ago
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.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

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

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

Comment 15

11 years ago
Created attachment 289729 [details] [diff] [review]
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 17

11 years ago
Created attachment 290276 [details] [diff] [review]
complete patch, as described in comment fo previous patch
Attachment #289729 - Attachment is obsolete: true
Attachment #290276 - Flags: superreview?(roc)
Attachment #290276 - Flags: review?(roc)
+		  content \
+		  layout \

Urgh, why do we need that?

Other than that it looks OK
(Assignee)

Comment 19

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

Because nsIWidget needs nsIContent defined.
And why's that? Can we break that dependency instead?
(Assignee)

Comment 21

11 years ago
Created attachment 290554 [details] [diff] [review]
actually, yes, we can remove the dependency by moving the nsAutoRollup constructor/destructor
Attachment #290276 - Attachment is obsolete: true
Attachment #290554 - Flags: superreview?(roc)
Attachment #290554 - Flags: review?(roc)
Attachment #290276 - Flags: superreview?(roc)
Attachment #290276 - Flags: review?(roc)
Attachment #290554 - Flags: superreview?(roc)
Attachment #290554 - Flags: superreview+
Attachment #290554 - Flags: review?(roc)
Attachment #290554 - Flags: review+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED

Updated

11 years ago
Depends on: 406956

Updated

10 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets

Updated

9 years ago
Blocks: 504172
You need to log in before you can comment on or make changes to this bug.