Address Table accessibility issues #1151
Comments
|
@leslieab, we will likely need to revisit some of the decisions surrounding focus behaviors for the expand/collapse button (see the description above). |
|
@atmgrifter00 I'm unclear from what you have linked whether those Firefox errors will resolve once we have implemented keyboard interactivity for expand/collapse operations and the parent row has aria-expanded being set? We have elements in the header row that are also not focusable, does FF throw errors for those as well? |
|
@leslieab, it would only raise warnings for interactive elements like buttons. At the moment, the header has no such elements. |
|
@leslieab I'm working on interactive sorting right now. Once we have click handlers on the header elements (to allow for interactive sorting), Firefox will have the same accessibility warning for those ("Clickable elements must be focusable and should have interactive semantics."), unless we make them focusable. |
|
The AG Grid and MUI X grids that you linked do have the same accessibility warnings on their column headers. The Telerik Blazor Grid doesn't, not sure why, maybe the click handlers are being handled at a higher level than the column headers. It's worth noting that the grids still work fine for end users in Firefox. You only see the accessibility warnings we're talking about if you open the Inspector / Developer Tools, go to the Accessibility tab, and click Check for Issues > All Issues (or Keyboard): |
|
Thanks, I opened the inspector but wasn't sure where to check for those. Is the goal not to have any accessibility errors (and therefore we need to fix this)? Or should we discuss/test this with keyboard navigation/screen reader users to determine whether it's an actual usability issue? |
|
I don't think we have a hard-and-fast rule about having no accessibility warnings/errors ( @jattasNI @rajsite might have a better answer here), but we try to address them if we can. For this one, I assume the concern is that clickable-but-unfocusable elements wouldn't show up to a screen reader as being interactable. |
|
Agreed that we try to avoid accessibility warnings but we can live with them if we can convince ourselves that what we're doing is actually accessible. |
|
I can't find anything online that suggests there's ever a use case for ignoring this error. The fact that the grid examples I've been using do so is probably an example of what Jim Allan was talking about when he said "people don’t have a lot of expectations [of grids] other than they’re usually coded poorly and they don’t work." I am going to need to go back and re-examine my wireframes for areas where we are doing this and come up with recommended approaches to add focus back in, which I will not be able to do until next week at the earliest. Is this currently blocking anyone, or can it wait until the week of 17th? |
|
I think it can wait / is not currently blocking anything. (I'll also be OOO next week and the following week) |
# Pull Request ## 🤨 Rationale Adds multi-select support in the table as part of #856 Functionality included in this change: - Add selection checkboxes to rows, group rows, and table header when multi-select is enabled Functionality explicitly excluded from this change: - Angular/Blazor support - Add labels to selection checkboxes. I've added a note in the table accessibility issue that we need to address this in a similar manner to addressing it for the expand/collapse button on group rows: #1151 - Add support for multi-selection through CTRL+CLICK and SHIFT+CLICK ## 👩💻 Implementation - Extend `TableRowSelectionMode` to include `multiple` - Extend `TableRowSelectionState` to include `single` - Add checkboxes to rows, group rows, and table when the selection mode was `multiple`. - I had to wrap the checkboxes in a `gridcell` because a checkbox isn't an allowable direct child of a `row`. A button also isn't an allowable direct child of a `row`, so I made the same change to wrap the existing expand/collapse button on group rows in a `gridcell` - I ran into issues because the `change` event is fired from the `nimble-checkbox` when a programmatic change to the value is made. Therefore, I am programmatically controlling the state of the selection checkboxes rather than binding to the `checked` and `indeterminate` properties from templates. - Some of the TanStack selection code does not work the way we need it to, particularly around selection + grouping. Therefore, I've written explicit code to: - deselect all children of a group when the group row's selection checkbox becomes unchecked - determine the selection state of group rows ## 🧪 Testing - Added a number of new unit tests - Manually tested in storybook - Extended table matrix tests to include all three selection modes - Added selection modes to the column stories to enable testing selection with other features such as column sizing and grouping ## ✅ Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
# Pull Request ## 🤨 Rationale Add Angular support for row selection in the table, which is part of #856. ## 👩💻 Implementation - Add `selectionMode` to the table's directive - Add `getSelectedRowIds()` and `setSelectedRowIds()` to the table's directive - Update the Angular example app to configure a selection mode of `multiple` - Update the Angular example app to render with 0 rows in the table because there are Lighthouse failures otherwise - This is not an ideal solution (nor a long-term solution). However, we decided it did not make sense to block adding Angular support for row selection on Lighthouse failures, particularly when we already have an issue to track accessibility issues with the table -- #1151 ## 🧪 Testing - Added/ran unit tests - Verified table worked correctly in example app ## ✅ Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
|
@leslieab I wanted to circle back and see if you had any updates regarding focusability for the interactive parts of the table (clicking column headers for sorting, etc). I should have interactive sorting implemented at the beginning of next week, so I'm just wondering what the impact would be to that (or interactive grouping / column header menus, which we may start implementing soon). Thanks. |
jattasNI
modified the milestones:
Table Milestone 5,
Table Milestone 6,
Table Milestone n
# Pull Request ## 🤨 Rationale Add localized labels and titles to remaining elements within the nimble-table. This is part of #1151 ## 👩💻 Implementation Add new label tokens for required strings and use them within the table. ## 🧪 Testing - Manually tested that the labels show up correctly in the accessibility tree* - Manually tested that titles are appropriately shown on elements in storybook - Created a branch that re-introduced initial rows in the Angular example app table and verified that lighthouse accessibility check now passes. *As part of this manual check, I found that the labels on our group header icons don't work as expected due to #1510 ## ✅ Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed.
|
Closing this issue by addressing the remaining noted items from the description in the following ways:
|
📌 User Story
The table needs the correct ARIA role.
Once the table row grouping PR goes through, there will be some accessibility issues that will require some follow up work on (some of which will possibly be part of the Table Interaction Keyboard work). Some of the issues raised include:
TableGroupRowis (ARIA guidance strongly suggests that therowgrouprole is only appropriate for DOM elements that actually containrow(or other) elements, which ourTableGroupRowdoes not).Assuming the expand/collapse button remains a button, we will need to create an API for users to supply it an accessible label, much like we did for the action menu label (except likely exposed on the[Addressed in Add localized labels to remaining table sub-elements #1519]Table).aria-expandedon the group rows, but this requires the table itself to have a role oftreegrid. Some questions about this:treegridor should the role change based on whether or not a column is grouped and whether or not hierarchy is enabled?aria-expandedmake sense on group rows if the rows beneath it are not child elements in the DOM?Add accessible label to the selection checkboxes in the multi-select case. This should probably be done in a consistent manner as adding a label to the expand/collapse button for group rows.[Addressed in Add localized labels to remaining table sub-elements #1519]Note: The localization concerns described above are also captured in #1090
Note: When this is fixed, we can also address #1195.
The text was updated successfully, but these errors were encountered: