Use treegrid role for schema table#310
Conversation
wparad
left a comment
wparad
left a comment
There was a problem hiding this comment.
Because this was a smaller PR, I've taken the time to review each of the individual changes and are asking for clarification on each. Hopefully we can get merged that which is of the highest priority.
One recommendation I have would be to split out everything other than the most critical changes from the PR so we can get those in, without blocking the based on lesser important changes.
| animation-duration: 0s !important; | ||
| } | ||
| .tr:not(.collapsed) + .object-body { | ||
| visibility: visible; |
There was a problem hiding this comment.
The PR says use treegrid role, it doesn't say why or why the visibility needs to be changed to do that
There was a problem hiding this comment.
The visibility change is about the expanding and collapsing sections, and their continued visibility to screen readers and the focus order. If a section is collapsed without either visibility: hidden or display: none, the section is still (confusingly) read by screen readers, and keyboard focus will become trapped in those sections (focus disappears visually from the screen, as the focussable content is accessible by keyboard, even though the content is not on screen). Because these sections use animations/transitions and display:none cannot be animated (and disrupts animations), visibility was used instead.
There was a problem hiding this comment.
Got it. If you split that out into its own PR, we can get that merged in while we address the other comments.
And fwiw, this response you provided is what is required in every PR description for the changes that are being made otherwise reviewers have no idea why the change that is being made is the right one.
There was a problem hiding this comment.
The PR says use treegrid role, it doesn't say why
I thought I explained why the treegrid role should be applied in the PR description, if it's not clear please let me know what needs further explanation.
There was a problem hiding this comment.
Got it. If you split that out into its own PR, we can get that merged in while we address the other comments.
Without the other changes, the buttons to expand and collapse these sections are not keyboard accessible at all (another of the original problems pointed out by our accessibility auditors). I'm sorry for the large PR, but it is all interconnected.
There was a problem hiding this comment.
Can always be split, there looks like 4 different PRs here:
- update visibility to handle cases when the object-body isn't actually shown
- improving aesthetic viewing of description fields when they are empty
- Adding treegrid role/attributes to different places
- Whatever is happening with the javascript that I just gave up reviewing at that point.
I don't care the order that we go through these, but my recommendation is to start with the one that you believe is most important first. Because if we don't do that, it might mean nothing else is of value. Is that the javascript one?
Regarding the PR description, I want to help you improve future PR descriptions, for context here is what was said:
Accessibility auditors discovered that in addition to containing clickable elements that cannot be navigated to via the keyboard, the schema table is a table in data, but uses non-table elements to display the tabular data. This means the tabular data is not accessible or understandable for screen readers.
Because the number of rows in the table can be changed after loading through the collapsible sections, the element cannot use a native HTML table or table roles, which are for static tables only, and must use ARIA treegrid (See the warning on the table role description).
This PR adds all the needed treegrid roles, required secondary aria attributes like labels and accurate aria-controls and aria-expanded for the expandable sections, and the recommended keyboard navigation for treegrid cells.
Accessibility auditors discovered that in addition to containing clickable elements that cannot be navigated to via the keyboard, the schema table is a table in data, but uses non-table elements to display the tabular data. This means the tabular data is not accessible or understandable for screen readers.
That doesn't mean anything to me. I don't care what auditors discovered, it doesn't matter that table uses non-table elements. That's not important. This segment could say something like:
Currently, the elements in the table are not accessible, when a screen reader navigates through the table,
insert exact problem, instead ofwhat should happen. This PR fixes this byinsert description of why the fix in the PR fixes the problem and why it is the right solution.
Because the number of rows in the table can be changed after loading through the collapsible sections, the element cannot use a native HTML table or table roles, which are for static tables only, and must use [ARIA treegrid](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/treegrid_role) (See the warning on [the table role description](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/table_role#description)).
This is equally useless, we don't need a review of why solution XYZ is irrelevant and doesn't apply, that offers no value what so ever, focus on what the solution is. An equally useless text that could have been here might as well have said, using ReactNative here wouldn't work because we aren't building a mobile application. Then the message goes on to say "Use this ARIA treegrid" but nowhere is explained why.
This section could have followed from the above one with The solution to make it accessible is using treegrid which solves this problem, applying treegrid causes _these UX changes_ and causes screen readers to instead _fill in the blank_.
and the last statement:
This PR adds all the needed treegrid roles, required secondary aria attributes like labels and accurate aria-controls and aria-expanded for the expandable sections, and the recommended keyboard navigation for treegrid cells.
This just repeats what is in code, and offers no value, it is tantamount to a comment in code that looks like this:
// adds 1 plus 1 to get 2 and stores this in a variable called sum
const sum = 1 + 1;Completely useless.
Does that help at all?
There was a problem hiding this comment.
I think this does give me a stronger idea of what you are looking for as far as explanations, and some of this (more generalized) could be added to the contributions guide, which is basically empty. For example, I'm not clear on what makes something a large "number of changes", like if I need to make a similar change to multiple files, can I make one PR, or do I need one for each file, or what?
I am just a developer myself. I am not an accessibility auditor or a screen reader user, it is difficult for me to say exactly what happens, to "insert the exact problem", in all of these use cases for inaccessible code. While keyboard navigation is easy to test, I have no idea what the bad experience vs the good experience is for screen reader users in all these cases—like any average developer, I have to rely on what MDN/ARIA docs say you need to do ("you have to do something for tabular data, but if you can modify the table, don't use table, use treegrid here we say in big red text!")—which is why I linked to the big red text.
What I do have is a huge list of accessibility problems stemming from this project that were given to me to fix. For this specific PR, the relevant problems listed for me are:
Data tables lack table markup that conveys relationships between cells and headers. Applies to all pages with column headers such as Field, Type, Description. Applies to multiple instances.
"Show/hide uses improper role and does not announce states. Applies to arrowheads on nav categories and inside tables under ""Model"" tabs.
Also applies to characters in ""Example"" code sections such as brackets and braces, which announce only ""clickable""."
I spent many days figuring out what all the feedback meant as far as implementation requirements. I tried to explain the requirement and the solution as succinctly as I could, but it is impossible for me to do so without pointing to outside expertise, or to fully explain the problem behind the requirement without pointing to the same.
I was legally required to make changes to this project code, to get my own project's website to pass an accessibility audit (accessibility requirements are in fact strong legal requirements, not meeting accessibility needs opens organizations up to lawsuits). I wanted to provide what I was able to back up the chain, as a courtesy to the open source project. But to me, this is very secondary to saving my own project from legal exposure, which is now already done. I can always update and publish my own fork as the "accessible version" of this project, if these changes are not considered worthwhile to the project core.
| </style> | ||
| <div class="table ${this.interactive ? 'interactive' : ''}"> | ||
| <div class="table ${this.interactive ? 'interactive' : ''}" role="treegrid" aria-label="JSON model" tabindex="0" @keydown="${(e) => this.treegridKeydown(e)}" @focus="${(e) => { | ||
| e.target.removeAttribute('tabindex'); |
There was a problem hiding this comment.
What does this have to do with tree grid role
There was a problem hiding this comment.
Sorry, this is a bit weird! First, this sets the role and adds a required accessible label.
There are a few different ways of implementing keyboard nav with treegrid (see https://www.w3.org/WAI/ARIA/apg/patterns/treegrid/examples/treegrid-1/ for some examples). Due to the treegrid only having the expandable sections and not other complex functionality, I wanted to go with the simplest, where only individual cells are focusable and with a roving tabindex. This would require setting tabindex to 0 on the very first gridcell in the treegrid, and -1 on all other gridcells. However, with the recursive way this component is engineered, I couldn't see a simple way to hard code the tabindex to 0 on only the very first gridcell. This is the simplest solution I saw considering that setup: to set the tabindex on the grid itself, but immediately switch it to the first gridcell and remove it from the treegrid once it is accessed.
There was a problem hiding this comment.
Honestly it feels like we should start at the beginning with a design. Can you share what your over technical strategy is here, and why it is that one? I don't like that I'm reviewing code that doesn't ever explain what we are doing and why.
I would appreciate to read some words from you that start with something like:
To make the schema table accessible means that — list of requirements — the technical implementation to achieve that will be....
I could imagine in the requirements are things like all divs must have an aria role, that's easily understood, and more importantly reviewable.
| e.target.setAttribute('aria-expanded', !rowEl.classList.contains('collapsed')); | ||
| } | ||
|
|
||
| treegridKeydown(e) { |
There was a problem hiding this comment.
What does any of this have to do with tree grid role
There was a problem hiding this comment.
A row that can be expanded or collapsed to show or hide a set of child rows is a parent row. Each parent row has the aria-expanded state set on either the row element or on a cell contained in the row.
From https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/treegrid_role
There was a problem hiding this comment.
I think you are saying that you want to support keyboard navigation for the schema table, and that the treegrid defines expected accessible controls for such a dynamic table.
Is that what you were saying?
If so, I think we need to comment up this method with all the oddities because there are so many things here that make no sense to me:
- Why are we explicitly excluding handling for the shift? doesn't that violate the tree grid accessibility?
- Is "gridcell" the correct role to be using here to justify not including it in the handling, maybe, it isn't immediately obvious for me.
- What is
rIndexsupposed to represent? index is almost always a bad name for things, I think we meant to call it columnNumber and rowNumber, is that what we meant? - Why are we setting the tabIndex to -1 / 0, that feels very hacky what problem is it solving
- Why do we prevent default for some of these and not others
- Why isn't every treegrid keyboard interaction supported
There was a problem hiding this comment.
I think you are saying that you want to support keyboard navigation for the schema table, and that the treegrid defines expected accessible controls for such a dynamic table.
This and the markup of the table itself is also important. I am not sure of what the experience is like for a screen reader user, but I believe there's important navigation options available with accurately tagged tables (being able to get the information of what row and column each cell belongs to). Which a treegrid also performs.
Why are we explicitly excluding handling for the shift? doesn't that violate the tree grid accessibility?
In my testing all the required keyboard options with shift + arrows were the existing default browser functions (selecting the text), so by excluding operations with the shift key I allowed for those automatically, rather than having them trapped by the arrow functions.
Is "gridcell" the correct role to be using here to justify not including it in the handling, maybe, it isn't immediately obvious for me.
Gridcells are in fact used in the handling
What is rIndex supposed to represent? index is almost always a bad name for things, I think we meant to call it columnNumber and rowNumber, is that what we meant?
You are 100% correct
Why are we setting the tabIndex to -1 / 0, that feels very hacky what problem is it solving
This is called roving tabindex.
From the treegrid role docs on MDN:
If the treegrid container itself receives focus, the value of its aria-activedescendant property should reference the id of the selected row, columnheader, rowheader or gridcell, unless roving tabindex is used to manage focus between those roles, in which case aria-activedescendant should not be used.
Using activedescendant instead of roving tabindex would require setting an id on each individual cell, and updating the activedescendant with each one as it was selected (a huge headache)
Why do we prevent default for some of these and not others
Home and End have a default action that must be prevented, the arrow keys on their own do not, and I don't want to prevent the default for keys in general (like tabs, space bar)
Why isn't every treegrid keyboard interaction supported
By only having the cells focussable, and not the rows as well, no row keyboard interactions need to be supported. It keeps things much, much simpler.
There was a problem hiding this comment.
It sounds like I may be undercommenting the code itself. I just based this on what I saw in there already, but I can definitely go through and add more comments.
| font-family:var(--font-regular); | ||
| flex-shrink: 1; | ||
| flex-grow: 1; | ||
| text-overflow: ellipsis; |
There was a problem hiding this comment.
In the treegrid each gridcell is focussable with the keyboard. Without flex-grow as well as flex shrink, if the last column of data has no content, it ends up also having zero width, which looks very strange when it's focused.
Desired effect:
Without flex-grow: 1:
Co-authored-by: Warren Parad <5056218+wparad@users.noreply.github.com> Signed-off-by: Audrey Hamelers <1749394+ahamelers@users.noreply.github.com>
|
It sounds like I should close this PR and make some with more comments, both in the code and explaining in the PR itself. With the sheer number of changes ultimately needed, I am a bit worried about merge conflicts, but I think we can make do, and I am willing to do this. I'm sorry that I will need to continue to be like, "accessibility experts say...", because I don't use a screen reader myself! |
|
👍 |
2 participants
Accessibility auditors discovered that in addition to containing clickable elements that cannot be navigated to via the keyboard, the schema table is a table in data, but uses non-table elements to display the tabular data. This means the tabular data is not accessible or understandable for screen readers.
Because the number of rows in the table can be changed after loading through the collapsible sections, the element cannot use a native HTML table or table roles, which are for static tables only, and must use ARIA treegrid (See the warning on the table role description).
This PR adds all the needed treegrid roles, required secondary aria attributes like labels and accurate aria-controls and aria-expanded for the expandable sections, and the recommended keyboard navigation for treegrid cells.
For #305