Bug 97161 – Mac Chromium: Ignore system numpad modifier

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sailesh Agrawal]

Mac Chromium: Ignore system numpad modifier

[Sailesh Agrawal]

Created attachment 164821 [details]
Patch

[Sailesh Agrawal]

This is a fix for http://crbug.com/145108
The flash app is getting confused because of the numpad modifier attached to the arrow keys.

NPAPI Flash handles this by ignoring the system numpad modifier.

We could do the same in PPAPI Flash or we could do it in WebKit.

Please take a look and let me know what you think.

Thanks,
Sailesh

[Nico Weber]

Comment on attachment 164821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164821&action=review

> Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:-139
> -        return true;

Won't this change how keyboard events are delivered to js key handlers?

If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.

[Sailesh Agrawal]

> Won't this change how keyboard events are delivered to js key handlers?
Yea it will.

> If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.

The only downside is that we'll diverge from Mac Safari. I think that's ok though.

[Viet-Trung Luu]

(In reply to comment #4)
> > Won't this change how keyboard events are delivered to js key handlers?
> Yea it will.
> 
> > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.

With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.

> The only downside is that we'll diverge from Mac Safari. I think that's ok though.

[Nico Weber]

(In reply to comment #5)
> (In reply to comment #4)
> > > Won't this change how keyboard events are delivered to js key handlers?
> > Yea it will.
> > 
> > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> 
> With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.

On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.

Is doing this change in flash much harder?

[Sailesh Agrawal]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > > Won't this change how keyboard events are delivered to js key handlers?
> > > Yea it will.
> > > 
> > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > 
> > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> 
> On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> 
> Is doing this change in flash much harder?

The flash change is already out for review. I wanted to have a discussion about doing this on the WebKit side first.

Also, if we decide not to do this change in WebKit then should the change be in Flash or Pepper?

[Nico Weber]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > Yea it will.
> > > > 
> > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > 
> > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > 
> > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> > 
> > Is doing this change in flash much harder?
> 
> The flash change is already out for review. I wanted to have a discussion about doing this on the WebKit side first.
> 
> Also, if we decide not to do this change in WebKit then should the change be in Flash or Pepper?

I don't care too much. Do you expect to have any pepper plugins besides flash? I guess the pdf viewer? Does it make a difference for these plugins? Does the npapi plugin path do it, or is it done in flash itself for npapi flash? Probably doesn't matter too much either way I think.

[Viet-Trung Luu]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > > Won't this change how keyboard events are delivered to js key handlers?
> > > Yea it will.
> > > 
> > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > 
> > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> 
> On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.

This is an argument for everyone continuing to do UA sniffing?

By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.

> 
> Is doing this change in flash much harder?

No, but it's dumb to.

Then every NaCl game would have to UA sniff.

I guess we could do it in Pepper.

But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.

[Viet-Trung Luu]

Also, fixing this for everyone in WebKit almost certainly only makes things better, and probably doesn't hurt anyone. What are the possibilities?

- you never look at the flag => no effect for you
- you look at the flag and but don't UA/platform sniff => you're probably broken on Mac (or on Windows, which is less likely)
- you look at the flag and do UA/platform sniff => you're probably ignoring the flag for arrow keys on Mac => no effect

(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > Yea it will.
> > > > 
> > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > 
> > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > 
> > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> 
> This is an argument for everyone continuing to do UA sniffing?
> 
> By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.
> 
> > 
> > Is doing this change in flash much harder?
> 
> No, but it's dumb to.
> 
> Then every NaCl game would have to UA sniff.
> 
> I guess we could do it in Pepper.
> 
> But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.

[Nico Weber]

(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > Yea it will.
> > > > 
> > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > 
> > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > 
> > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> 
> This is an argument for everyone continuing to do UA sniffing?
> 
> By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.

[citation needed]

> > Is doing this change in flash much harder?
> 
> No, but it's dumb to.
> 
> Then every NaCl game would have to UA sniff.
> 
> I guess we could do it in Pepper.
> 
> But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.

"Adobe have reported a minor inconsistency in flapper. Let's change how keyboard events on the web are handled!" doesn't sound all that coherent to me.

If you want to change js key events, that should be done independent of random flash bugs, and in safari and chrome at the same time. We decided to be safari-compatible in our keyboard handling on mac, and so far that's worked well I think.

[Viet-Trung Luu]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > > Yea it will.
> > > > > 
> > > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > > 
> > > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > > 
> > > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> > 
> > This is an argument for everyone continuing to do UA sniffing?
> > 
> > By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.
> 
> [citation needed]
> 
> > > Is doing this change in flash much harder?
> > 
> > No, but it's dumb to.
> > 
> > Then every NaCl game would have to UA sniff.
> > 
> > I guess we could do it in Pepper.
> > 
> > But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.
> 
> "Adobe have reported a minor inconsistency in flapper. Let's change how keyboard events on the web are handled!" doesn't sound all that coherent to me.

No, it just shows that no one has ever paid close attention to web key events.

> 
> If you want to change js key events, that should be done independent of random flash bugs, and in safari and chrome at the same time. We decided to be safari-compatible in our keyboard handling on mac, and so far that's worked well I think.

Do you think Safari doesn't have bugs?!?!?!?

The consequence of not having consistent events is:
- WEB content won't work properly on Mac (more so in markets where Macs are less common); this will become a more frequent problem as people try to do more using JS ... unless they code SPECIFICALLY for Mac
- NaCl content won't work on Mac, again unless they code SPECIFICALLY for Mac
- Flash content won't work on Mac

You're confusing the symptom with the underlying illness. We get the event wrong in Flash because we get the event wrong in Pepper because we get the event wrong in WebKit.

Yes, we could trivially fix it in Flash, but that's dumb. It forces everyone else, at EVERY OTHER LEVEL to similarly detect that they're on a Mac, and fix the issue for themselves. The real consequence is that many people writing content on Windows will produce content that's broken on Mac, and vice versa.

[Viet-Trung Luu]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > > > > Won't this change how keyboard events are delivered to js key handlers?
> > > > > > Yea it will.
> > > > > > 
> > > > > > > If npapi flash does this, it sounds like ppapi should do this too instead of it happening here.
> > > > > > I was talking to Trung about this. Currently Chrome on Mac is inconsistent with Chrome on Windows. It seems like fixing it such that they're both consistent would be better.
> > > > > > Also, having the numpad modifier on arrow keys is very unexpected and should probably be treated like a bug.
> > > > > 
> > > > > With respect to key events, the behavior on Windows (probably of Netscape) should essentially be considered canonical, AFAIU.
> > > > 
> > > > On Mac, we've historically treated Safari's behavior canonical. It also has the advantage of being compatible with the current chrome/mac behavior.
> > > 
> > > This is an argument for everyone continuing to do UA sniffing?
> > > 
> > > By canonical, I basically mean that the spec is based off of browsers on Windows. It's a bug in Safari, plain and simple.
> > 
> > [citation needed]

I suppose you think Macs produce Windows virtual key codes naturally.

> > 
> > > > Is doing this change in flash much harder?
> > > 
> > > No, but it's dumb to.
> > > 
> > > Then every NaCl game would have to UA sniff.
> > > 
> > > I guess we could do it in Pepper.
> > > 
> > > But then Pepper and JS would disagree about events. I guess everyone would still have to UA sniff anyway.
> > 
> > "Adobe have reported a minor inconsistency in flapper. Let's change how keyboard events on the web are handled!" doesn't sound all that coherent to me.
> 
> No, it just shows that no one has ever paid close attention to web key events.
> 
> > 
> > If you want to change js key events, that should be done independent of random flash bugs, and in safari and chrome at the same time. We decided to be safari-compatible in our keyboard handling on mac, and so far that's worked well I think.
> 
> Do you think Safari doesn't have bugs?!?!?!?
> 
> The consequence of not having consistent events is:
> - WEB content won't work properly on Mac (more so in markets where Macs are less common); this will become a more frequent problem as people try to do more using JS ... unless they code SPECIFICALLY for Mac
> - NaCl content won't work on Mac, again unless they code SPECIFICALLY for Mac
> - Flash content won't work on Mac
> 
> You're confusing the symptom with the underlying illness. We get the event wrong in Flash because we get the event wrong in Pepper because we get the event wrong in WebKit.
> 
> Yes, we could trivially fix it in Flash, but that's dumb. It forces everyone else, at EVERY OTHER LEVEL to similarly detect that they're on a Mac, and fix the issue for themselves. The real consequence is that many people writing content on Windows will produce content that's broken on Mac, and vice versa.

[Nico Weber]

Kinuko, Trung points out that you added this bit of code in http://trac.webkit.org/changeset/52608/trunk/WebCore/platform/mac/KeyEventMac.mm . Do you remember why it was necessary?

[Viet-Trung Luu]

Also, the current Safari does not have this bug.

[Sailesh Agrawal]

(In reply to comment #15)
> Also, the current Safari does not have this bug.

Safari on 10.6 doesn't have this bug either, weird!
Their code still have the numpad modifier check though:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/mac/PlatformEventFactoryMac.mm#L293
Maybe the modifier gets attached to the javascript event elsewhere.

Anyways, fixing this bug in WebKit seems like a clear choice now.

[Alexey Proskuryakov]

Please always post all relevant information about a bug to Bugzilla. Collecting bits of information from multiple bug trackers is not fun for anyone who hasn't been on the bug from the start.

(In reply to comment #2)
> This is a fix for http://crbug.com/145108
> The flash app is getting confused because of the numpad modifier attached to the arrow keys.

URL/test case :http://www.nick.com/games/teenage-mutant-ninja-turtles-sewer-run.html

Steps:
1. let game load, then choose the following: Play ->Story -> Skip -> Leo(left most choice) -> Play
2. after character is controllable press 'z' (character should lunge forward)
3. hold down an arrow key and hit z(lunge won't occur)

Expected: Should be able to hold arrow keys and press z to lunge forward at the same time. 

Observed:Multiple button presses while holding an arrow key, that IS NOT another arrow key won't be registered within the game. You must release the arrow key to use other buttons.

[Sailesh Agrawal]

Created attachment 167135 [details]
Patch

[Sailesh Agrawal]

Replying to Kinuko:

> The reason I added the change is to distinguish key location (if it's from NUMPAD or not) for arrow keys.
(The issue link in the ChangeLog seems to be wrong)
https://bugs.webkit.org/show_bug.cgi?id=32602

Yea, your original patch looks good. My CL will simply suppress that behavior for arrow keys. Mac keyboards don't have arrow keys on the number pad so this should be ok.

> I don't remember the details well but I'm afraid your change may affect the layout test (fast/events/keydown-numpad-keys.html) and how we used to report KeyLocation information.  Did you see if the patched version still reports the key location correctly?

Yep it does.
I've also made a chromium-mac version of your expectations file with the num pad modifier removed from the arrow key events.

[Sailesh Agrawal]

Hi Tony, would you be a good person to review this? Thanks

[Eric Seidel (no email)]

Comment on attachment 167135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167135&action=review

> LayoutTests/ChangeLog:10
> +        * platform/chromium-mac/fast/events/keydown-numpad-keys-expected.txt: Added.

I'm confused why we have our own set of expecations here?  These look identical to the ones in fast/events.  platform/mac doesn't have expecxtations.  Did you try running webkit-patch optimize-baselines?

[Sailesh Agrawal]

(In reply to comment #21)
> (From update of attachment 167135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167135&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        * platform/chromium-mac/fast/events/keydown-numpad-keys-expected.txt: Added.
> 
> I'm confused why we have our own set of expecations here?  These look identical to the ones in fast/events.

The only difference is that I removed the numpad modifier for keycodes 37 to 40 (arrow keys).

> platform/mac doesn't have expecxtations.

This is skipped for mac-wk2/Skipped.
I'm not sure about mac though. I tested Safari and the behavior matches my patch.

> Did you try running webkit-patch optimize-baselines?

I haven't seen this before. Looking into it now.

Thanks,
Sailesh

[Tony Chang]

It sounds like the discussion is saying that Safari has the same bug as Chromium Mac.  Can we fix both?  Also, what does Firefox Mac do?

[Sailesh Agrawal]

(In reply to comment #23)
> It sounds like the discussion is saying that Safari has the same bug as Chromium Mac.  Can we fix both?  Also, what does Firefox Mac do?

Mac Safari does not have this bug.
Pressing arrow keys never sets the number pad modifier.

[Tony Chang]

(In reply to comment #24)
> (In reply to comment #23)
> > It sounds like the discussion is saying that Safari has the same bug as Chromium Mac.  Can we fix both?  Also, what does Firefox Mac do?
> 
> Mac Safari does not have this bug.
> Pressing arrow keys never sets the number pad modifier.

Why is the Chromium Mac expected result for keydown-numpad-keys.html different from the Apple Mac result (or from the Chromium Windows result)?

[Sailesh Agrawal]

Created attachment 167594 [details]
Patch

[Sailesh Agrawal]

> Why is the Chromium Mac expected result for keydown-numpad-keys.html different from the Apple Mac result (or from the Chromium Windows result)?

Hi Tony, here's the summary of what's going on:
  #1 A normal keyboard has arrow keys on the normal keyboard and on the number pad.
  #2 Mac keyboards have arrows keys only on the normal keyboard.
  #3 Mac Safari (correctly) does not attach number pad modifier when sending arrow keys to Javascript/DOM.
  #4 Mac Chromium (incorrectly) does attach number pad modifier when sending arrow keys to Javascript/DOM.
  #5 Windows Chromium (correctly) does not attach number pad modifier for normal arrow keys and does attach it for number pad arrow keys.

At this point I fixed #4 by changing WebInputEventFactory::keyboardEvent().

There's one complication though. Layout  tests behave completely differently. Layout tests don't use WebInputEventFactory so we get a completely different behavior:
  #1 Mac Safari (incorrectly) attaches num pad modifier when sending arrow keys.
  #2 Mac Chromium (incorrectly) attaches num pad modifier when sending arrow keys.

This patch fixes #2 by changing EventSender::keyDown() to strip number pad modifier for arrow keys.

I'm going to file a separate bug for #1. The fix for #1 involves changing PlatformEventFactoryMac.mm/isKeypadEvent() and changing the relevant test.

[Tony Chang]

Comment on attachment 167594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167594&action=review

It sounds like we can't actually test this behavior in a layout test because the event created by eventSender doesn't go through the port specific WebKit code.  I would just revert the changes to DRT and the layout test.  The unittest you added seems sufficient to test the change to Chromium's WebKit code.  Trying to make DRT smarter just makes the layout test more confusing.

> Source/WebKit/chromium/WebKit.gypi:136
> +                    'tests/WebInputEventFactoryTestMac.mm',

Nit: I think you can put this in the main webkit_unittest_files list and it'll get removed on Win/Linux because of the .mm extension.

> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:43
> +using WebKit::WebInputEvent;
> +using WebKit::WebKeyboardEvent;
> +using WebKit::WebMouseEvent;
> +using WebKit::WebInputEventFactory;

Nit: Sort.

> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:62
> +}  // namespace

Nit: WebKit style is only one space before // comments.

[Sailesh Agrawal]

Created attachment 167609 [details]
Patch for landing

[Sailesh Agrawal]

Comment on attachment 167594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167594&action=review

>> Source/WebKit/chromium/WebKit.gypi:136
>> +                    'tests/WebInputEventFactoryTestMac.mm',
> 
> Nit: I think you can put this in the main webkit_unittest_files list and it'll get removed on Win/Linux because of the .mm extension.

Done.

>> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:43
>> +using WebKit::WebInputEventFactory;
> 
> Nit: Sort.

Done.

>> Source/WebKit/chromium/tests/WebInputEventFactoryTestMac.mm:62
>> +}  // namespace
> 
> Nit: WebKit style is only one space before // comments.

Done.

[WebKit Review Bot]

Comment on attachment 167609 [details]
Patch for landing

Clearing flags on attachment: 167609

Committed r130691: <http://trac.webkit.org/changeset/130691>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.