Bug 52606 – keyLocation for KeyboardEvent object doesn't always have correct value

• Component: DOM
• Status: UNCONFIRMED

[Nicholas C. Zakas]

When a keyboard event is fired, the keyLocation property of the event object is always either 0 (for main keyboard) or 3 (for numeric keypad), but never 1 (for left) or 2 (for right). Since I believe keyLocation is intended to mimic the location property as defined in DOM Level 3 Events, I think this is a bug.

I've tested with the following code:

var textbox = document.getElementById("myTextbox");
textbox.addEventListener("keydown", function(event){
    alert(event.keyLocation);
}, false);

Then, try the left Shift key and the right Shift key. For the left Shift key, the value of keyLocation should be 1 and for the right Shift key it should be 2.

Note: I am on Windows, I don't have a Mac to verify if it's just a Windows issue.

[Devon Govett]

I can confirm that this is not only a Windows issue as the same issue occurs on the Mac as well.

Created attachment 86603 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 86603 [details]
Patch

I don't think that it's OK to only implement this for a small subset of platforms.

[Darin Adler]

Comment on attachment 86603 [details]
Patch

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

I see no attempt to set these flags correctly for Qt or Mac or EFL. It would be much better if the patch covered those too. Typically tests make clear what’s missing on what platforms so the lack of a test here is a problem.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

A bug fix like this needs a test. And you can’t land something that still has any OOPS other than the Reviewed by line which is filled in automatically.

> Source/WebCore/dom/KeyboardEvent.cpp:73
> +    , m_keyLocation(DOM_KEY_LOCATION_STANDARD)
>      , m_altGraphKey(false)
>  {
> +    if (key.isKeypad())
> +        m_keyLocation = DOM_KEY_LOCATION_NUMPAD;
> +    else if (key.isLeftKey())
> +        m_keyLocation = DOM_KEY_LOCATION_LEFT;
> +    else if (key.isRightKey())
> +        m_keyLocation = DOM_KEY_LOCATION_RIGHT;

It would be more elegant to have a function to do this and set m_keyLocation directly to the correct value instead of first setting it to standard and then overwriting it.

> Source/WebCore/platform/PlatformKeyboardEvent.h:155
> +        bool isLeftKey() const { return m_isLeftKey; }
> +        bool isRightKey() const { return m_isRightKey; }

This should be an enum with three values rather than two separate booleans. Left and right are not independent. Separate booleans is inelegant and can lead to unnecessary ambiguity.

It would probably be best to use the same four value design as the DOM itself: numpad, left, right, or none of the above.

(In reply to comment #4)
> (From update of attachment 86603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86603&action=review
> 
> I see no attempt to set these flags correctly for Qt or Mac or EFL. It would be much better if the patch covered those too. Typically tests make clear what’s missing on what platforms so the lack of a test here is a problem.
> > Source/WebCore/ChangeLog:10
> > +        No new tests. (OOPS!)
> 
> A bug fix like this needs a test. And you can’t land something that still has any OOPS other than the Reviewed by line which is filled in automatically.

Yes; I was hoping to have a quick chat with you about exactly what tests would look like.

> > Source/WebCore/platform/PlatformKeyboardEvent.h:155
> > +        bool isLeftKey() const { return m_isLeftKey; }
> > +        bool isRightKey() const { return m_isRightKey; }
> 
> This should be an enum with three values rather than two separate booleans. Left and right are not independent. Separate booleans is inelegant and can lead to unnecessary ambiguity.
> 
> It would probably be best to use the same four value design as the DOM itself: numpad, left, right, or none of the above.

Agreed.  The only problem switching to the four-value system is that it'll break source compatibility with any code that generates PlatformKeyboardEvents manually, because the m_isKeypad field and accessor will be gone.  That can be addressed easily enough, though.

[Ahmad Saleem]

NOTE - keyLocation was non-standard and was removed from web-specification in favor of location ('keyLocation' was alias of it).

It is removed from WebKit, in this commit:

https://github.com/WebKit/WebKit/commit/bba680e17fee76325be7facc38899eda3f43d419

I am not sure whether we have to fix it in terms of 'location' or not. So will leave for others to confirm.