203183 - Bookmarks context menu stays when clicking other menus

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Rodrigo Cabrera]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030422 Firebird™ Browser/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030422 Firebird™ Browser/0.6

The popup menu displayed when right-clicking a bookmark doesn't go away when
clicking another menu

Reproducible: Always

Steps to Reproduce:
1. You must have a bookmark hanging from the main "Bookmarks" menu
2. Right-click the bookmark. Popup menu appears
3. Move mouse to another top-level menu (Go, Tools, whatever)



Actual Results:  
The popup menu belonging to a bookmark still shows on screen. Actions performed
on this menu do have the intended effect(open, delete, etc) on the bookmark

Expected Results:  
Firebird should hide the popup menu when focus is taken away from the
"Bookmarks" menu

Using default theme, vanilla Firebird

Comment 1

[Sébastien Delahaye]

Confirming.

Comment 2

[David Tenser [:djst]]

Updating summary. This is probably related to bug 197112.

Comment 3

[David Tenser [:djst]]

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

Comment 4

[Simon Paquet [:sipaq]]

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

Comment 5

[Bill Mason]

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

Comment 6

[Bill Mason]

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

Comment 7

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 8

[Bill Mason]

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

Comment 9

[Stuart Gilbert]

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

Comment 10

[Bill Mason]

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

Comment 11

[mmortal03]

This also happens sporadically when right clicking bookmark FOLDERS.  I am using 
the latest nightly 20040320.  It doesn't happen everytime, but I will right click
 a folder in the bookmarks, and then click away on the current page, and the 
context menu will stay.  When I right click a bookmark and click away, this does 
not occur.  As long as the context menu stays on the screen, trying to right click
 ANOTHER bookmark or bookmark folder acheives weird results, as in the context 
menu not appearing at all or the context menu appearing BEHIND the bookmark menu.

Comment 12

[Oleg Sidletskiy]

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

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

worth investigating before 1.0 due to dupecount. 

Comment 14

[Bill Mason]

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

Comment 15

[Anders Pedersen]

Confirmed.

- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040510
Firefox/0.8.0+
- Microsoft Windows 2000 Pro 5.00.2195 SP4

Comment 16

[mmortal03]

Because of Bug 203556 being fixed 
http://bugzilla.mozilla.org/show_bug.cgi?id=203556 , now this bug applies to 
Bookmark Toolbar context menus as well.

Comment 17

[Simon Paquet [:sipaq]]

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

Comment 18

[Dimitrios]

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

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: contextmenu-popup-0.patch

Two instances of this bug:

1) Drop down Bookmarks menu, right-click on a bookmark.  Move mouse
over another top-level menu (i.e. File).  The context menu stays.

2) Drop down Bookmarks menu, right-click on a folder -before- the
folder submenu opens.  You're correctly locked out until you move the
mouse back over the folder item, and let it hover there for the
submenu open timeout delay -- at which point the submenu appears, and
you're no longer locked out of the menus, and the context menu stays.

The patch fixes both issues.  While the fixed behaviour is consistent
with IE, I'm not particularily fond of it (locking you out of the
toplevel menubar); would it be better, for the first case only, to
allow the user to drag over top level menus but just hide the context
menu appropriately?

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: contextmenu-popup-1.patch

Avoid crash on context menu item selection from submenu; blocking
SetCurrentMenuItem(nsnull) seems bad.

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 151624 [details] [diff] [review]
contextmenu-popup-1.patch

r=[e-mail]

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: contextmenu-popup-2.patch

The band-aid fix in patch-1 wasn't correct; the issue was nsIFrames that were
being stored in nsCOMPtr's, and being destroyed while still in a nsCOMPtr. 
When the destructor tried to Release, it went boom.

There's still an outstanding issue of keyboard navigation being strange with
context menus, but at least it sort of works now -- hitting ESC with a context
menu active will tear down all open menus, instead of just the context menu
like it's supposed to.	Still trying to track down why, but it's not crucial
(i.e. much less broken than before).  Hitting TAB also does the same thing
(which is probably wrong behaviour for tab to begin with -- tab on OSX cycles
toplevel menus, which might be better).

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 152084 [details] [diff] [review]
contextmenu-popup-2.patch

I like it, let's get ben to double-up.	nsIFrame and nsISupports should be rent
asunder, mayhap.

sr=shaver

Comment 25

[:Gavin Sharp [email: [e-mail]]]

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

Comment 26

[Worcester12345]

I am seeing this annoying one here. Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.7) Gecko/20040723 Firefox/0.9.1+ (bangbang023)

Comment 27

[Ryan Polk (Quark)]

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

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 152084 [details] [diff] [review]
contextmenu-popup-2.patch

This sort of got dropped.. any menus gurus want to take a look so I can get it
in?

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 152084 [details] [diff] [review]
contextmenu-popup-2.patch

a=[e-mail]

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In on aviary, not sure about r= status for trunk.

Comment 31

[Simon Paquet [:sipaq]]

Ben, is still listed as a peer for layout/xul (but who knows how current the
information on the module owners list is). Just to be a safe I would ask another
XUL peer to review this. BZ or Bryner seem like good choices here.

Comment 32

[[e-mail]]

Comment on attachment 152084 [details] [diff] [review]
contextmenu-popup-2.patch

