416390 - ctrl-click (right-click) does not select row

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Madhava Enros [:madhava]]

Attachment: context menu should be about the one under the click, but it isn't

Right-clicking on a row in the download manager doesn't select that row, which means that the context-menu that comes up, no matter where you click, is related to the currently selected row (see attachment for example).  Unless you realize what's going on, you end up performing actions on a download other than your intended one.

Comment 1

[Madhava Enros [:madhava]]

Seems to work on Vista, though.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

I find that right-click works fine, and only see the problem you
report when using a ctrl-left-click.

This is still a problem, though.

And since it's most likely a Cocoa widgets bug, I'm changing the
categorization.

I tested on OS X 10.5.1, with today's nightly.

Judging from the picture, you also seem to be using "Minefield", which
is a name for pre-release distros of what will become Firefox 3.

Comment 3

[Shawn Wilsher :sdwilsh]

Only the ctrl + click doesn't work for me on 10.4.  Double finger on touch pad + click and regular mouse button works fine.

Comment 4

[Neil Deakin]

Attachment: fix so that the selected item is changed instead of the current item in muti-select lists

Comment 5

[[e-mail]]

Comment on attachment 311071 [details] [diff] [review]
fix so that the selected item is changed instead of the current item in muti-select lists

Feel free to convince me that they're wrong, but I've got two suggestions:
1. Is there any reason why this doesn't belong in the listitem binding?
2. Should the code resemble the mousedown handler in the listitem binding (which also happens takes care of the usual right-click case)?

Comment 6

[Neil Deakin]

Hmm, yes, it looks like that contextmenu handler isn't actually needed, as the handler for the inherited listitem should do the trick.

So I think this is probably a Cocoa bug.

Ctrl+click results in the following event sequence:
  contextmenu
  mousedown
  mouseup
Two-finger click:
  mousedown
  contextmenu
  mouseup
  click
  mouseup
Non-Cocoa (1.8), both ctrl+click and two-finger click:
  mousdown
  contextmenu
  mouseup
On Windows:
  mousedown
  mouseup
  click
  contextmenu

You can test this with http://www.xulplanet.com/ndeakin/tests/xul/listtest.xul

The bug is caused because the Ctrl+click case doesn't fire mousedown until after the contextmenu. Note also another bug where two mouseups are fired for the two-finger click.

It would probably be best if the events were fired the same as 1.8

Comment 8

[Josh Aas]

I can confirm that this is a Cocoa widgets bug. We need to make sure not to regress bug 340870 when we fix this.

Comment 9

[Josh Aas]

Attachment: fix v0.5

This fixes the event ordering in Cocoa widgets but it doesn't fix this bug. Neil - can you confirm that this is the event ordering you want and see what is wrong with the listboxes?

Comment 10

[Shawn Wilsher :sdwilsh]

should we not also add a simple test to ensure that the events are dispatched in the proper order for this bug?

Comment 11

[Neil Deakin]

OK, that patch fixes the event ordering.

The bug now is that the listitem mousedown handler has these lines, which
filter out cases where control is pressed.

if (!event.ctrlKey && !event.shiftKey && !event.metaKey) {
  if (!this.selected) {
    control.selectItem(this);

So we could just change this to (possibly in an ifdef Mac block):

if ((!event.ctrlKey || event.button == 2) && !event.shiftKey && !event.metaKey)
{
 ...

Neil, any thoughts?

Comment 12

[Neil Deakin]

(In reply to comment #10)
> should we not also add a simple test to ensure that the events are dispatched
> in the proper order for this bug?
> 

I'll write a test with the toolkit part of this patch

Comment 13

[Neil Deakin]

Actually, no, we can't test widget behaviour automatically currently.

Comment 14

[Josh Aas]

Comment on attachment 311733 [details] [diff] [review]
fix v0.5

Starting review on the widget patch in case we end up wanting to take it.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 311733 [details] [diff] [review]
fix v0.5

This looks fine to me.  And I can confirm that it causes the
contextmenu event to be sent to Gecko after the mousedown event when
you ctrl-click (the same order these events are sent when you
right-click or two-finger click).  In my limited testing, this doesn't
seem to cause any problems.

(Just so that people are aware:  The underlying problem is that the OS
calls menuForEvent: (which sends the contextmenu event to Gecko)
before it calls mouseDown: on a ctrl-click, but calls menuForEvent:
after it calls rightMouseDown: on a right-click (or a two-finger
click).)

Comment 16

[Steven Michaud [:smichaud] (Retired)]

I should also say that (with this patch) I still see the results I reported in comment #2.

Comment 17

[Josh Aas]

Attachment: widget fix v1.0

Comment 18

[Josh Aas]

landed widget fix v1.0, waiting for Neil's part of this fix to close the bug out

Comment 19

[Neil Deakin]

Attachment: allow control key to be down for mac for selection changing

This also fixes up a related test which checks contextmenus on richlistboxes that is currently disabled and includes a few extra checks for this bug.

Comment 20

[Neil Deakin]

Also, it would be great if someone could run this test on Linux

Comment 21

[[e-mail]]

Comment on attachment 312038 [details]
allow control key to be down for mac for selection changing

Sorry, but I don't have a linux build with tests enabled.

Comment 23

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

I applied the patch and ran test_contextmenu_list.xul, and it passed.

Comment 24

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

Can we get this landed and close this bug out?

Comment 25

[Neil Deakin]

Great, thanks! But did you run it separately (where it runs in a full content area) or in the frame? I think the issue on Mac was caused because the frame was too small.

I'll check this in tomorrow.

Comment 26

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

It works both ways.

Comment 27

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

So Neil checked this in and tests failed on centos5 :-(. Sorry, but they did work in my VM, honest!

Comment 28

[Neil Deakin]

OK, I disabled the test on Linux for now and will try to investigate at some point.

Comment 29

[Neil Deakin]

Fixed the test as part of bug 487631.