425787 - Context menus don't appear with ctrl-click in Camino

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This regressed sometime between 2008-03-20-01 and 2008-03-28-00; I don't have a better range than that atm.

Right-click works, but for folks without mice (or one-button mice), this is pretty bad.

STR:
1) Load page
2) Ctrl-click

AR: No CM displayed
ER: Page context menu displayed (or text or image, etc., CM, depending on what you're hovering over at the time.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Er, I also haven't checked Minefield, thus the filing here (and also our CMs are different, which might matter).

Comment 2

[Josh Aas]

Bug 416390 probably caused this. Can somebody check by backing that out?

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Yep, I backed "widget fix v1.0" out and my context menus started working again.

-->Widget:Cocoa, per Josh.

Comment 4

[Josh Aas]

Attachment: fix v1.0

Maybe this'll work, though I'm not convinced this is something we should try to check in yet.

Comment 5

[Stuart Morgan]

I haven't thought this all the way through yet (or tested it obviously), but what about reversing your current approach?
- Store the last mouse event you've mouseDown'd
- When you get a menuForEvent: where the event is a mouse-down event, if that's not the same as the stored event, manually feed it to mouseDown:
- In mouseDown:, if the event is same as the stored event, just bail.

Comment 6

[Stuart Morgan]

Attachment: comment 5 approach

This implements comment 5. If it's really the case that the correct fix for the original bug is to force the synthesis of a mouse-down that would not normally exist (the normal behavior of a control-click on OS X that shows a context menu is not to send either a mouse-down or a mouse-up) so that all context menus have an associated mouse down, this seems like a more robust approach--although if Gecko requires that, then it should probably also be synthesizing a mouse-up as well (which this doesn't do, but could be made to do the same way).

And if that is necessary, should it be synthesizing a right mouse button instead, or does core logic that relies on this behavior understand that a modified left-mouse-down could trigger a context menu? I see that menuForEvent: always claims that the context menu event it creates for Gecko always claims to be a right mouse button.


I haven't tested this with Firefox, but it doesn't appear to have any negative impact on Camino.

Comment 7

[Josh Aas]

Comment on attachment 312387 [details] [diff] [review]
comment 5 approach

This was looking pretty good until I noticed that it regresses the fix for 416390. It doesn't regress the widget behavior I fixed in that bug, but it does regress the toolkit fix to the reported bug.

Comment 8

[Josh Aas]

+  if ([theEvent type] == NSLeftMouseDown)
+    [self mouseDown:theEvent];
+  else
+    [self maybeRollup:theEvent];

This is the problem. Doesn't regress 416390 if we do this instead:

  [self maybeRollup:theEvent];
  if ([theEvent type] == NSLeftMouseDown)
    [self mouseDown:theEvent];

Now I have to re-do all of my testing and profiling to see if that breaks anything else.

Comment 9

[Stuart Morgan]

Sorry, I did that because one of the first things mouseDown: does is call maybeRollup:, and I wasn't sure if doing it twice would be an issue. I didn't realize that ensureCorrectMouseEventTarget:, which is called before that, interacts with the rollup stuff.

Comment 10

[Josh Aas]

Attachment: fix v2.0

This includes 3 changes to Stuart's patch.

- always call maybeRollup: from menuForEvent:
- fix an incorrect comment, Cocoa does sometimes call mouseDown: after menuForEvent: if menuForEvent: returns nil
- fixes a spelling error (Stuart didn't introduce it, unrelated to his patch)

Comment 11

[Steven Michaud [:smichaud] (Retired)]

This looks fine.

But (oddly) I find that it still regresses bug 416390.

I built and tested on OS X 10.5.2.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Oops, I tested with a build pulled before that patch landed.

Will have to rebuild from scratch and try again.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 313166 [details] [diff] [review]
fix v2.0

Rebuilt and retested -- no problems.

Comment 14

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

I wonder if the fix in 416390 was right. Could we have made the richlistbox just process a context menu event like a left click before showing the context menu? Do we really want ctrl-click to do everything that a left-click does?

Comment 15

[Stuart Morgan]

(If the answer is yes, I don't believe that this patch as-is is right, per comment 6.)

Comment 16

[Josh Aas]

Regardless of what happened with 416390, Camino's context menus are broken and this is a better way to write code we've had for a while.

Comment 17

[Neil Deakin]

(In reply to comment #9)
> Sorry, I did that because one of the first things mouseDown: does is call
> maybeRollup:, and I wasn't sure if doing it twice would be an issue.

Rollup() should only be called once per mouse press, otherwise extra menus might be closed that shouldn't.

> Could we have made the richlistbox just process a context menu event like a left click before showing the context menu?

Possibly, but it should not do anything if the context menu was opened via the keyboard.

For bug 416390, the goal was to be compatible with 1.8 and with other platforms, the onle difference being that context menus appear on mouseup on Windows.

Comment 18

[Josh Aas]

We don't call Rollup() twice with my patch. We call maybeRollup: twice but that is "maybe" - there is logic so Rollup() is only called once.

Comment 20

[Josh Aas]

landed on trunk