Closed Bug 1332279 Opened 7 years ago Closed 7 years ago

Actions keyDown/keyUp do not set the keyCode

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
53 Branch
Type:
defect

Tracking

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: lucast1533, Assigned: impossibus)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

Bug 1332279 - Include keyCode in KeyboardEvents synthesized for key actions;
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1332279 - Test keyDown action for all WebDriver special keys;
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1332279 - Test key actions backspace behaviour;
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug_1332279_f54.patch
10.60 KB, patch
Details | Diff | Splinter Review
Bug_1332279_f53.patch
10.60 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce:

Add keydown/keyup listeners to a page:

document.addEventListener("keydown", function(event){console.log(event)});
document.addEventListener("keyup", function(event){console.log(event)});

Using actions, perform a keydown & keyup of 'a'

Json sent was:
{"actions":[{"type":"key","id":"keyboard","actions":[{"type":"keyDown","value":"a"},{"type":"keyUp","value":"a"}]}]}


Actual results:

Events were logged to the console with keyCode of 0

keydown { target: body, key: "a", charCode: 0, keyCode: 0 }
keyup { target: body, key: "a", charCode: 0, keyCode: 0 }


Expected results:

Events should have included a keyCode of 65

keydown { target: body, key: "a", charCode: 0, keyCode: 65 }
keyup { target: body, key: "a", charCode: 0, keyCode: 65 }

Typing into the browser itself produces the correct keycode of 65.
Using element#send_keys to the <body> element also produces the correct keycode of 65.
Page I used during testing was http://the-internet.herokuapp.com/key_presses
Flags: needinfo?(mjzffr)
Depends on: 1320389
Are you using Marionette?  Marionette does not yet support actions, so I somewhat doubt you got that input to work.  Perhaps you are reporting a bug on FirefoxDriver, which is not maintained by Mozilla?
Flags: needinfo?(lucast1533)
This is gecko + Firefox Nightly
From my understanding, Actions are implemented in Nightly and they are in fact working, they just aren't sending the keyCode
If you're using Nightly + geckodriver 0.13 and sending "performActions" as the command then you're using the new actions implementation.

Marionette suppresses the keyCode on purpose [1], partly because keyCode is deprecated, partly because it wasn't behaving as expected in testing/marionette/event.js if I recall correctly. That being said, I agree that we should match whatever the UA actually does as closely as possible, so I'm happy to look into the details and fix. Keeping the need-info flag set so I don't forget.


[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/action.js#815
Alright, I only pointed this out for consistency since the Element.send_keys endpoint `/session/{session id}/element/{element id}/value` does set the keyCode. If it's deprecated then so be it
Either way, thanks for the detailed and clear report.
Flags: needinfo?(lucast1533)
Hmmm, it seems that Firefox actually still cares about keyCode, which, etc. for some "special" characters, at least. I just reproduced an actions bug someone mentioned a while ago, wherein the Backspace key does nothing in a text field even though Marionette sets key and code correctly when synthesizing the KeyEvent. I've confirmed locally that forcing Marionette to set the legacy keyCode for Backspace gets the key actions working as expected.

So, it looks like we'll be fixing this sooner rather than later. :)
Assignee: nobody → mjzffr
Flags: needinfo?(mjzffr)
Great! I also noticed that issue with arrow keys, Home/End, and CTRL I believe so I'm assuming those will get fixed by this as well.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: webdriver
Priority: -- → P1
[mass] Setting priority
Comment on attachment 8853635 [details]
Bug 1332279 - Include keyCode in KeyboardEvents synthesized for key actions;

https://reviewboard.mozilla.org/r/125720/#review128262
Attachment #8853635 - Flags: review?(ato) → review+
Comment on attachment 8853636 [details]
Bug 1332279 - Test keyDown action for all WebDriver special keys;

https://reviewboard.mozilla.org/r/125722/#review128264

::: testing/web-platform/tests/webdriver/actions/special_keys.py:1
(Diff revision 1)
> +# META: timeout=long

FYI you may be interested that I’ve submitted https://bugzilla.mozilla.org/show_bug.cgi?id=1352463 to increase the default timeouts.

::: testing/web-platform/tests/webdriver/actions/support/keys.py
(Diff revision 1)
>          "key": "+",
>          "location": 3,
>          "meta": False,
>          "shift": False,
>          "value": u"\ue025",
> -        "which": 0,

Why has ‘which’ gone missing?
Attachment #8853636 - Flags: review?(ato) → review+
Comment on attachment 8853637 [details]
Bug 1332279 - Test key actions backspace behaviour;

https://reviewboard.mozilla.org/r/125724/#review128266
Attachment #8853637 - Flags: review?(ato) → review+
Comment on attachment 8853636 [details]
Bug 1332279 - Test keyDown action for all WebDriver special keys;

https://reviewboard.mozilla.org/r/125722/#review128264

> FYI you may be interested that I’ve submitted https://bugzilla.mozilla.org/show_bug.cgi?id=1352463 to increase the default timeouts.

Cool. Thanks for the quick review, btw.

> Why has ‘which’ gone missing?

In retrospect, I don't think it makes sense for wdspec tests to check `which` because it's deprecated and I believe it varies by browser anyway (for keypress at least). Instead we check `key` and `code`, which are actually mentioned in the WebDriver spec, as well as key behaviour.
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14abe9aca211
Include keyCode in KeyboardEvents synthesized for key actions; r=ato
https://hg.mozilla.org/integration/autoland/rev/e5a01985b6ce
Test keyDown action for all WebDriver special keys; r=ato
https://hg.mozilla.org/integration/autoland/rev/4a7345bdd3d4
Test key actions backspace behaviour; r=ato
https://hg.mozilla.org/mozilla-central/rev/14abe9aca211
https://hg.mozilla.org/mozilla-central/rev/e5a01985b6ce
https://hg.mozilla.org/mozilla-central/rev/4a7345bdd3d4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please uplift to 54 and 53, a=testonly. I've attached a patch file for each. 

Note that I'm proposing to only uplift the first commit in the series pushed to m-c. Please let me know if that's a problem. I'm not uplifting the other commits because they consist of tests that are incompatible with previous revisions of wptharness, and we don't want to uplift those. Related to that, the existing wdspec tests on aurora are set to "expected:fail" anyway.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-aurora/rev/28ada033908a
status-firefox54: --- → fixed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Depends on: 1358020
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: