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

RESOLVED FIXED in Firefox 64

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: maja_zf, Assigned: whimboo)

Tracking

(Blocks 2 bugs)

Version 3
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(URL)

Attachments

(8 attachments, 3 obsolete attachments)

13.91 KB, patch
ato
: review+
Details | Diff | Splinter Review
7.19 KB, patch
ato
: review+
Details | Diff | Splinter Review
1.11 KB, patch
ato
: review+
Details | Diff | Splinter Review
3.44 KB, patch
ato
: review+
Details | Diff | Splinter Review
21.79 KB, patch
ato
: review+
Details | Diff | Splinter Review
18.77 KB, patch
ato
: review+
Details | Diff | Splinter Review
1.52 KB, patch
ato
: review+
Details | Diff | Splinter Review
2.08 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
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.
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}]}"
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) == ""
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Priority: P3 → P2
Annoyance for a lot of people. Taking.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
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.
Blocks: 1418995
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.
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.
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
To avoid confusion with the global "window" object we should
avoid using this name as function argument.
Attachment #9010876 - Flags: review?(ato)
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.
Attachment #9010878 - Flags: review?(ato)
As already mentioned in the comment above the code it is all
done automatically by "event.synthesizeKey()".
Attachment #9010879 - Flags: review?(ato)
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.
Attachment #9010880 - Flags: review?(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.
Attachment #9010875 - Flags: review?(ato) → review+
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.
Attachment #9010876 - Flags: review?(ato) → review-
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.
Attachment #9010877 - Flags: review?(ato) → review+
(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?
Flags: needinfo?(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 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.
Attachment #9010879 - Flags: review?(ato) → review+
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?
Attachment #9010880 - Flags: review?(ato) → review+
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?
Attachment #9010881 - Flags: review?(ato) → review+
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?
Attachment #9010882 - Flags: review?(ato) → review+
(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.
Flags: needinfo?(ato)
(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?
Flags: needinfo?(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!
Flags: needinfo?(ato)
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.
Attachment #9011037 - Flags: review?(ato)
Attachment #9010876 - Attachment is obsolete: true
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.
Attachment #9011039 - Flags: review?(ato)
Attachment #9010878 - Attachment is obsolete: true
Attachment #9010878 - Flags: review?(ato)
(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
(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.
(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.
(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.
Attachment #9010882 - Attachment is obsolete: true
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.
Attachment #9011046 - Flags: review+
Attachment #9011037 - Flags: review?(ato) → review+
Attachment #9011039 - Flags: review?(ato) → review+

Comment 39

8 months 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
James, looks like something blocks the sync with upstream here?
Flags: needinfo?(james)
Depends on: 1493546
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13199 for changes under testing/web-platform/tests
This was blocked on landing the PR that reorganised the directory structure.
Flags: needinfo?(james)
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)
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.