1405370 - Synthesized key event for "Shift" doesn't result in capitalized letters

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Shift+a in an input field should result in the text "A", but we get "a".

I wrote a minimal test case and checked the synthesized events, which *do* have the "shift" property set to true as expected, but we still get lowercase letters in the input field. 

In other words, this is not a regression of https://bugzilla.mozilla.org/show_bug.cgi?id=1367430 -- Marionette does actually set the appropriate modifier properties on the event, but in the case of "shift", that doesn't result in capitalized letters in an input field.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

A wdspec testcase the reproduces this:

> def test_shift_capitalizes_keys(session, key_reporter, key_chain):
>    key_chain \
>        .send_keys("ef") \
>        .key_down(Keys.SHIFT) \
>        .key_down("c") \
>        .key_up(Keys.SHIFT) \
>        .key_up("c") \
>        .send_keys("d") \
>        .key_down(Keys.SHIFT) \
>        .send_keys("ab") \
>        .key_up(Keys.SHIFT) \
>        .perform()
>    print get_events(session)
>    assert get_keys(key_reporter) == "efCdAB"


Results in:

POST /session/c01c19f0-c534-2146-b931-31d62166f9cb/actions {"actions": [{"type": "key", "id": "keyboard_id", "actions": [{"type": "keyDown", "value": "e"}, {"type": "keyUp", "value": "e"}, {"type": "keyDown", "value": "f"}, {"type": "keyUp", "value": "f"}, {"type": "keyDown", "value": "\ue008"}, {"type": "keyDown", "value": "c"}, {"type": "keyUp", "value": "\ue008"}, {"type": "keyUp", "value": "c"}, {"type": "keyDown", "value": "d"}, {"type": "keyUp", "value": "d"}, {"type": "keyDown", "value": "\ue008"}, {"type": "keyDown", "value": "a"}, {"type": "keyUp", "value": "a"}, {"type": "keyDown", "value": "b"}, {"type": "keyUp", "value": "b"}, {"type": "keyUp", "value": "\ue008"}]}]}"


And the synthesized events are below, note that "shift" is true where we expect:

{"value":[{"code":"KeyE","ctrl":false,"key":"e","location":0,"meta":false,"repeat":false,"shift":false,"type":"keydown","which":69},{"code":"KeyE","ctrl":false,"key":"e","location":0,"meta":false,"repeat":false,"shift":false,"type":"keypress","which":101},{"code":"KeyE","ctrl":false,"key":"e","location":0,"meta":false,"repeat":false,"shift":false,"type":"keyup","which":69},{"code":"KeyF","ctrl":false,"key":"f","location":0,"meta":false,"repeat":false,"shift":false,"type":"keydown","which":70},{"code":"KeyF","ctrl":false,"key":"f","location":0,"meta":false,"repeat":false,"shift":false,"type":"keypress","which":102},{"code":"KeyF","ctrl":false,"key":"f","location":0,"meta":false,"repeat":false,"shift":false,"type":"keyup","which":70},{"code":"ShiftLeft","ctrl":false,"key":"Shift","location":1,"meta":false,"repeat":false,"shift":true,"type":"keydown","which":16},{"code":"KeyC","ctrl":false,"key":"c","location":0,"meta":false,"repeat":false,"shift":true,"type":"keydown","which":67},{"code":"KeyC","ctrl":false,"key":"c","location":0,"meta":false,"repeat":false,"shift":true,"type":"keypress","which":99},{"code":"ShiftLeft","ctrl":false,"key":"Shift","location":1,"meta":false,"repeat":false,"shift":false,"type":"keyup","which":16},{"code":"KeyC","ctrl":false,"key":"c","location":0,"meta":false,"repeat":false,"shift":false,"type":"keyup","which":67},{"code":"KeyD","ctrl":false,"key":"d","location":0,"meta":false,"repeat":false,"shift":false,"type":"keydown","which":68},{"code":"KeyD","ctrl":false,"key":"d","location":0,"meta":false,"repeat":false,"shift":false,"type":"keypress","which":100},{"code":"KeyD","ctrl":false,"key":"d","location":0,"meta":false,"repeat":false,"shift":false,"type":"keyup","which":68},{"code":"ShiftLeft","ctrl":false,"key":"Shift","location":1,"meta":false,"repeat":false,"shift":true,"type":"keydown","which":16},{"code":"KeyA","ctrl":false,"key":"a","location":0,"meta":false,"repeat":false,"shift":true,"type":"keydown","which":65},{"code":"KeyA","ctrl":false,"key":"a","location":0,"meta":false,"repeat":false,"shift":true,"type":"keypress","which":97},{"code":"KeyA","ctrl":false,"key":"a","location":0,"meta":false,"repeat":false,"shift":true,"type":"keyup","which":65},{"code":"KeyB","ctrl":false,"key":"b","location":0,"meta":false,"repeat":false,"shift":true,"type":"keydown","which":66},{"code":"KeyB","ctrl":false,"key":"b","location":0,"meta":false,"repeat":false,"shift":true,"type":"keypress","which":98},{"code":"KeyB","ctrl":false,"key":"b","location":0,"meta":false,"repeat":false,"shift":true,"type":"keyup","which":66},{"code":"ShiftLeft","ctrl":false,"key":"Shift","location":1,"meta":false,"repeat":false,"shift":false,"type":"keyup","which":16}]}"

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Interestingly, a similar testcase with ctrl passes:

