389542 - [Mac only] Consecutive right/control/context-clicks only pop context menu the first time

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Wayne Woods]

Since 20070705, if you context(right/control)-click in a window to bring up a context menu, and then immediately try to repeat the procedure, all consecutive context clicks fail to pop the context menu again. Left/primary clicking, or briefly changing the focus usually allows the context menu to pop once more (and once in row only).

With ctrl-click, the bug isn't as obvious as with right-click, because in the former the context menu sticks open, while in the latter it vanishes on the second click. But if you move the mouse around as you ctrl-click, you'll notice that the context menu fails to move to the new click location. 

This occurs in Firefox/SeaMonkey browser and Thunderbird message windows. I think it's Mac-specific.

The bug doesn't seem to occur when clicking in the URL and search fields of the location bar, but does occur in form fields.

The regression range is between 20070704-04 and 20070705-01. Looking at Bonsai, bug 279703 stands out as a likely culprit.

Comment 1

[Neil Deakin]

Yes, there's another bug on this somewhere but I can't find it at the moment.

Comment 2

[Wayne Woods]

I couldn't see any relevant Mac bugs with "menu" or "click" from the regression date onwards, but maybe I missed something.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I've seen this too.  I'm not sure when it started happening.

I plan to do another "consolidated Cocoa widgets popup window" patch
(like the one I just landed for bug 387164).  There's at least one
other popup problem that needs to be fixed.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a fix for this bug.

As Wayne noticed, it was triggered by a change made by the patch for
bug 279703.  Specifically, that patch started hiding popup windows
asynchronously (using the nsXULPopupHiding event).  But this means
that, if a custom context menu is already open, the new one gets
"shown" before the old one has been "hidden".  This (somehow) confuses
the XUL popup manager, and results in the symptoms reported in comment
#0.

My solution is to also send the NS_CONTEXTMENU Gecko event
asynchronously (using the nsCustomContextMenuEvent event) -- though
only on browsers that use custom context menus (if I do this in
Camino, it completely disables all context menus).

Besides fixing this bug, my patch also does a little incidental
cleanup:  There were two places from which an NS_CONTEXTMENU event
could be sent (ChildView's menuForEvent: and mouseDown: methods).  But
I found that things work fine without the code that sends an
NS_CONTEXTMENU event from the mouseDown: method, so I dropped it.

Comment 6

[Neil Deakin]

Actually, it seems that the patch in bug 388995 fixes this, or part of it anyway. Make sure to test with that patch though.

It may be easier to just make Rollup use synchronous events if that is the problem. Why don't other platforms have the bug?

I suppose as this patch contains the changes in bug 381106 that that bug is going to be fixed by this too.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 274941 [details] [diff] [review]
Fix

Thanks for telling me about bug 388995 and bug 381106 -- I didn't know
about either of them.

> I suppose as this patch contains the changes in bug 381106 that that
> bug is going to be fixed by this too.

Great minds think alike :-)

> Actually, it seems that the patch in bug 388995 fixes this, or part
> of it anyway. Make sure to test with that patch though.

If it's possible to completely fix this problem in the layout module,
I agree that's the place to do it.

I'll test with your patch for bug 381106 ... but not right away (since
I'm going to be gone on vacation next week).

In the meantime I guess we should just put aside this problem (and my
patch for it).

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Is Bug 389344 a version of this same issue. It seems like it is (and I managed to log it first!). :-)

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> Is Bug 389344 a version of this same issue.

Yes, it seems to be.

But I could only tell by testing whether my fix for this bug also
fixes bug 389344 (it does).

Which is another way of saying that this bug is a much simpler (and
therefore better) example of the problem.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Works for me. I'm not concerned about which bug is the master here, just whether it is fixed. :-)

Comment 11

[Wayne Woods]

As predicted, looks like bug 388995 fixed it, thanks Neil (and thanks Steven for looking at it!)

Comment 13

[Neil Deakin]

Stephen, is there anything in this patch that is still needed? Also, do you think the patch in bug 381106 is the right patch for that bug?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #11)

> As predicted, looks like bug 388995 fixed it, thanks Neil (and
> thanks Steven for looking at it!)

The patch for bug 388995 _doesn't_ fix this problem ... at least not
in tests I just did on today's Minefield nightly
(2007-08-14-04-trunk).  It just slightly changes the symptoms.

Every second right-click now brings up a context menu.  But the
symptoms for ctrl-click remain exactly the same -- the first context
menu stays open on each subsequent ctrl-click.

Comment 15

[Wayne Woods]

Hmmm, you're right. It fixes the problem for right clicks (at least tested using a Mighty Mouse), but not control clicks.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

I get the same (bad) results right-clicking my Mighty Mouse as I do
with my Microsoft Mouse (with left and right buttons and a scroll
wheel that can double as a middle button) -- only every second
right-click brings up a context menu.

Comment 17

[Wayne Woods]

So that's twice in a row I've put my foot in my mouth. Sorry! I thought what you described was normal behaviour for right click (second time hides the menu), but I see that it's not the case. It's been hard to test with my Mighty Mouse given that it has its own frequent right click problems (3rd replacement.. trying to make Apple give me my money back now ;) )

Right click behaviour is different to ctrl-click, where the menu freezes open in the one spot, but as you point out, still buggy.

Comment 18

[Josh Aas]

Comment on attachment 274941 [details] [diff] [review]
Fix

Please make it more clear in the comments what you mean by "custom context menu" - maybe either use Camino as an example, or associate that phrase with the particular pref value. This can be changed on checkin.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Revised patch landed on trunk.

Comment 20

[Neil Deakin]

Note that this isn't really the right way we want to fix this. We should either:

- figure out why other platforms don't need asynchronous contextmenu event
- figure out why the Mac doesn't require this for leftclick type menus (or does it)
- fix bug 392652

Comment 21

[Steven Michaud [:smichaud] (Retired)]

> - figure out why other platforms don't need asynchronous contextmenu
>   event

I suspect there _are_ problems on other platforms ... just more subtle
ones (that might not have been noticed yet).

> - figure out why the Mac doesn't require this for leftclick type
>   menus (or does it)

Without this patch, both right-click and ctrl-click are broken (see
comment #14 and comment #16).

> - fix bug 392652

Yes.  But I suspect that'll take a while.  When you've got a fix we
can reconsider this patch.  But I suspect we'll need to keep using it
anyway, if only to fix bug 385408 (which are is fixed by my patch).

Comment 22

[Josh Aas]

The plan is that if someone fixes 392652 then we'll get rid of this change. Neil - do you think you'll be able to do that any time soon?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> The plan is that if someone fixes 392652 then we'll get rid of this
> change.

Be aware that bug 385408 might still need something like this patch,
even if synchronous Gecko popup events work out perfectly.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> Be aware that bug 385408 might still need something like this patch,
> even if synchronous Gecko popup events work out perfectly.

This may also be true for bug 381477.

Comment 25

[Neil Deakin]

(In reply to comment #22)
> The plan is that if someone fixes 392652 then we'll get rid of this change.
> Neil - do you think you'll be able to do that any time soon?
> 

That bug is fairly easy to fix. I've looked at all the callers and the only thing that needs to change, besides flipping the flag in nsXULPopupManager::Rollup, is the two calls to Rollup in nsMenuFrame and nsMenuBarFrame should be changed to call HidePopup instead.