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

Use better assertions for computedrole #39618

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook cookiecrook commented Apr 20, 2023

Resolves #39118

Replaces two invalid computedrole assertions which relied on an orphaned menuitem and a previously unspecified but now incorrect role synonym.

All three of the updated computedrole tests pass in WebKit and Chromium ( and Gecko soon), and this test effectively exercises the webdriver computedrole getter. Other disputed or erroneous tests that were removed here will be handled in /html-aam and /wai-aria/role

More detail in #39118

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Apr 20, 2023

@jcsteh looks like Gecko is failing the new "main" role test. Would you like to suggest another innocuous one for this webdriver test, since that specific role failure is already tested in /html-aam/role/roles.html? Perhaps <div role=main> or <nav>?

@jcsteh
Copy link
Contributor

jcsteh commented Apr 20, 2023

ComputedRole is currently pretty broken in Gecko, as it exposes our internal role strings rather than standardised ARIA roles. I've been working on this and I have a bunch of patches up for review to fix it. Once those land, this test should pass.

That said, <article>foo</article> should pass even with the current brokenness.

@cookiecrook
Copy link
Contributor Author

Gecko fails for <nav> too, but I have not been able to get WPT running in Firefox so I'm not going to play trial and error with the Azure Pipeline. I'll add an <h2> heading as the last attempt, but if that doesn't pass, I'll leave as-is. These are all valid failures in Gecko.

@cookiecrook
Copy link
Contributor Author

Comments crossed. I will try <article> thanks.

@cookiecrook
Copy link
Contributor Author

Confirmed Gecko passes the <article> test. Thanks Jamie.

Okay to merge once @patrickangle or @AutomatedTester reviews.

@jgraham
Copy link
Contributor

jgraham commented Apr 20, 2023

This doesn't require review from all the people marked as reviewers. In particular if the people working on a11y are happy with this change, and it doesn't also require a WebDriver spec change, we should land it without review from a WebDriver reviewer (if it does require a spec change we could still find a way to land it as a tentative test without WebDriver review, as long as there's a spec issue open).

Copy link
Contributor

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

Given the conversation, I think it's fine to approve this, as long as @jcsteh is happy with the change as well.

@cookiecrook cookiecrook merged commit e1beec6 into web-platform-tests:master Apr 21, 2023
@cookiecrook cookiecrook deleted the better-assertions-for-computedrole branch April 21, 2023 00:19
cookiecrook added a commit to cookiecrook/wpt that referenced this pull request Apr 21, 2023
Resolves web-platform-tests#39118

Changes the WebDriver tests for computedrole to avoid some invalid tests. Note: The removed validation test are either already tests in /html-aam or /wai-aria, or are on the list to be written soon, so this should keep the WebDriver tests reliable without churn from unrelated accessibility changes.

Reviewed by juliandescottes and jcsteh.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some role assertions in webdriver/tests/get_computed_role/get.py seem incorrect
6 participants