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

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: madhava, Assigned: enndeakin)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reviewed patch in hand])

Attachments

(4 attachments, 1 obsolete attachment)

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
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.
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
Status: NEW → ASSIGNED
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:
  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
Assignee: enndeakin → joshmoz
Status: ASSIGNED → NEW
Component: XUL Widgets → Widget: Cocoa
Product: Toolkit → Core
Duplicate of this bug: 381942
I can confirm that this is a Cocoa widgets bug. We need to make sure not to regress bug 340870 when we fix this.
Posted 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) {
    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?
(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
click).)
Attachment #311733 - Flags: review?(smichaud) → review+
I should also say that (with this patch) I still see the results I reported in comment #2.
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
Status: NEW → ASSIGNED
Attachment #312038 - Flags: review?(neil)
Also, it would be great if someone could run this test on Linux
Assignee: enndeakin → jag
Status: ASSIGNED → NEW
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+
Duplicate of this bug: 425676
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.
Status: NEW → ASSIGNED
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.
Status: ASSIGNED → RESOLVED
Closed: 12 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.