>+void
>+nsMenuFrame::GetContextMenu(nsIMenuParent** aContextMenu)
>+{
>+  *aContextMenu = nsnull;
>+  if (!nsMenuFrame::sDismissalListener)
>+    return;
>+
>+  nsIMenuParent *menuParent = nsnull;
>+  nsMenuFrame::sDismissalListener->GetCurrentMenuParent(&menuParent);
>+  if (!menuParent)
>+    return;
>+
>+  PRBool isContextMenu;
>+  menuParent->GetIsContextMenu(isContextMenu);
>+  if (isContextMenu) {
>+    *aContextMenu = menuParent;
>+    NS_ADDREF(*aContextMenu);
>+  }
>+}
The menuParent is a frame so the NS_ADDREF is still wrong; also it would make
things much neater if you returned the nsIMenuParent* making
IsContextMenuActive() redundant.

Comment 33

[Asa Dotzler [:asa]]

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

Comment 34

[Asa Dotzler [:asa]]

Ignore that bogus dupe, I meant to reassign the regression to vlad, not to dupe
it to this bug.

Comment 35

[Worcester12345]

(In reply to comment #26)
> I am seeing this annoying one here. Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.7) Gecko/20040723 Firefox/0.9.1+ (bangbang023)


I am not seeing it with this build: Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.7) Gecko/20040802 Firefox/0.9.1+ (bangbang023)

Comment 36

[Mike Kaply [:mkaply]]

Someone should probably consider trying to get this on the trunk if it is important.

Comment 37

[Worcester12345]

Adding fixed-aviary1.0 to keywords. Let me know if I'm wrong in doing so.

Never mind, it wouldn't let me.

Comment 38

[Ryan Polk (Quark)]

Vlad: this is still not in on trunk, but comments 31 and 32 still need to be
addressed.

Comment 39

[Stefan Pietschmann]

looks like it has been fixed on the branch, so could somebody please add the 
fixed-aviary1.0 keyword?
(i know this is trivial but it would be useful for querying the overall number
of remaining bugs)

Comment 40

[Stefan Pietschmann]

D'OH! Forget my comment!

Comment 41

[:Gavin Sharp [email: [e-mail]]]

[e-mail] : Please do not touch the blocking-aviary1.0 flag
other than to request it (?).

Comment 42

[claus jensen]

I get this a lot when i'm accidentally clicking a wrong bookmark and trying to
undo using right click. Like I do in explorer.

1. Click bookmarks
2. Press and hold left mouse over a bookmark (dont move cursor at all)
3. Press and release right mouse
4. Release left mouse

Now you are on the page that you didnt want to go to and the context menu
sticks. You have to click an item to make it go away.

Seems the bookmarks menu should somehow know its child context menu and not
leave it dangling when it goes away itself.

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 152084 [details] [diff] [review]
contextmenu-popup-2.patch

a=asa for 1.7.5.

Comment 44

[Worcester12345]

needs aviary landing keyword

Comment 45

[Bill Mason]

(In reply to comment #44)
> needs aviary landing keyword

This bug is over a year old.  It is not an aviary landing bug.

Comment 46

[Worcester12345]

Still here with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050228 Firefox/1.0+

Comment 47

[Asa Dotzler [:asa]]

vlad, ben, or mconnor, can one of you land this patch if it hasn't landed and if
it has but the bug is still here, can you obsolete that patch and help us get a
new one?

Comment 48

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'll rework this for trunk/1.8b2 shortly.  I was hoping it would get fixed by
some of bz's recent work in the menupopup stuff, but I guess not...

Comment 49

[Mike Connor [:mconnor]]

vlad says he'll get this.

Comment 50

[José Jeria]

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

Comment 51

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 1.8 patch

Because I like pain, I waited until after the branch to fix this (when I looked
at it originally, I thought my patch wasn't relevant any more.. that turned out
to not be the case).

This seems to fix the problem for me, though once again, I have no idea if its
the right fix or not...

Comment 52

[Boris Zbarsky [:bzbarsky]]

So why is this not a problem for other context menus?

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It is, it's just that bookmarks is the only place (I think) where we have a
context menu for a menu item.  In virtually all other uses you either have a
normal menu, or you have just a context menu, but not both on screen at the same
time.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  I'll try to get to this as soon as I can, but I don't know this code
that well, so it might take some time (6-7 days?)

Comment 55

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

np, I don't think anyone knows this code all that well :)

If it's any help, virtually the same patch is shipping with the 1.0.x series,
and I haven't heard of any related bugs.

Comment 56

[Boris Zbarsky [:bzbarsky]]

OK, I'm not going to be able to review this in any sort of reasonable timeframe.
 roc, could you possibly take a look at this?

Comment 57

[Phil Ringnalda (:philor)]

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

Comment 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We shouldn't minus this, it's an extremely annoying bug.  We've been shipping
with this patch in 1.0.x; i'm inclined to check it in based on that alone..

Comment 59

[Asa Dotzler [:asa]]

as this is a regression over 1.0.x and we have a patch that basically gives us
what we had there, setting the bug as a blocker. 

Comment 60

[Mike Schroepfer]

Comment on attachment 192759 [details] [diff] [review]
1.8 patch

Approved for 1.8b5.

Comment 61

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: un-bit-rotted patch for trunk

Attaching actual patch that's going in; some de-COM work was done in
nsMenuFrame that my patch did as well, so those bits conflicted.  No functional
changes.

Comment 62

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed for good.

Comment 63

[Peter van der Woude [:Peter6]]

this caused bug 310480

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I cannot reproduce on main menu. But I can rereproduce on bookmark toolbar. Is
it another bug?

2005100106-trunk/WinXP

Comment 65

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #64)
> I cannot reproduce on main menu. But I can rereproduce on bookmark toolbar. Is
> it another bug?
> 
> 2005100106-trunk/WinXP

Argh. Yeah, that's a slightly different issue.  Can you file another bug and
assingn to [e-mail]?  I think I know what the problem is.

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed new bug that is bug 31802.

-> v.

Comment 67

[Adam Guthrie]

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

Comment 68

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change