Bug 111112

Summary: [chromium] Keydown event for 'shift+alt' returns win keycode instead of 'alt'
Product: WebKit Reporter: chandra shekar vallala <chandra.vallala>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
patch
none
patch
tony: review+, tony: commit-queue-
updated patch
none
rebased the patch none

Description chandra shekar vallala 2013-02-28 14:11:21 PST
Steps to reproduce the problem:
1. Create an onkeydown event handler for an html input element
2. Press shift
3. While holding shift, press alt

What is the expected behavior?
The onkeydown event for the alt keypress should contain the following:

altKey: true
which: 18

What went wrong?
The onkeydown event for the alt keypress contains the following:

altKey: false
which: 91

Ref: http://code.google.com/p/chromium/issues/detail?id=177100
Comment 1 chandra shekar vallala 2013-02-28 14:27:31 PST
Created attachment 190807 [details]
patch
Comment 2 Tony Chang 2013-03-04 13:14:05 PST
Comment on attachment 190807 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +        Added Manual Test : ManualTests/shift-alt-key-event.html
> +        Try press Shift then alt key. The test passes if the shiftKey, altKey values
> +        of JSKeyEvent are true and keycode/which is 18.

Did you try writing a layout test using eventSender?  It would be nice to know why that doesn't work.

Also, did you test this in Firefox on Linux?  What does it send?  If we are matching Firefox, please include that in the ChangeLog.
Comment 3 chandra shekar vallala 2013-03-05 16:06:17 PST
(In reply to comment #2)
> (From update of attachment 190807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190807&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Added Manual Test : ManualTests/shift-alt-key-event.html
> > +        Try press Shift then alt key. The test passes if the shiftKey, altKey values
> > +        of JSKeyEvent are true and keycode/which is 18.
> 
> Did you try writing a layout test using eventSender?  It would be nice to know why that doesn't work.
> 
Yes, I tried writing a layout test using eventSender. It didn't work since EventeSender::keyDown is unable to simulate a WebKeyboardEvent with keycode = GDK_Meta, modifier=Shift|Alt

> Also, did you test this in Firefox on Linux?  What does it send?  If we are matching Firefox, please include that in the ChangeLog.
Yes, I tried in Firefox, Opera and its working fine. Firefox do not map the keycode to Windows keycode directly, They find the right keycode for a GdkEventKey if the key event has a modifier.

Chrome key code conversion does the same in above case. 
ui/base/keycodes/keyboard_code_conversion_x.cc:791
Comment 4 chandra shekar vallala 2013-03-05 16:07:50 PST
Created attachment 191592 [details]
patch

Added Firefox behaviour in change log
Comment 5 Tony Chang 2013-03-05 16:58:11 PST
Comment on attachment 191592 [details]
patch

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

Looks like you also need to rebase your patch to ToT.

> ManualTests/shift-alt-key-event.html:8
> +function myFunction(event)

Please use a better name than myFunction.  How about keyDown?

> ManualTests/shift-alt-key-event.html:14
> +    if(event.shiftKey == true && event.altKey == true && event.which == 18) {

Nit: Put a space after if.

> ManualTests/shift-alt-key-event.html:16
> +    }else {

Nit: Space between } and else.

> ManualTests/shift-alt-key-event.html:19
> +    output.innerHTML=text;

Nit: Spaces around =.

> ManualTests/shift-alt-key-event.html:25
> +<p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=?????">bug ?????</a>:

Please fill in the bug number or just remove this text.
Comment 6 chandra shekar vallala 2013-03-05 17:13:27 PST
Created attachment 191610 [details]
updated patch

Done.
Comment 7 chandra shekar vallala 2013-03-08 14:48:39 PST
Created attachment 192288 [details]
rebased the patch

Hi Tony,

Please look at the rebased patch.
Comment 8 WebKit Review Bot 2013-03-08 15:29:21 PST
Comment on attachment 192288 [details]
rebased the patch

Clearing flags on attachment: 192288

Committed r145276: <http://trac.webkit.org/changeset/145276>
Comment 9 WebKit Review Bot 2013-03-08 15:29:25 PST
All reviewed patches have been landed.  Closing bug.