> def test_ctrl_a_ctrl_x_erases_keys(session, key_reporter, key_chain):
>     key_chain \
>         .send_keys("efcd") \
>         .key_down(Keys.CONTROL) \
>         .key_down("a") \
>         .key_up(Keys.CONTROL) \
>         .key_up("a") \
>         .key_down(Keys.CONTROL) \
>         .key_down("x") \
>         .key_up(Keys.CONTROL) \
>         .key_up("x") \
>         .perform()
>     assert get_keys(key_reporter) == ""

Comment 3

[David Burns :automatedtester]

[Mass Change 2018-01-15] Moving bugs to backlog

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Annoyance for a lot of people. Taking.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The problem here is that `action.dispatchKeyDown()` is creating a `Key` dictionary with the unmodified key, and later in `event.sendSingleKey()` only the `keyName` gets uppercased, leaving the `key` property of `modifiers` alone.

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/testing/marionette/action.js#1143
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/testing/marionette/event.js#1235

Then `event.createKeyboardEventDictionary_()` is using the unchanged modifier key, which is the lower-case letter:

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/testing/marionette/event.js#629

The fix here should be to update `event.sendSingleKey()` to also update the key in `modifiers` for the correct upper-case letter.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Note that I got this fixed locally. With it you will see the removal of a lot of unused code, and what's more important the addition of more keys in the `VIRTUAL_KEYCODE_LOOKUP` table which weren't covered yet, and caused problems in correctly identifying special and modifier keys when those are located at the right side of the keyboard.

As talked with Andreas yesterday I don't have to keep parity with the EventUtils.js module. If necessary we can drift away from that copy and make our modifications where we feel it's best placed.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6599b65a9be4b29e4cfab707f56ddef8912822

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The try build shows a lot of failures for Mn jobs, mainly because the old legacy action tests are failing now. I totally missed to check those, and it's bad to see that we still have to handle them. Hopefully we can get those removed by bug 1354578 soon.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c550a8a3b1834224d1e7fbeba081e1a1614e0569

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And a follow-up try for Wdspec only because I missed to update the manifest for the essential shift key test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae134a4dfc0873402f505c1833c3a978e924efbe

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Global "window" object is not available in Javascript modules

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Avoid usage of "window" for function arguments

To avoid confusion with the global "window" object we should
avoid using this name as function argument.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Remove unused methods in event.js

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Add missing keys to virtual keyboard lookup table

Until now we only supported the special keys at the standard and
left location of the keyboard. This patch adds all the remaining
keys which are located at the right side and in the numpad area.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Remove obsolete extra keypress event firing

As already mentioned in the comment above the code it is all
done automatically by "event.synthesizeKey()".

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Synthesized key event for "Shift" has to capitalize typable characters

The current code only capitalizes the "keyName" and not the
"modifier.key" property. As such the capitalization gets lost,
because in "event.createKeyboardEventDictionary_()" the
"modifier.key" property gets precedence over "keyName".

To not have to capitalize both at this level, it's better to move
this code directly into "event.createKeyboardEventDictionary_()".

This also makes the method "event.sendSingleKey()" useless, so it
can be removed.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [wdspec] Reorganize key action tests for "Perform Actions"

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [wdspec] Add test for shift modifier produces capital letters

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010875 [details] [diff] [review]
[marionette] Global "window" object is not available in Javascript modules

Review of attachment 9010875 [details] [diff] [review]:
-----------------------------------------------------------------

This is a welcome change, but can you make the commit message explain
what the patch _does_?  It is stating a fact, but not what it is
actually doing about it.

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010876 [details] [diff] [review]
[marionette] Avoid usage of "window" for function arguments

Review of attachment 9010876 [details] [diff] [review]:
-----------------------------------------------------------------

The window global is only a thing is if you load one of the files
as a subscript, which we used to do with EventUtils.js.  It is, for
example, not a thing in either the JSMs or in frame scripts, so I
doubt there can be any confusion regarding its use in Marionette
today.

But I would really rather not want to make this change right now
becuase of the number of conflicts it would cause for in-flight
work, such as the window tracking refactoring.  Would it be a
possibility to drop it, or at least hold this specific patch off,
for some more time?  From what I can tell it’s not essential to
what you’re trying to achieve.

