The default bug view has changed. See this FAQ.

Need to generate events for modifier key events (Cocoa widgets)

VERIFIED FIXED

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: jhp (no longer active), Assigned: Josh Aas)

Tracking

(Blocks: 1 bug, {fixed1.8.1.4})

Other Branch
PowerPC
Mac OS X
fixed1.8.1.4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Need to generate Gecko key events for the Shift, Ctrl, Option/Alt, and Command keys.

This was fixed for Carbon widgets in bug 26269.

Testcases:
https://bugzilla.mozilla.org/attachment.cgi?id=230983 (from bug 26269)
http://www.din.or.jp/~hagi3/JavaScript/JSTips/Mozilla/Samples/KeyEvent.htm
(Reporter)

Comment 1

10 years ago
Created attachment 256849 [details] [diff] [review]
patch

This patch borrows slightly from mento's Carbon patch.  I'm not too familiar with the code in this file, but I tried to keep it as clean as possible.

I wasn't too sure how to integrate with the existing code in |flagsChanged|.  Here, I always send out the key events before running the existing code;  but only for |NSFlagsChanged| events, since |flagsChanged| can get called for other events (such as |NSMouseEntered|).
Attachment #256849 - Flags: review?(joshmoz)
(Assignee)

Updated

10 years ago
Blocks: 372987
Javier, I tried to build with this patch and hit a failure:

/Users/smokey/Camino/dev/trunk/mozilla/widget/src/cocoa/nsChildView.mm: In 
   function `void -[ChildView flagsChanged:](ChildView*, objc_selector*, 
   NSEvent*)':
/Users/smokey/Camino/dev/trunk/mozilla/widget/src/cocoa/nsChildView.mm:3256: error: `
   NSDeviceIndependentModifierFlagsMask' undeclared (first use this function)
/Users/smokey/Camino/dev/trunk/mozilla/widget/src/cocoa/nsChildView.mm:3256: error: (Each
   undeclared identifier is reported only once for each function it appears 
   in.)

According to http://developer.apple.com/releasenotes/Cocoa/AppKit.html it seems that NSDeviceIndependentModifierFlagsMask constant is new in 10.4.

(I'm on 10.3.9 with the 10.3 SDK, but the 10.3.9 SDK we use for PPC builds should have the same issue.)
(Reporter)

Comment 3

10 years ago
Created attachment 257999 [details] [diff] [review]
patch w/ 10.3 fix

I'm on Intel, building with 10.4u, so didn't notice this.  This patch should work for 10.3 also.
Attachment #256849 - Attachment is obsolete: true
Attachment #257999 - Flags: review?(joshmoz)
Attachment #256849 - Flags: review?(joshmoz)

Comment 4

10 years ago
Javier, I decided to hit you with a drive-by.  This looks pretty good.  And pretty familiar, too.  :)

Can the logic in ConvertCocoaKeyEventToMacEvent be cleaned up a little?  How's this?

    UInt32 charCode = 0;
    if ([cocoaEvent type] == NSFlagsChanged) {
      macEvent.what = keyType == NS_KEY_DOWN ? keyDown : keyUp;

    } else {
      charCode = [[cocoaEvent characters] characterAtIndex:0];
      if ([cocoaEvent type] == NSKeyDown)
        macEvent.what = [cocoaEvent isARepeat] ? autoKey : keyDown;
      else
        macEvent.what = keyUp;
    }

I personally think that's cleaner, compared to essentially branching on the same condition twice to set macEvent.what and charCode.

> - (void)flagsChanged:(NSEvent*)theEvent

