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 Get Element Attribute. #27

Merged
merged 16 commits into from
May 3, 2024

Conversation

grokys
Copy link
Contributor

@grokys grokys commented Apr 30, 2024

Implements /session/{session id}/element/{element id}/attribute/{name}.

Not so crazy about using reflection here, but it seems it's the only way to get the property ID from a string?

Fixes #25

TODO

@grokys
Copy link
Contributor Author

grokys commented Apr 30, 2024

The test for this feature is currently failing, even though in another scenario it works fine. I think (but I'm not sure) that it's because it's using Selenium.WebDriver whereas the other scenario is using Appium.WebDriver. Selenium.WebDriver seems to be trying to run a script to get attributes?

@grokys
Copy link
Contributor Author

grokys commented Apr 30, 2024

Yep, confirmed that the problem is the Selenium.WebDriver package. Changing to use <PackageReference Include="Appium.WebDriver" Version="5.0.0-rc.8" /> allows the tests to run.

grokys added a commit to grokys/FlaUI.WebDriver that referenced this pull request Apr 30, 2024
This allows the `Get Element Attribute` test in FlaUI#27 to work. Seems like Selenium.WebDriver tries to execute a script in order to read attributes?
@grokys grokys force-pushed the feature/get-element-attribute branch from e7f6666 to 5952e4e Compare April 30, 2024 21:33
@aristotelos
Copy link
Collaborator

Yep, confirmed that the problem is the Selenium.WebDriver package. Changing to use <PackageReference Include="Appium.WebDriver" Version="5.0.0-rc.8" /> allows the tests to run.

I did a quick test, GetAttribute indeed converts to a script. But you can use GetDomAttribute instead which maps to the attribute API.

I am OK converting the tests to use Appium.WebDriver, but I think keeping the freedom to use Selenium.WebDriver as a user is also good. Can we document both options in the README? I.e. we should document that Selenium.WebDriver's GetAttribute is not supported but GetDomAttribute is. (I am also still wondering if there is a Selenium option to use the attribute API when calling GetAttribute.)

@aristotelos
Copy link
Collaborator

aristotelos commented May 1, 2024

Can we add a little explanation to the README about attributes and properties and their difference (if there is any)? Adding it to the WebDriver interpretation section would make sense to me. @Roemer Do you have suggestions of attribute vs. property interpretation, is there a difference or are they the same for Windows UI automation elements?

Is there a documentation from Microsoft on which attributes are supported on Windows UI automation elements? If so, it would be good to refer to it in the README.

`Selenium.WebDriver`'s `GetAttribute` tries to run a script on the server. `GetDomAttribute` works as we need and allows the test to pass.
@grokys
Copy link
Contributor Author

grokys commented May 1, 2024

I did a quick test, GetAttribute indeed converts to a script. But you can use GetDomAttribute instead which maps to the attribute API.

Ah! Yes that works! Ok then I think #28 is unnecessary for the moment then. I've pushed a change to the test to use GetDomAttribute.

@grokys
Copy link
Contributor Author

grokys commented May 1, 2024

Added support for pattern properties. I think this PR should be ready for review now.

Can we add a little explanation to the README about attributes and properties and their difference (if there is any)? Adding it to the WebDriver interpretation section would make sense to me

Sure, though I'm not sure I'm the right person to write that ;) From what I understand in WinAppDriver, attributes simply map to UIA element properties and pattern properties if they're in the form Pattern.Property. If it really is that simple I'm happy to update the readme.

Is there a documentation from Microsoft on which attributes are supported on Windows UI automation elements? If so, it would be good to refer to it in the README.

I'm not sure there is... I can't remember where I found out about it but I assume it was StackOverflow.

@aristotelos
Copy link
Collaborator

@Roemer Is this the right link to reference for UIA3 automation element properties? https://learn.microsoft.com/en-us/windows/win32/winauto/uiauto-automation-element-propids

@grokys
Copy link
Contributor Author

grokys commented May 2, 2024

@aristotelos you didn't ask me, but I believe that is the correct link for the UAI3 properties, yes.

@grokys grokys marked this pull request as ready for review May 2, 2024 07:36
@aristotelos
Copy link
Collaborator

@grokys Created grokys#1 for updating the documentation a little.

Could you also look into the build warnings generated about using nullable reference types?

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.

Fix the build check warnings: "The annotation for nullable reference types should only be used in code within a '#nullable' annotations context."

@grokys
Copy link
Contributor Author

grokys commented May 2, 2024

Fix the build check warnings: "The annotation for nullable reference types should only be used in code within a '#nullable' annotations context."

Fixed. Any reason the tests aren't using nullable?

@aristotelos
Copy link
Collaborator

Fix the build check warnings: "The annotation for nullable reference types should only be used in code within a '#nullable' annotations context."

Fixed. Any reason the tests aren't using nullable?

No, no good reason, so a PR to change that is welcome.

@aristotelos
Copy link
Collaborator

@grokys Could you still merge grokys#1 into your branch so that the documentation is more complete?

…-doc

Document interpretation of attribute in README
@grokys
Copy link
Contributor Author

grokys commented May 3, 2024

@aristotelos grokys#1 merged, thanks for doing that!

EDIT: Also added some docs about how attributes map to UIA properties.

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.

Approved with one minor test naming suggestion

Co-authored-by: aristotelos <arisvd@gmail.com>
@grokys
Copy link
Contributor Author

grokys commented May 3, 2024

Thanks, applied!

@aristotelos aristotelos merged commit 8ca04cc into FlaUI:main May 3, 2024
3 checks passed
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.

feat: Add Get Element Attribute implementation
2 participants