753256 - Cocoa widget shouldn't use GetCurrentKeyModifiers() directly

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, GetCurrentKeyModifiers() is used for getting *cocoa* modifier flags. But the value is different from expected value.

I'll post a patch soon.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

I think that we should discuss whether we should use event-queue-synthesized API or not. But for now, we should fix just the bug.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

This looks fine to me.

But don't you also need to handle 'rightShiftKey', 'rightOptionKey' and 'rightControlKey' in nsCocoaUtils::GetCurrentModifiers()?

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> event-queue-synthesized

You mean event-queue-synchronized?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Steven Michaud from comment #2)
> This looks fine to me.
> 
> But don't you also need to handle 'rightShiftKey', 'rightOptionKey' and
> 'rightControlKey' in nsCocoaUtils::GetCurrentModifiers()?

I've never known the constants. However, looks like ::GetCurrentKeyModifiers() doesn't return the values. On my environment, both sides modifiers cause same result.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Steven Michaud from comment #3)
> > event-queue-synthesized
> 
> You mean event-queue-synchronized?

Ah, yes.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch (altenative)

If you like this better, please r+ this.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

>> But don't you also need to handle 'rightShiftKey', 'rightOptionKey' and
>> 'rightControlKey' in nsCocoaUtils::GetCurrentModifiers()?
>
> I've never known the constants.

They're documented here (among other places):

http://developer.apple.com/legacy/mac/library/documentation/Carbon/Reference/Event_Manager/Reference/reference.html#//apple_ref/doc/uid/TP30000158-CH3g-C004232

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 623964 [details] [diff] [review]
Patch (altenative)

Looks fine to me.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 622661 [details] [diff] [review]
Patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/72349c11ceaa

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/72349c11ceaa