>+    const PRUint32 kModifierMaskTable[] =
>+      { NSShiftKeyMask, NSControlKeyMask, NSAlternateKeyMask, NSCommandKeyMask };
>+
>+    for(PRUint32 i = 0 ; i < sizeof(kModifierMaskTable) ; i++) {

sizeof(kModifierMaskTable) / sizeof(kModifierMaskTable[0]), right?

More food for thought:

flagsChanged is currently used in a weird way with that hand-scolling stuff.  Should the existing flagsChanged be split out into a different function that the existing callers can continue to call, and that the new flagsChanged with your additions can also call?

Are the cases where flagsChanged passes the event on to |super| still correct (or were they even all correct in the first place?)
A web app developer emailed the Camino list about this general issue last night, and on his testcase http://www.cormoranusa.com/Test/key.html

I get this with fx2 for ctrl-right arrow:

KeyDown: 17
KeyDown: 39
KeyPress: 39
KeyUp: 39
KeyUp: 17

and with CmTrunk with Javier's 10.3-compat patch:

KeyDown: 17
KeyDown: 17
KeyDown: 17
KeyDown: 17
KeyUp: 39
KeyUp: 17
KeyUp: 17
KeyUp: 17
KeyUp: 17

(sometimes 4 keyup/downs for ctrl, sometimes 3)
(Reporter)

Comment 6

10 years ago
(In reply to comment #4)
> Are the cases where flagsChanged passes the event on to |super| still correct
> (or were they even all correct in the first place?)

I'm not sure.  I'm not familiar with that code, so I just left it as is.  It may be safe to just drop it.

(In reply to comment #5)
> A web app developer emailed the Camino list about this general issue last
> night, and on his testcase http://www.cormoranusa.com/Test/key.html

I just built Firefox Trunk and tested this site.  I get the same results as with Fx2.  Could be a Camino only bug, but not sure why.
(Reporter)

Comment 7

10 years ago
OK, I was able to recreate the multiple events issue mentioned in comment #5.  If I load that page, then do Cmd+Option+Click anywhere on the page, then modifier key events come in twos.  Has to do with |super flagsChanged:| calling through multiple |nsChildView| objects.

However, it looks like we can't simply remove the |super| call.  It is necessary to make the cursor change to the hand when the mouse isn't moved.  It goes up the chain of |nsChildView| objects until it finds one that does need to display the hand.
(Reporter)

Comment 8

10 years ago
Created attachment 258059 [details] [diff] [review]
patch v1.2

Ignore the second part of comment #7.  I was wrong.  This patch includes Mark's suggestions, and removes the |super| call, so that multiple events shouldn't appear.

The only issue I noticed is that the hand cursor changes to a normal arrow when scrolling, but that happens on an unpatched Firefox nightly, so not my bug.
Attachment #257999 - Attachment is obsolete: true
Attachment #258059 - Flags: review?(mark)
Attachment #257999 - Flags: review?(joshmoz)
I'm getting this with the new patch and CmTrunk:

KeyDown: 17
KeyUp: 39
KeyUp: 17

So it's either 10.3 or Camino doing something odd, but I don't have a browser/ tree and don't have any idea how (read: skills) to investigate this in Camino.

Comment 10

10 years ago
This patch looks OK.  Re comment 9, the KeyDown and KeyUp for 17 are what we want to see.  There should be a KeyDown and KeyPress for 39, but that's a separate issue.  Re comment 5, we should check to see who's sending all of the different KeyDown and KeyUp events for 17 - that can probably also be handled separately.

Updated

10 years ago
Attachment #258059 - Flags: review?(mark) → review+
(In reply to comment #10)
> Re comment 9, the KeyDown and KeyUp for 17 are what we
> want to see.  There should be a KeyDown and KeyPress for 39, but that's a
> separate issue.  

OK.

> Re comment 5, we should check to see who's sending all of the
> different KeyDown and KeyUp events for 17 - that can probably also be handled
> separately.

I haven't been able to repro the tripling/quadrupling with the patch you r+ ;)
(Reporter)

Comment 12

10 years ago
Checked in to trunk.  -> FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

10 years ago
Created attachment 258307 [details] [diff] [review]
1.8.1 branch patch

For embedding with JavaXPCOM, we need to use Cocoa widgets, so it would be nice to take this on the branch.  Mark, do you see any issue with taking this on the MOZILLA_1_8_BRANCH?
Attachment #258307 - Flags: review?(mark)
We'd like this for Camino, too ;)
(In reply to comment #10)
> Re comment 9, the KeyDown and KeyUp for 17 are what we
> want to see.  There should be a KeyDown and KeyPress for 39, but that's a
> separate issue.  

Since those events worked fine for Javier in Firefox, I filed bug 373680 against Camino for the missing events, but I'll verify with the next Fx nightly that it's really a Camino issue and not a wacky 10.3 bug.
(In reply to comment #15)
> Since those events worked fine for Javier in Firefox, I filed bug 373680
> against Camino for the missing events, but I'll verify with the next Fx nightly
> that it's really a Camino issue and not a wacky 10.3 bug.

FWIW, the fix is indeed fine/complete in FxTrunk on 10.3, so bug 373680 is Camino-only.

v. with Minefield 2007-03-13-04-trunk
Status: RESOLVED → VERIFIED

Comment 17

10 years ago
Comment on attachment 258307 [details] [diff] [review]
1.8.1 branch patch

Yup, we do want this for Camino, too.
Attachment #258307 - Flags: review?(mark) → review+
(Reporter)

Updated

10 years ago
Attachment #258307 - Flags: approval1.8.1.4?
Comment on attachment 258307 [details] [diff] [review]
1.8.1 branch patch

approved for 1.8.1.4, a=dveditz for release-drivers
Attachment #258307 - Flags: approval1.8.1.4? → approval1.8.1.4+

Comment 19

10 years ago
Checked in on MOZILLA_1_8_BRANCH.  Javier, I hope you don't mind my landing this, we wanted to get it in as soon as possible for testing.

/cvsroot/mozilla/widget/src/cocoa/nsChildView.h,v  <--  nsChildView.h
new revision: 1.31.4.8; previous revision: 1.31.4.7
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.120.2.16; previous revision: 1.120.2.15
Keywords: fixed1.8.1.4
Depends on: 366695
You need to log in before you can comment on or make changes to this bug.