Comment 21

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010877 [details] [diff] [review]
[marionette] Remove unused methods in event.js

Review of attachment 9010877 [details] [diff] [review]:
-----------------------------------------------------------------

Trusting this is alright.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #20)
> Comment on attachment 9010876 [details] [diff] [review]
> [marionette] Avoid usage of "window" for function arguments
>
> But I would really rather not want to make this change right now
> becuase of the number of conflicts it would cause for in-flight
> work, such as the window tracking refactoring.  Would it be a

Why should this be a problem? As of now I don't see why your changes would have to touch those files at all. Can you please explain why that is necessary?

Comment 23

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010878 [details] [diff] [review]
[marionette] Add missing keys to virtual keyboard lookup table

Review of attachment 9010878 [details] [diff] [review]:
-----------------------------------------------------------------

From the commit message I don’t quite understand where these originate
from.  Is VIRTUAL_KEYCODE_LOOKUP a WebDriver-to-EventUtils-primitives
thing, or is it fully internal to EventUtils?

Also, I don’t understand what is meant by “we only supported the
special keys at the standard”.

Comment 24

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010879 [details] [diff] [review]
[marionette] Remove obsolete extra keypress event firing

Review of attachment 9010879 [details] [diff] [review]:
-----------------------------------------------------------------

I don’t know this code, but looks OK.

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010880 [details] [diff] [review]
[marionette] Synthesized key event for "Shift" has to capitalize typable characters

Review of attachment 9010880 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty great.

