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

grid vs. treegrid allowed row attributes #2794

Closed
smhigley opened this issue Feb 9, 2021 · 12 comments
Closed

grid vs. treegrid allowed row attributes #2794

smhigley opened this issue Feb 9, 2021 · 12 comments
Labels
feat New feature or enhancement pr A pr has been created for the issue rules Issue or false result from an axe-core rule

Comments

@smhigley
Copy link
Contributor

smhigley commented Feb 9, 2021

Expectation: aria-posinset, aria-setsize, aria-expanded, and aria-level should all be allowed on role="row", but only when descending from a treegrid.

Actual: aria-posinset and aria-setsize always cause an allowed attribute error on row, while aria-expanded and aria-level never do.

You can see the false positives on the aria-practices example on this page: https://w3c.github.io/aria-practices/examples/treegrid/treegrid-1.html. If you change the parent role to grid then run the test again, you can see that aria-expanded and aria-level are not flagged.

Motivation:
Valid treegrid attributes are getting errors flagged, resulting in a stream of bad bugs, and some easily-caught errors like aria-expanded or aria-level on grids and tables are not flagged.


axe-core version: 4.1.1
@straker
Copy link
Contributor

straker commented Feb 9, 2021

I'm not going to lie, I'm having a lot of difficulty navigating that treegrid. I'm trying to test screen reader support but any type of navigation I'm trying isn't consistent and makes it hard to use. For example, in IE11/JAWS I tab to the start of the tree grid, which reads the first row, tells me it's expanded, and that it's 1, 1 of 1 (which I assume are the aira-postinset and aria-setsize being read out). However, when I press down arrow it goes to the next row (expected), but neither level, size, nor number in list is read out. Pressing down to move to the next row does not do that and instead it moves screen reader focus to the next column, and then I'm broke.

I'm not very well versed in navigating treegrids with expansions so maybe I'm doing something wrong?

@smhigley
Copy link
Contributor Author

smhigley commented Feb 9, 2021

JAWS and other screen readers will read level, expand/collapse, row, and column information conditionally based on the type of interaction you're doing. I'd also recommend using Chrome over IE11 for treegrids :).

Does the specific screen reader UX matter for this issue? The posinset and setsize attributes are necessary on a treegrid that doesn't render all its rows, and I can't think of a reason not to warn about the use of those attributes + expanded/level on a grid or table row.

@straker
Copy link
Contributor

straker commented Feb 9, 2021

Just looking at what each screen reader does with the attributes is all.

@smhigley
Copy link
Contributor Author

@straker I just wanted to follow up -- is there any other info you'd need for this issue? Do you have a sense of when it might be fixed, or if an external PR would be accepted if one were made? I'm very aware that treegrid support in general is all over the place, but I can't think of a reason to allow a treegrid role but flag specific valid attributes on them.

@straker
Copy link
Contributor

straker commented Feb 18, 2021

Sorry we've been really busy with something so haven't even had a chance to look further into this. With axe-con coming up and everything on our plate, I'm not sure we could work on this any time soon.

However, I can't see any adverse problems with supporting the attributes (though @WilcoFiers might have a different opinion) so if you're up for it you could create a PR to add them and that would probably get it in sooner. One of the major problems this issue faces is our code does not handle allowed attributes on roles only when those roles are descendants of a specific role.

@WilcoFiers
Copy link
Contributor

@smhigley You bring such interesting questions! Really appreciate these. Thank you!

aria-expanded and aria-level are allowed on row by axe-core. I'm less convinced that they should be. But that's not what you asked for. Adding setsize and posinset makes sense to me. I think they are of benefit to AT that support them, and i don't think there's a problem when AT doesn't support them.

I like the idea of restricting those attributes to treegrid as well. If someone uses aria-level on grid > row, something's gone wrong there.

Having seen this I think we should seriously consider flagging aria-level and aria-expanded as unsupported for rows. I can't get either of these announced at all in VO. It'll announce expanded/collapsed on the cell (example 3), but not on the row - or at least not until after you've tried to activate it, which is no help at all.

Not knowing if a row is expanded or not, or even if it can be expanded at all is a serious problem. Similarly, not knowing what level a row is at seems like a major issue. Treegrids are confusing even if they work as intended, let alone if you're missing half the info. I'd like to see some more tests done on this, if VO is an outlier here, we may mark it for review instead. I would be fine with that, but I don't think we should continue to pass ara-expaned / aria-level on row the way we're doing now.

@WilcoFiers WilcoFiers added feat New feature or enhancement rules Issue or false result from an axe-core rule labels Feb 24, 2021
@jessiehuff
Copy link

I completely agree with @smhigley here. We're trying to build out this structure in Red Hat's design system, PatternFly, and we've been running into this issue. We currently have axe-core running against our CI/CD build so the PR we had that follows the previous example shared, was being flagged as failing. (This is a preview of our implementation.) We set it to ignore that specific example since it seems to be the best structure we tested, but we worry about the other products who might use this variation. We've been encouraging Red Hat products to also check their build against axe-core so we're concerned that they might see these failures and be confused. I'd love to see these attributes supported!

@straker
Copy link
Contributor

straker commented Jun 2, 2021

We recently merged a PR that allows aria-posinset and aria-setsize to be allowed on row elements to remove the false positive. We'll work on another PR which will scope the attributes to just treegrid and warn the user if they use it on grid or table.

@straker
Copy link
Contributor

straker commented Oct 7, 2021

We recently merged a PR that should now restrict aria-posinset, aria-setsize, aria-expanded, and aria-level attributes for only treegrid elements . That should be the last thing for this issue to be resolved.

@straker straker added the pr A pr has been created for the issue label Oct 7, 2021
@padmavemulapati
Copy link

Validated with latest axe-core develop-branch code base,
Fix is working as,
aria-posinset, aria-setsize, aria-expanded, and aria-level - allowing on role="row", but only when descending from a treegrid. (can see in the first screenshot)
Earlier these are failing (can see in the below second screenshot)
image

image

@havishat
Copy link

Hi, I am facing same issue. What version do we need to use in order to fix the issue.

@straker
Copy link
Contributor

straker commented Oct 24, 2022

Latest version should have the fix, but I believe 4.3.4 was the release that introduced this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or enhancement pr A pr has been created for the issue rules Issue or false result from an axe-core rule
Projects
None yet
Development

No branches or pull requests

7 participants