Bug 86694 – Implement DOM_KEY_LOCATION_LEFT and RIGHT of KeyboardEvent's location property

• Component: UI Events
• Status: RESOLVED FIXED

[Takashi Sakamoto]

Add DOM_KEY_LOCATION_LEFT and DOM_KEY_LOCATION_RIGHT to distinguish  between left-control and right-control, between left shift and right shift, and between left menu and right menu.
Currently only DOM_KEY_LOCATION_STANDARD and DOM_KEY_LOCATION_NUMPAD are implemented.

The location property is defined in the W3C DOM3 specification 
http://www.w3.org/TR/DOM-Level-3-Events/#events-keyboardevents

[Takashi Sakamoto]

Created attachment 142419 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 142419 [details]
Patch

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

Looks great.

> Source/WebCore/ChangeLog:25
> +        (WebCore):

Unhelpful cruft like this can be removed from ChangeLogs. This is just a bug in the script - it's meant to be helpful, not authoritative.

Thank you for providing helpful comments!

> Source/WebCore/dom/KeyboardEvent.cpp:57
> +static inline int getWindowsVirtualCodeWithoutLocation(int keycode)

In WebCore coding style, "get" prefix means that the result is returned via an out argument. So, just "windowsVirtualCodeWithoutLocation" would be right.

> Source/WebCore/dom/KeyboardEvent.cpp:74
> +static inline unsigned getKeyLocation(const PlatformKeyboardEvent& key)

Ditto.

Also, I don't understand why this returns unsigned, and not KeyboardEvent::KeyLocationCode.

> Tools/DumpRenderTree/mac/EventSendingController.mm:644
> +    } else if ([character isEqualToString:@"rightMenu"]) {

Perhaps "leftAlt" and "rightAlt" would be better for these. On Windows, the key that maps to VK_MENU is Alt, and on Mac, it's Option - calling that "Menu" is surprising and confusing, regardless of how the Windows API constant is named.

> LayoutTests/fast/events/keydown-leftright-keys.html:51
> +    testKeyEventWithLocation("rightShift", VK_SHIFT, KEY_LOCATION_RIGHT);

If you put KEY_LOCATION_RIGHT in quotes, will test output become easier to read?

[Takashi Sakamoto]

Created attachment 142640 [details]
Patch

[Takashi Sakamoto]

Thank you for reviewing.

(In reply to comment #2)
> (From update of attachment 142419 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142419&action=review
> 
> Looks great.
> 
> > Source/WebCore/ChangeLog:25
> > +        (WebCore):
> 
> Unhelpful cruft like this can be removed from ChangeLogs. This is just a bug in the script - it's meant to be helpful, not authoritative.

I see. I removed.

> 
> Thank you for providing helpful comments!
> 
> > Source/WebCore/dom/KeyboardEvent.cpp:57
> > +static inline int getWindowsVirtualCodeWithoutLocation(int keycode)
> 
> In WebCore coding style, "get" prefix means that the result is returned via an out argument. So, just "windowsVirtualCodeWithoutLocation" would be right.

Done.

> > Source/WebCore/dom/KeyboardEvent.cpp:74
> > +static inline unsigned getKeyLocation(const PlatformKeyboardEvent& key)
> 
> Ditto.

Done.

> Also, I don't understand why this returns unsigned, and not KeyboardEvent::KeyLocationCode.

I see. I don't have enough reason.
Done.

> > Tools/DumpRenderTree/mac/EventSendingController.mm:644
> > +    } else if ([character isEqualToString:@"rightMenu"]) {
> 
> Perhaps "leftAlt" and "rightAlt" would be better for these. On Windows, the key that maps to VK_MENU is Alt, and on Mac, it's Option - calling that "Menu" is surprising and confusing, regardless of how the Windows API constant is named.

Done.

> 
> > LayoutTests/fast/events/keydown-leftright-keys.html:51
> > +    testKeyEventWithLocation("rightShift", VK_SHIFT, KEY_LOCATION_RIGHT);
> 
> If you put KEY_LOCATION_RIGHT in quotes, will test output become easier to read?

I agree. Done.

[Alexey Proskuryakov]

Comment on attachment 142640 [details]
Patch

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

> Source/WebCore/dom/KeyboardEvent.cpp:74
> +static inline int keyLocationCode(const PlatformKeyboardEvent& key)

You changed the returned value to int, not to KeyboardEvent::KeyLocationCode. Is there something preventing you from using the latter?

[Takashi Sakamoto]

Created attachment 142955 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #5)
> (From update of attachment 142640 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142640&action=review
> 
> > Source/WebCore/dom/KeyboardEvent.cpp:74
> > +static inline int keyLocationCode(const PlatformKeyboardEvent& key)
> 
> You changed the returned value to int, not to KeyboardEvent::KeyLocationCode. Is there something preventing you from using the latter?

Sorry. I misunderstood your comment. I changed the code to use KeyboardEvent::KeyLocationCode type for returning value type of the function: WebCore::keyLocationCode.

Best regards,
Takashi Sakamoto

[Hajime Morrita]

SInce you already got r+, you can
- Rewrite "Reviewed by" line with the reviewer's name and
- Upload the refined patch with no r?, but with cq+.

[Takashi Sakamoto]

Created attachment 143202 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 143202 [details]
Patch

As long as you are not a committer, you could have just set cq? on the previous patch.

[WebKit Review Bot]

Comment on attachment 143202 [details]
Patch

Clearing flags on attachment: 143202

Committed r118001: <http://trac.webkit.org/changeset/118001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

You updated keyDown in DRT, but forgot about WTR:

http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r118030%20(7431)/fast/events/keydown-leftright-keys-pretty-diff.html

I have a fix for this, but please try to remember about WTR in the future.

[Jessie Berlin]

Fixed WTR in http://trac.webkit.org/changeset/118064.

[Takashi Sakamoto]

(In reply to comment #14)
> Fixed WTR in http://trac.webkit.org/changeset/118064.

Thank you for fixing WTR, Jessie.
I didn't notice that I had to modify WTR. 

Best regards,
Takashi Sakamoto

[Jessie Berlin]

(In reply to comment #15)
> (In reply to comment #14)
> > Fixed WTR in http://trac.webkit.org/changeset/118064.
> 
> Thank you for fixing WTR, Jessie.
> I didn't notice that I had to modify WTR. 
> 
> Best regards,
> Takashi Sakamoto

Windows also needed fixing: http://trac.webkit.org/changeset/118540

[Yusuke Sato]

It seems that on Chrome for Windows (canary channel) the location property is still always zero.

[Yusuke Sato]

Looks like you need to fix WebInputEventFactory::keyboardEvent() in Source/WebKit/chromium/src/win/WebInputEventFactory.cpp too:

WebKeyboardEvent WebInputEventFactory::keyboardEvent(HWND hwnd, UINT message,
                                                     WPARAM wparam, LPARAM lparam)
{
    WebKeyboardEvent result;

    // TODO(pkasting): http://b/1117926 Are we guaranteed that the message that  
    // GetMessageTime() refers to is the same one that we're passed in? Perhaps  
    // one of the construction parameters should be the time passed by the       
    // caller, who would know for sure.                                          
    result.timeStampSeconds = GetMessageTime() / 1000.0;

    result.windowsKeyCode = static_cast<int>(wparam);  // <--------------- HERE