Bug 94142 – Added modifiers to distinguish between left/right Shift/Ctrl/Alt

• Component: New Bugs
• Status: RESOLVED FIXED

[Raymes Khoury]

Added modifiers to distinguish between left/right Shift/Ctrl/Alt

[Alexey Proskuryakov]

Could you please provide more details about this issue? Specifically, I'm wondering if it has been already resolved in bug 86694.

[Raymes Khoury]

Thanks for the link (I intended on uploading a patch with this but webkit-patch upload broke in the process).

There are 2 problems with the existing behavior:
1) The Windows implementation of WebInputEventFactory never produces key events with e.g. VK_LSHIFT or VK_RSHIFT, only VK_SHIFT. 
2) The more major problem here is that after these events round-trip through WebCore and get converted back to WebKeyboardEvents, the location information is lost (see Source/WebCore/dom/KeyboardEvent.cpp:keyCode() which calls windowsVirtualKeyCode()). This means that we don't have access to the key location in plugins, for example. Furthermore, I would be concerned about converting back to the L/R versions of the keycodes because existing code relies on checking for the non-locational version of the keycode (e.g. VK_SHIFT). 

I believe that the L/R version of the keycodes should never be returned from event factories (after all they are never returned by the Windows API). We should have a separate location modifier/field in WebKeyboardEvent for specifying the location of the keypress (as in KeyboardEvent). I'll work up a patch to this effect.

[Raymes Khoury]

Created attachment 161297 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 161297 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/tests/WebInputEventConversionTest.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/tests/WebInputEventConversionTest.cpp:36:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymes Khoury]

Created attachment 161305 [details]
Patch

[WebKit Review Bot]

Attachment 161305 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/tests/WebInputEventConversionTest.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo (cr-android ews)]

Comment on attachment 161305 [details]
Patch

Attachment 161305 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13687395

[WebKit Review Bot]

Comment on attachment 161305 [details]
Patch

Attachment 161305 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13687499

[Raymes Khoury]

Created attachment 161493 [details]
Patch

[Raymes Khoury]

Ping for review

[Tony Chang]

Comment on attachment 161493 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:20
> +        modifiers. Since this only changes internal representation, no new
> +        tests are added.

Can you add a 'why' to the ChangeLog entry?  Saying that you need this for plugins would be sufficient.

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:51
> +    if (wparam == VK_SHIFT) {
> +        if (GetKeyState(VK_LSHIFT))
> +            modifier = WebInputEvent::IsLeft;
> +        else if (GetKeyState(VK_RSHIFT))

Should we move all of this into the switch below?

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:104
> +    ASSERT(modifier

Is this assert correct? Can't modifier be 0?

[Raymes Khoury]

Created attachment 162280 [details]
Patch

[Raymes Khoury]

Comment on attachment 161493 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:20
>> +        tests are added.
> 
> Can you add a 'why' to the ChangeLog entry?  Saying that you need this for plugins would be sufficient.

Done.

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:51
>> +        else if (GetKeyState(VK_RSHIFT))
> 
> Should we move all of this into the switch below?

Done.

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:104
>> +    ASSERT(modifier
> 
> Is this assert correct? Can't modifier be 0?

Yes, sorry this was introduced while fixing style.

[Tony Chang]

Comment on attachment 162280 [details]
Patch

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

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:86
> +        if (GetKeyState(VK_LSHIFT) & 0x8000)

Nit: I would probably make a function IsHighBitSet. It would be a little easier to read.

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:111
> +    ASSERT(modifier == 0

Nit: WebKit style is !modifier rather than comparing to 0.

[Raymes Khoury]

Created attachment 162340 [details]
Patch

[Raymes Khoury]

Comment on attachment 162280 [details]
Patch

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

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:86
>> +        if (GetKeyState(VK_LSHIFT) & 0x8000)
> 
> Nit: I would probably make a function IsHighBitSet. It would be a little easier to read.

Changed to isKeyDown

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:111
>> +    ASSERT(modifier == 0
> 
> Nit: WebKit style is !modifier rather than comparing to 0.

Done.

[Raymes Khoury]

*** Bug 93132 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 162340 [details]
Patch

Clearing flags on attachment: 162340

Committed r127711: <http://trac.webkit.org/changeset/127711>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.