Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Description • 7 years ago

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.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 1 • 7 years ago

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}]}"

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 2 • 7 years ago

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) == ""

	David Burns :automatedtester
	Comment 3 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 6 years ago

Annoyance for a lot of people. Taking.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 9 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 14 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 16 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 20 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 21 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 22 • 6 years ago

(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?

	Andreas Tolfsen ❲:ato❳
	Comment 23 • 6 years ago

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”.

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 25 • 6 years ago

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?

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 6 years ago

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?

	Andreas Tolfsen ❲:ato❳
	Comment 27 • 6 years ago

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?

	Andreas Tolfsen ❲:ato❳
	Comment 28 • 6 years ago

(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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 29 • 6 years ago

(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?

	Andreas Tolfsen ❲:ato❳
	Comment 30 • 6 years ago

(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!

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 31 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 32 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 33 • 6 years ago

(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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 34 • 6 years ago

(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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 35 • 6 years ago

(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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 36 • 6 years ago

(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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 38 • 6 years ago

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.

	Pulsebot
	Comment 39 • 6 years ago

Pushed by hskupin@mozilla.com:
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

	Andreea Pavel [:apavel]
	Comment 40 • 6 years ago
bugherder

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 41 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 42 • 6 years ago

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

	James Graham [:jgraham]
	Comment 43 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 44 • 6 years ago

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)

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 45 • 6 years ago

Upstream PR merged