451870 - ctrl+mouseup doesn't set left button in event

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Shane Caraveo]

Attachment: osx-mouseup.patch

ctrl+mousedown shows event.button = 2
ctrl+mouseup shows event.button = 0

patch attached

Comment 1

[Steven Michaud [:smichaud] (Retired)]

This is a Cocoa widgets bug, not a Mac widgets bug (Mac widgets are
what Firefox 2.X and prior versions used.)

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 335194 [details] [diff] [review]
osx-mouseup.patch

Furthermore, when you submit a patch you should ask someone to review
it.  Normally this would be Josh Aas.  But since your patch is very
straightforward I'm going to r+ it myself.  For good measure I'll also
ask Josh to review it.

Comment 3

[Josh Aas]

Comment on attachment 335194 [details] [diff] [review]
osx-mouseup.patch

   [self convertCocoaMouseEvent:theEvent toGeckoEvent:&geckoEvent];
 
+  unsigned int modifierFlags = [theEvent modifierFlags];
+  if (modifierFlags & NSControlKeyMask)


Get rid of the newline between "[self convert..." and the code you're adding so it matches mouseDown: stylistically, and there is no reason to create a variable for the modifier flags. Just use the obj-c call in place of the var.

Nice catch, thanks!

Comment 4

[Josh Aas]

Attachment: fix v1.1

updated patch to reflect my comments

Comment 5

[Josh Aas]

landed on trunk

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 337924 [details] [diff] [review]
fix v1.1

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 7

[Josh Aas]

landed for 1.9.0.4

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Someone correct me if I'm wrong here.

The old behavior is:

ctrl+mousedown shows event.button = 2
ctrl+mouseup shows event.button = 0

The new (correct) behavior is:

ctrl+mousedown shows event.button = 2
ctrl+mouseup shows event.button = 2

This is what I'm seeing when I compare 1.9.0.3 and 1.9.0.4 on my OS X box.

Comment 9

[Josh Aas]

That looks correct to me.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Marking as verified then. Thanks, Josh.