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

role/label tests needed for display: contents #60

Closed
jensimmons opened this issue Sep 20, 2023 · 10 comments · Fixed by web-platform-tests/wpt#43740
Closed

role/label tests needed for display: contents #60

jensimmons opened this issue Sep 20, 2023 · 10 comments · Fixed by web-platform-tests/wpt#43740
Assignees
Labels

Comments

@jensimmons
Copy link

jensimmons commented Sep 20, 2023

When display: contents was first implemented by browsers, it was not designed to fully take into consideration how browser engines could implement this in an accessible way. Because it required significant refactor of the entire accessibility system in browser engines, it's taken time to fix... and meant the initial implementations were not at all accessible.

Web developers have been told "don't use this!" Although, of course, not everyone gets the memo, and developers are using it. Especially since subgrid has taken so very long to ship.

Browsers have, over time, greatly improved their implementation of display: contents. It is now behaving properly most of the time / for core use cases. But there are still ways in which it's not yet fixed, and that can create an accessibility minefield.

If there were WPT tests for display: contents, it could be included in Interop 2024. And we could encourage all three engines to finish improving their implementation, squash all the remaining accessibility bugs, and ensure elements with display: contents applied (and all their children) don't magically vanish from screenreaders.

@zcorpan
Copy link
Member

zcorpan commented Oct 24, 2023

Can this be sufficiently tested with the "get computed role" command?

@cookiecrook
Copy link
Collaborator

I think a combination of computedrole and computedlabel can get us most of the way there, probably over 90%.

@cookiecrook
Copy link
Collaborator

Some examples of that for others who may want to start writing the tests...

  • display:contents in tables shouldn't make cells generic, should retain gridcell or cell roles.
  • ditto for a number of other roles; we should cross-reference the myriad of accessibility fixes and edge cases that went in to the engines for display:contents
  • AccName and other host language computedlabel tests when display:contents is on the label or labeled element, or some referenced sub-label via aria-labelledby.

@alice
Copy link

alice commented Oct 25, 2023

I think a combination of computedrole and computedlabel can get us most of the way there, probably over 90%.

Unfortunately this very much depends on how these are computed (I apologise that I don't know enough about how they're specified to know whether I'm barking up the wrong tree here.)

There was a regression in Blink (see comment 2) which was not detected due to precisely the issue of not explicitly checking whether a node was ignored, and only checking its computed role - the role was being computed and exposed to the relevant testing API even though the node was (incorrectly) ignored.

@cookiecrook
Copy link
Collaborator

@alice I'm not sure if you are saying this can't be tested at all with the existing webdriver methods, or just that, in order to make the tests 100% reliable, we need some additional interface (WICG/aom#203 or otherwise). If the latter, I think we can continue with this issue tracker, while acknowledging it may not get us all the way there.

@alice
Copy link

alice commented Nov 7, 2023

I guess I'm not sure of the value of testing with the existing methods if we're not completely certain they're testing (what I think is) the main thing that needs to be tested, i.e. that using display: contents doesn't incorrectly remove elements from the accessibility tree.

@chrishtr
Copy link

chrishtr commented Jan 9, 2024

Agenda+ to ask if PR 43740 is all for behaviors that are in a consensus spec.

@cookiecrook
Copy link
Collaborator

Agenda+ to ask if PR 43740 is all for behaviors that are in a consensus spec.

Yes, other than some that are noted for removal or change in the review.

@cookiecrook
Copy link
Collaborator

To reiterate, @alice is correct that current test API does not allow us to test everything we would like to test (like isIgnored or equivalent) but the included tests are sound and get us further towards Interop than what is tested today.

@cookiecrook cookiecrook changed the title display: contents tests needed role/label tests needed for display: contents Jan 16, 2024
@cookiecrook
Copy link
Collaborator

I changed the title, so that it's more precise about what is currently testable. Once we have more API available to test (like isIgnored), we should revisit display: contents and do more thorough testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants