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.

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

Implement Element Send Keys according to spec #33

Merged
merged 21 commits into from
May 8, 2024

Conversation

grokys
Copy link
Contributor

@grokys grokys commented May 6, 2024

The spec for Element Send Keys requires dispatching actions, so actions dispatch was moved to a separate ActionsDispatcher class (I put this in a new Services namespace, let me know if it should go somewhere else).

To implement properly, also required implementing input sources. Only key input sources are currently implemented. Also required adding an Id property to the actions objects.

This implementation tries to follow the spec to the letter, even when it probably doesn't make a ton of sense. I figured it can deviate from the spec in a separate PR if it's decided that makes sense. I'll point out some places where it might want to deviate in the comments.

I think I've also found some issues with the spec: w3c/webdriver#1809 w3c/webdriver#1810. I've added unit tests and workarounds for the issues.

There is a deviation from the spec due to missing APIs in UIA. I've called that out in a comment: the behavior follows the WinAppDriver behavior here rather than the spec.

There is also an ignored test for what is I believe a FlaUI issue: FlaUI/FlaUI#320

grokys added 11 commits May 4, 2024 00:17
- The spec is unclear on how to tell if a cluster is a modifier. Maybe normalizing the cluster is the right thing to do, maybe it's not
- Add the input ID to the action sequence
Makes tests pass more consistently. There is probably a better way to do this.
As far as I can see this failing test is caused by a defect in the spec w3c/webdriver#1809?
/// <summary>
/// Normalized key mapping from https://www.w3.org/TR/webdriver2/#keyboard-actions
/// </summary>
private static readonly Dictionary<char, string> s_normalizedKeys = new Dictionary<char, string>()
Copy link
Contributor Author

@grokys grokys May 6, 2024

Choose a reason for hiding this comment

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

The spec maps these unicode code points to a string value, but this would probably be better expressed in C# using an enum.

src/FlaUI.WebDriver/Keys.cs Outdated Show resolved Hide resolved
/// </summary>
private static readonly Dictionary<char, string> s_normalizedKeys = new Dictionary<char, string>()
{
{ '\uE000', "Unidentified" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unicode chars in this list should maybe use the constants defined below, but that makes checking that they're correct a bit more difficult as it involves a double-lookup for each.

@grokys grokys marked this pull request as draft May 6, 2024 12:01
@grokys
Copy link
Contributor Author

grokys commented May 6, 2024

Marked this as a draft due to w3c/webdriver#1810.

I get the feeling that I'm the first person to try to implement this stuff according to the spec... Do you know what is considered the reference implementation of the WebDriver spec?

@aristotelos
Copy link
Collaborator

Marked this as a draft due to w3c/webdriver#1810.

I get the feeling that I'm the first person to try to implement this stuff according to the spec... Do you know what is considered the reference implementation of the WebDriver spec?

I think the reference implementation would be https://github.com/SeleniumHQ/selenium/tree/trunk/java/src/org/openqa/selenium ?

@grokys
Copy link
Contributor Author

grokys commented May 6, 2024

I think the reference implementation would be https://github.com/SeleniumHQ/selenium/tree/trunk/java/src/org/openqa/selenium ?

Yeah I looked there but I could only find the client libraries, not the webdriver implementation... Do you know where that might be?

@aristotelos
Copy link
Collaborator

I am a bit puzzled by the spec about input sources. Is their purpose to be able to see if the modifier keys are already down on an element? E.g., checking if the Shift key is already down on an element so that we do not try to send a key down event on the Shift key when we try to type a character that requires the Shift key?

@grokys
Copy link
Contributor Author

grokys commented May 6, 2024

I am a bit puzzled by the spec about input sources. Is their purpose to be able to see if the modifier keys are already down on an element? E.g., checking if the Shift key is already down on an element so that we do not try to send a key down event on the Shift key when we try to type a character that requires the Shift key?

That seems to be part of the purpose. The other purpose seems to be resetting the key state at the end of an operation, though this part of the spec looks to me like it's been written without a reference implementation and seems to contain quite a few oversights (though it may just be my reading of the spec).

grokys and others added 3 commits May 6, 2024 17:05
@grokys
Copy link
Contributor Author

grokys commented May 6, 2024

I've put in a few hacks in order to get existing tests to pass, so I'm marking this as ready for review.

@grokys grokys marked this pull request as ready for review May 6, 2024 20:54
Copy link
Collaborator

@aristotelos aristotelos left a comment

Choose a reason for hiding this comment

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

I've added some comments after reading the specification more closely on this point. I am very happy with the improvements so far, but would like to see those comments addressed.

src/FlaUI.WebDriver/Controllers/ElementController.cs Outdated Show resolved Hide resolved
src/FlaUI.WebDriver/Controllers/ActionsController.cs Outdated Show resolved Hide resolved
@aristotelos
Copy link
Collaborator

I think the reference implementation would be https://github.com/SeleniumHQ/selenium/tree/trunk/java/src/org/openqa/selenium ?

Yeah I looked there but I could only find the client libraries, not the webdriver implementation... Do you know where that might be?

I think that some of the implementation can be found there, see e.g. https://github.com/SeleniumHQ/selenium/blob/trunk/java/src/org/openqa/selenium/interactions/Actions.java. However, the browsers have their own driver components as well, and I am not sure how much they implement... and they are maybe not open source.

grokys added 2 commits May 8, 2024 09:53
Looks like `ResetInputState` must be called manually to clean up?
@aristotelos aristotelos merged commit 18a6075 into FlaUI:main May 8, 2024
3 checks passed
@aristotelos aristotelos linked an issue May 8, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element Send Keys does not follow spec.
2 participants