::: testing/marionette/event.js
@@ +495,2 @@
>    result.dictionary = {
> +    key: resultKey,

I find it a bit confusing what the difference between key and
resultKey is in this function.  But then I looked at
createKeyboardEventDictionary_ and I’m not sure there’s anything
that can be easily done about it… the whole function is pretty
convoluted to begin with…

Anyway, just airing my frustrations about the complexity of this
code.

@@ +1025,5 @@
> +    // For |sendKeysToElement| and legacy actions we assume that if
> +    // |keyToSend| is a raw code point (like "\uE009") then |modifiers| does
> +    // not already have correct value for corresponding |modName| attribute
> +    // (like ctrlKey), so that value needs to be flipped.
> +    let modName = MODIFIER_KEYCODES_LOOKUP[keyCode];

I had to take some time to understand what this does, but I understand
this is only called by legacyaction.js?

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010881 [details] [diff] [review]
[wdspec] Reorganize key action tests for "Perform Actions"

Review of attachment 9010881 [details] [diff] [review]:
-----------------------------------------------------------------

I didn’t check the moved content in great detail, but I saw there
were a few minor changes.  Assuming these are not world-changing.

::: testing/web-platform/tests/webdriver/tests/perform_actions/key_special_keys.py
@@ +11,2 @@
>      (u"\u0BA8\u0BBF"),
>      (u"\u1100\u1161\u11A8"),

Could we add comments explaining what we’re testing?

Comment 27

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9010882 [details] [diff] [review]
[wdspec] Add test for shift modifier produces capital letters

Review of attachment 9010882 [details] [diff] [review]:
-----------------------------------------------------------------

r+wc

s/for/that/ in commit message.

::: testing/web-platform/tests/webdriver/tests/perform_actions/key_modifiers.py
@@ +14,5 @@
> +        .send_keys("d") \
> +        .key_down(modifier) \
> +        .send_keys("ef") \
> +        .key_up(modifier) \
> +        .perform()

Can we also test that the modifier also lasts across multiple
key_down/key_up invocations?

Comment 28

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> (In reply to Andreas Tolfsen ❲:ato❳ from comment #20)
> >
> > But I would really rather not want to make this change right now
> > becuase of the number of conflicts it would cause for in-flight
> > work, such as the window tracking refactoring.  Would it be a
> 
> Why should this be a problem? As of now I don't see why your changes would
> have to touch those files at all. Can you please explain why that is
> necessary?

Yes, it will make it significantly harder to rebase my WIP patches
because it moves some of the code changed here around.

I’m not saying it is _necessary_, but I wanted to ask if you would
do me the courtesy.  I suppose I can suffer through a painful
rebasing but I wanted to put the question to you first.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #28)
> Yes, it will make it significantly harder to rebase my WIP patches
> because it moves some of the code changed here around.

Sorry I was confused with the actual patch. What if I remove the changes for driver.js?

Comment 30

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #29)
> (In reply to Andreas Tolfsen ❲:ato❳ from comment #28)
> > Yes, it will make it significantly harder to rebase my WIP patches
> > because it moves some of the code changed here around.
> 
> Sorry I was confused with the actual patch. What if I remove the changes for
> driver.js?

That would be really great!

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Avoid usage of "window" for function arguments

To avoid confusion with the global "window" object we should
avoid using this name as function argument.

Changes for driver.js are left-out and will be done by the
patch on bug 1311041.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [marionette] Add missing keys to virtual keyboard lookup table

Until now we only supported the internal Unicode to virtual
key mapping at the standard and left location of the keyboard.
This patch adds all the remaining special keys which are
located at the right side and in the numpad area.

This mapping table is somewhat equivalent to the normalized
key translation, but is using the "VK_" prefix.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #23)
> Also, I don’t understand what is meant by “we only supported the
> special keys at the standard”.

As referenced by the UI Event spec:
https://www.w3.org/TR/uievents/#events-keyboard-key-location

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #25)
> @@ +495,2 @@
> >    result.dictionary = {
> > +    key: resultKey,
> 
> I find it a bit confusing what the difference between key and
> resultKey is in this function.  But then I looked at
> createKeyboardEventDictionary_ and I’m not sure there’s anything
> that can be easily done about it… the whole function is pretty
> convoluted to begin with…
> 
> Anyway, just airing my frustrations about the complexity of this
> code.

I only join you and I hope that we can simplify all this at a later time. But it is not something I want to do now given that our support for key actions, and element send key isn't that great. Once we have fixed a lot of the bugs, and got way more tests added I will feel much safer in cleaning up this method.

> > +    // For |sendKeysToElement| and legacy actions we assume that if
> > +    // |keyToSend| is a raw code point (like "\uE009") then |modifiers| does
> > +    // not already have correct value for corresponding |modName| attribute
> > +    // (like ctrlKey), so that value needs to be flipped.
> > +    let modName = MODIFIER_KEYCODES_LOOKUP[keyCode];
> 
> I had to take some time to understand what this does, but I understand
> this is only called by legacyaction.js?

No, also by `sendKeysToElement()` as stated in this comment. It' simply when a string like `"a" + Key.Shift + "b"` is send to the remove end.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #26)
> testing/web-platform/tests/webdriver/tests/perform_actions/key_special_keys.
> py
> @@ +11,2 @@
> >      (u"\u0BA8\u0BBF"),
> >      (u"\u1100\u1161\u11A8"),
> 
> Could we add comments explaining what we’re testing?

I only moved that code, but didn't add anything. But as far as I get these are all invalid Unicode code points which cannot be used for keyUp and keyDown. I would have to dig deeper into Unicode definitions first to answer that. I hope it will be fine to add more details at a later point.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #27)
> > +        .send_keys("d") \
> > +        .key_down(modifier) \
> > +        .send_keys("ef") \
> > +        .key_up(modifier) \
> > +        .perform()
> 
> Can we also test that the modifier also lasts across multiple
> key_down/key_up invocations?

Oh, that is what `send_keys("ef")` is for. But given that this is actually only a wrapper of the webdriver client it makes indeed sense to change this to `keyDown()` and `keyUp()` for both letters.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: [wdspec] Add test that shift modifier produces capital letters

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 9011046 [details] [diff] [review]
[wdspec] Add test that shift modifier produces capital letters

Updated with review comments. Taking over r+ from ato.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56671f6f6db4
[marionette] Global "window" object is not available in Javascript modules.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83c5c065957
[marionette] Avoid usage of "window" for function arguments.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0cff38be06
[marionette] Remove unused methods in event.js.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3125cc183560
[marionette] Add missing keys to virtual keyboard lookup table. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec10aa9703c
[marionette] Remove obsolete extra keypress event firing. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/2998b10eb5a7
[marionette] Synthesized key event for "Shift" has to capitalize typable characters. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1a9d25aaf0
[wdspec] Reorganize key action tests for "Perform Actions". r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb144bdca16
[wdspec] Add test that shift modifier produces capital letters. r=ato

Comment 40

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/56671f6f6db4
https://hg.mozilla.org/mozilla-central/rev/e83c5c065957
https://hg.mozilla.org/mozilla-central/rev/dc0cff38be06
https://hg.mozilla.org/mozilla-central/rev/3125cc183560
https://hg.mozilla.org/mozilla-central/rev/eec10aa9703c
https://hg.mozilla.org/mozilla-central/rev/2998b10eb5a7
https://hg.mozilla.org/mozilla-central/rev/7c1a9d25aaf0
https://hg.mozilla.org/mozilla-central/rev/bdb144bdca16

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

James, looks like something blocks the sync with upstream here?

Comment 42

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13199 for changes under testing/web-platform/tests

Comment 43

[James Graham [:jgraham]]

This was blocked on landing the PR that reorganised the directory structure.

Comment 44

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13199
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/433007536?utm_source=github_status&utm_medium=notification)

Comment 45

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged