Bug 205997 - WebDriver: pressed virtual keys not correctly handled in action sequences
Summary: WebDriver: pressed virtual keys not correctly handled in action sequences
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-09 06:19 PST by Carlos Garcia Campos
Modified: 2020-01-13 01:32 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2020-01-09 06:22 PST, Carlos Garcia Campos
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-01-09 06:19:08 PST
We are assuming that only one virtual key can be pressed and that a key up always releases the pressed virtual key if any. We should keep a list of pressed keys and remove them from the list when key up happens for them.
Comment 1 Carlos Garcia Campos 2020-01-09 06:22:25 PST
Created attachment 387216 [details]
Patch
Comment 2 Dean Jackson 2020-01-09 10:44:15 PST
Comment on attachment 387216 [details]
Patch

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

Can this be tested?

> Source/WebDriver/Session.cpp:2613
> +                            virtualKeys->pushString(virtualKey);

Where does currentState.pressedVirtualKeys get emptied?

> Source/WebDriver/Session.h:206
> +        HashSet<String> pressedVirtualKeys;

Would this be better off as a Vector? Do you want the keys replayed in order?

> Source/WebKit/ChangeLog:8
> +        When modifiers are present we need to translate the keys that might be affected by the modifiers.

I'm not sure how this is related.
Comment 3 Carlos Garcia Campos 2020-01-10 00:53:01 PST
(In reply to Dean Jackson from comment #2)
> Comment on attachment 387216 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387216&action=review
> 
> Can this be tested?

It's already tested, this fixes imported/w3c/webdriver/tests/perform_actions/key_modifiers.py::test_shift_modifier_generates_capital_letters[\ue008] and imported/w3c/webdriver/tests/perform_actions/key_modifiers.py::test_shift_modifier_generates_capital_letters[\ue050]

> > Source/WebDriver/Session.cpp:2613
> > +                            virtualKeys->pushString(virtualKey);
> 
> Where does currentState.pressedVirtualKeys get emptied?

The keys are removed when the keyup happens for them.

> > Source/WebDriver/Session.h:206
> > +        HashSet<String> pressedVirtualKeys;
> 
> Would this be better off as a Vector? Do you want the keys replayed in order?

You can get keyDown(Shift), keyDown(Control), keyUp(Shift), keyUp(Control) for example.

> > Source/WebKit/ChangeLog:8
> > +        When modifiers are present we need to translate the keys that might be affected by the modifiers.
> 
> I'm not sure how this is related.

There are actually two issues the tests, one is that the virtual key pressed is lost when any other keyup happens, and the other is that we don't translate the keyvals when modifiers are present. I need both changes to fix those tests. I can split the patch, but I think they are related enough.
Comment 4 Brian Burg 2020-01-10 13:54:18 PST
Comment on attachment 387216 [details]
Patch

r=me
Comment 5 Carlos Garcia Campos 2020-01-13 01:31:35 PST
Committed r254424: <https://trac.webkit.org/changeset/254424>
Comment 6 Radar WebKit Bug Importer 2020-01-13 01:32:13 PST
<rdar://problem/58523183>