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

Add basic accname tests for shadow DOM #36541

Merged

Conversation

nolanlawson
Copy link
Member

@nolanlawson nolanlawson commented Oct 18, 2022

Adds basic tests for accname in shadow DOM. These are manual tests, modeled after the existing tests automated tests.

Cases covered:

I don't have access to all the screenreaders listed, so I verified using the Firefox DevTools Accessibility pane (Firefox 105). Tested locally using ./wpt run.

@nolanlawson
Copy link
Member Author

Relevant PR on accname: w3c/accname#167

Note that I'm not solving the reflected element property ariaLabelledByElements for the moment. Nor am I handling aria-describedby or ariaDescribedByElements. (See w3c/accname#170 for context.)

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

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

Hi Nolan, thanks so much for writing these!! I know I also suggested you just write these "manual" tests, but I also just found out we do have infrastructure for writing automated tests of accname specifically, using webdriver and "get_computed_label", within WPT. There are some links from this issue: w3c/accname#174

Eventually we will translate all of these tests into using webdriver, so if you would prefer to land these tests as manual tests, these tests will suffice for now.

However, if you would like to write the first set of automated tests for accname+shadow DOM, you could look into how with the links above, and your tests will show up in wpt.fyi! :)

accname/name_test_case_shadowdom_1-manual.html Outdated Show resolved Hide resolved
accname/name_test_case_shadowdom_3-manual.html Outdated Show resolved Hide resolved
accname/name_test_case_shadowdom_4-manual.html Outdated Show resolved Hide resolved
accname/name_test_case_shadowdom_5-manual.html Outdated Show resolved Hide resolved
accname/name_test_case_shadowdom_6-manual.html Outdated Show resolved Hide resolved
@nolanlawson
Copy link
Member Author

nolanlawson commented Nov 8, 2022

Eventually we will translate all of these tests into using webdriver, so if you would prefer to land these tests as manual tests, these tests will suffice for now.

@spectranaut Thanks for the context! For the automated tests, it seems like it should be possible to write a script to bulk-convert the existing manual tests into automated tests?

If that's the case, then I think for now I would prefer to write the manual tests. If not, then yes, maybe it would be worth the effort for me to start writing automated tests from scratch.

@wpt-pr-bot
Copy link
Collaborator

There are no reviewers for this pull request. Please reach out on the chat room to get help with this. Thank you!

@cookiecrook cookiecrook self-requested a review April 20, 2023 01:41
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

The manual tests are not being maintained (or run, to my knowledge), but this could be re-written as an automated test in wpt/accname/name/comp_name_from_content.html once #39604 lands. Please update the ReadMe in that same dir if you're the one making the change.

@cookiecrook
Copy link
Contributor

cookiecrook commented Apr 20, 2023

@nolanlawson wrote:

For the automated tests, it seems like it should be possible to write a script to bulk-convert the existing manual tests into automated tests?

If that's the case, then I think for now I would prefer to write the manual tests. If not, then yes, maybe it would be worth the effort for me to start writing automated tests from scratch.

Maybe there is a misplaced "not" in your comment above? "If that's [not] the case…" and "If [so] not,"?

I don't think you could bulk convert the manual tests, as those rely on platform mappings that are not testable with browsers internals. For examples of what is testable with automation so far, review HEAD in wpt/accname, wpt/html-aam, and wpt/wai-aria-role. It's computedrole and computedlabel but with a testdriver extension so you don't have to use webdriver directly.

Of note, since the manual ATTA tests are not being maintained, I moved them into manual directories respectively per spec. If you decide to continue with the manual test, please move it into the newer wpt/accname/manual dir.

@nolanlawson
Copy link
Member Author

nolanlawson commented Apr 21, 2023

@cookiecrook I guess I was trying to say: "If we can automate the migration from manual to automated tests, then can we apply that to my PR?" But if there is no automated process, then no biggie – I ported my manual tests to automated tests in 39f49b9.

I put the new tests under accname/shadowdom – let me know if that looks okay. I also managed to test locally, and the tests seem to pass in both Safari and Chrome.

@cookiecrook cookiecrook assigned nolanlawson and unassigned halindrome May 1, 2023
@wpt-pr-bot wpt-pr-bot requested a review from jnurthen May 1, 2023 23:16
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Looks good overall... Last nit to avoid name collision with future WPT.fyi results.

accname/name/shadowdom/basic.html Outdated Show resolved Hide resolved
accname/name/shadowdom/basic.html Outdated Show resolved Hide resolved
accname/name/shadowdom/slot.html Outdated Show resolved Hide resolved
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Looks great. Merge when you're ready, or I can if you prefer.

@nolanlawson
Copy link
Member Author

I'm not able to merge, so please go ahead! 🙂

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.

6 participants