Closed Bug 416390 Opened 17 years ago Closed 17 years ago

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


(Core :: XUL, defect, P2)






(Reporter: madhava, Assigned: enndeakin)



(Whiteboard: [reviewed patch in hand])


(4 files, 1 obsolete file)

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.
Seems to work on Vista, though.
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

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.
Assignee: nobody → joshmoz
Component: Download Manager → Widget: Cocoa
Product: Firefox → Core
QA Contact: download.manager → cocoa
Only the ctrl + click doesn't work for me on 10.4.  Double finger on touch pad + click and regular mouse button works fine.
Flags: blocking1.9?
Summary: Right-click does not select row → ctrl-click (right-click) does not select row
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee: joshmoz → enndeakin
Attachment #311071 - Flags: review?(neil)
Component: Widget: Cocoa → XUL Widgets
OS: Mac OS X → All
Product: Core → Toolkit
Hardware: Macintosh → All
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)?
Attachment #311071 - Flags: review?(neil) → review-
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:
Two-finger click:
Non-Cocoa (1.8), both ctrl+click and two-finger click:
On Windows:

You can test this with

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
Assignee: enndeakin → joshmoz
Component: XUL Widgets → Widget: Cocoa
Product: Toolkit → Core
I can confirm that this is a Cocoa widgets bug. We need to make sure not to regress bug 340870 when we fix this.
Attached patch fix v0.5 (obsolete) — Splinter Review
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?
should we not also add a simple test to ensure that the events are dispatched in the proper order for this bug?
Flags: in-testsuite?
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) {

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?
(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
Actually, no, we can't test widget behaviour automatically currently.
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.
Attachment #311733 - Flags: review?(smichaud)
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
Attachment #311733 - Flags: review?(smichaud) → review+
I should also say that (with this patch) I still see the results I reported in comment #2.
Attached patch widget fix v1.0Splinter Review
Attachment #311733 - Attachment is obsolete: true
Attachment #311849 - Flags: superreview?(vladimir)
Attachment #311849 - Flags: superreview?(vladimir) → superreview+
landed widget fix v1.0, waiting for Neil's part of this fix to close the bug out
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.
Assignee: joshmoz → enndeakin
Attachment #312038 - Flags: review?(neil)
Also, it would be great if someone could run this test on Linux
Assignee: enndeakin → jag
Component: Widget: Cocoa → XP Toolkit/Widgets
QA Contact: cocoa → xptoolkit.widgets
Assignee: jag → enndeakin
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.
Attachment #312038 - Flags: review?(neil) → review+
I applied the patch and ran test_contextmenu_list.xul, and it passed.
Can we get this landed and close this bug out?
Whiteboard: [reviewed patch in hand]
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.
So Neil checked this in and tests failed on centos5 :-(. Sorry, but they did work in my VM, honest!
OK, I disabled the test on Linux for now and will try to investigate at some point.
Closed: 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Fixed the test as part of bug 487631.
You need to log in before you can comment on or make changes to this bug.