Last Comment Bug 371828 - Need to generate events for modifier key events (Cocoa widgets)
: Need to generate events for modifier key events (Cocoa widgets)
Status: VERIFIED FIXED
: fixed1.8.1.4
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Other Branch
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Josh Aas
:
Mentors:
http://www.din.or.jp/~hagi3/JavaScrip...
Depends on: 366695
Blocks: 372987
  Show dependency treegraph
 
Reported: 2007-02-26 14:03 PST by jhp (no longer active)
Modified: 2007-04-23 11:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.96 KB, patch)
2007-02-28 15:46 PST, jhp (no longer active)
no flags Details | Diff | Splinter Review
patch w/ 10.3 fix (6.52 KB, patch)
2007-03-09 10:06 PST, jhp (no longer active)
no flags Details | Diff | Splinter Review
patch v1.2 (10.33 KB, patch)
2007-03-09 15:34 PST, jhp (no longer active)
mark: review+
Details | Diff | Splinter Review
1.8.1 branch patch (10.37 KB, patch)
2007-03-12 09:47 PDT, jhp (no longer active)
mark: review+
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review

Description jhp (no longer active) 2007-02-26 14:03:10 PST
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
Comment 1 jhp (no longer active) 2007-02-28 15:46:51 PST
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|).
Comment 2 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-09 00:33:17 PST
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.)
Comment 3 jhp (no longer active) 2007-03-09 10:06:21 PST
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.
Comment 4 Mark Mentovai 2007-03-09 12:16:38 PST
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?)
Comment 5 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-09 13:09:18 PST
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)
Comment 6 jhp (no longer active) 2007-03-09 14:20:04 PST
(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.
Comment 7 jhp (no longer active) 2007-03-09 15:03:16 PST
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.
Comment 8 jhp (no longer active) 2007-03-09 15:34:28 PST
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.
Comment 9 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-09 19:37:52 PST
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 Mark Mentovai 2007-03-10 10:46:49 PST
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.
Comment 11 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-10 13:11:19 PST
(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+ ;)
Comment 12 jhp (no longer active) 2007-03-12 09:05:37 PDT
Checked in to trunk.  -> FIXED
Comment 13 jhp (no longer active) 2007-03-12 09:47:17 PDT
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?
Comment 14 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-12 11:14:36 PDT
We'd like this for Camino, too ;)
Comment 15 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-12 11:42:21 PDT
(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.
Comment 16 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-13 15:29:19 PDT
(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
Comment 17 Mark Mentovai 2007-03-21 13:46:02 PDT
Comment on attachment 258307 [details] [diff] [review]
1.8.1 branch patch

Yup, we do want this for Camino, too.
Comment 18 Daniel Veditz [:dveditz] 2007-03-29 11:41:08 PDT
Comment on attachment 258307 [details] [diff] [review]
1.8.1 branch patch

approved for 1.8.1.4, a=dveditz for release-drivers
Comment 19 froodian (Ian Leue) 2007-03-30 09:59:03 PDT
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

Note You need to log in before you can comment on or make changes to this bug.