-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
- 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>() |
There was a problem hiding this comment.
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.
/// </summary> | ||
private static readonly Dictionary<char, string> s_normalizedKeys = new Dictionary<char, string>() | ||
{ | ||
{ '\uE000', "Unidentified" }, |
There was a problem hiding this comment.
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.
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 ? |
Yeah I looked there but I could only find the client libraries, not the webdriver implementation... Do you know where that might be? |
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). |
Co-authored-by: aristotelos <[email protected]>
Because the spec doesn't explain...
I've put in a few hacks in order to get existing tests to pass, so I'm marking this as ready for review. |
There was a problem hiding this 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.
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. |
Looks like `ResetInputState` must be called manually to clean up?
The spec for Element Send Keys requires dispatching actions, so actions dispatch was moved to a separate
ActionsDispatcher
class (I put this in a newServices
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