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

RESOLVED FIXED

Status

()

Core
XUL
P2
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: madhava, Assigned: Neil Deakin (not available until Aug 9))

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reviewed patch in hand])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 302147 [details]
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.
(Reporter)

Comment 1

10 years ago
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.
(Reporter)

Updated

9 years ago
Flags: blocking1.9?
Summary: Right-click does not select row → ctrl-click (right-click) does not select row

Updated

9 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Created attachment 311071 [details] [diff] [review]
fix so that the selected item is changed instead of the current item in muti-select lists
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 5

9 years ago
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

Updated

9 years ago
Duplicate of this bug: 381942

Comment 8

9 years ago
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

9 years ago
Created attachment 311733 [details] [diff] [review]
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?
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 14

9 years ago
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.

Comment 17

9 years ago
Created attachment 311849 [details] [diff] [review]
widget fix v1.0
Attachment #311733 - Attachment is obsolete: true
Attachment #311849 - Flags: superreview?(vladimir)
Attachment #311849 - Flags: superreview?(vladimir) → superreview+

Comment 18

9 years ago
landed widget fix v1.0, waiting for Neil's part of this fix to close the bug out
Created attachment 312038 [details]
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.
Assignee: joshmoz → enndeakin
Status: NEW → ASSIGNED
Attachment #312038 - Flags: review?(neil)
Also, it would be great if someone could run this test on Linux

Updated

9 years ago
Assignee: enndeakin → jag
Status: ASSIGNED → NEW
Component: Widget: Cocoa → XP Toolkit/Widgets
QA Contact: cocoa → xptoolkit.widgets

Updated

9 years ago
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+

Updated

9 years ago
Duplicate of this bug: 425676
Depends on: 425787
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
It works both ways.
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
Last Resolved: 9 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.