Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix incorrect keyboard key code on down and up #807

Merged
merged 4 commits into from
Mar 3, 2023
Merged

Fix incorrect keyboard key code on down and up #807

merged 4 commits into from
Mar 3, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Mar 3, 2023

This fixes an issue that was found while working with a Dockerized browser and running the fillform.js test. It turns out that we were setting the wrong code on DispatchKeyEvent CDP command.

This was verified by taking a look at how the KeyDescription was being built and from there it was clear we were using the wrong code.

The change was tested with fillform.js after the fix and we're no longer seeing the following warnings:

[0303/123453.108267:WARNING:keycode_converter.cc(235)] unrecognized code string 'a'
[0303/123453.158086:WARNING:keycode_converter.cc(235)] unrecognized code string 'a'
[0303/123453.211439:WARNING:keycode_converter.cc(235)] unrecognized code string 'd'
[0303/123453.277041:WARNING:keycode_converter.cc(235)] unrecognized code string 'd'
[0303/123453.360240:WARNING:keycode_converter.cc(235)] unrecognized code string 'm'
[0303/123453.419135:WARNING:keycode_converter.cc(235)] unrecognized code string 'm'
[0303/123453.469978:WARNING:keycode_converter.cc(235)] unrecognized code string 'i'
[0303/123453.521013:WARNING:keycode_converter.cc(235)] unrecognized code string 'i'
[0303/123453.569520:WARNING:keycode_converter.cc(235)] unrecognized code string 'n'
[0303/123453.621017:WARNING:keycode_converter.cc(235)] unrecognized code string 'n'
[0303/123453.820499:WARNING:keycode_converter.cc(235)] unrecognized code string '1'
[0303/123453.873024:WARNING:keycode_converter.cc(235)] unrecognized code string '1'
[0303/123453.920364:WARNING:keycode_converter.cc(235)] unrecognized code string '2'
[0303/123453.978734:WARNING:keycode_converter.cc(235)] unrecognized code string '2'
[0303/123454.037274:WARNING:keycode_converter.cc(235)] unrecognized code string '3'
[0303/123454.087314:WARNING:keycode_converter.cc(235)] unrecognized code string '3'

Fixes: #806

@ankur22
Add code to the keyboard definitions
8b4e293
@ankur22
Fix to use the src Code in the local key def
2ac59e3 
1. Key cannot be empty value, so this check isn't needed.
2. Since we've now defined the Code in the original us keyboard
   definition file, we can safely copy the src Code to the new local
   keyDef

Fixes: #806
@ankur22
Add a test to ensure correct code value
cf7b5a5 
The code for a key should not be a (for example), but something like
KeyA. This test ensures that we're sticking to this behaviour.
@ankur22 ankur22 requested review from inancgumus and ka3de March 3, 2023 14:05
inancgumus
inancgumus approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice.

common/keyboard_test.go Outdated Show resolved Hide resolved
@ankur22
Add tests for all known key values
39c4459 
This test goes through all the known key values on the keyboard. Note
that the key value can associate to more than one code.

Resolves: #807 (comment)
ka3de
ka3de approved these changes Mar 3, 2023
View reviewed changes
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ankur22 ankur22 merged commit 1bb18f8 into main Mar 3, 2023
@ankur22 ankur22 deleted the fix/806-code-warning branch March 3, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ka3de ka3de ka3de approved these changes

@inancgumus inancgumus inancgumus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warnings when typing in dockerised browser
3 participants
@ankur22 @inancgumus @ka3de