[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213

chandlerprall · 2021-09-23T19:56:10Z

Summary

Fixes #4474 by providing div[role=row]s which aria-owns cells.

@1Copenut I'd like help testing impact on screen readers & other assistive tech. AXE is happy, but that doesn't always mean Ship It.

My initial thought was to let the cells create div[role=row] on mount, if that row didn't already exist, and inject it into the DOM. That feels WAY hacky, even for me. Instead, I found react-window provides the row&column start/stop indices, which allows the grid to know what rows need to be created, so I thought maybe the cells could still move themselves into that location of the DOM. However, there's an aria-owns attribute which allows the row to declare its relation to the cells, and everything can stay in React.

AXE before

AXE after

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~

Added or updated jest tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2021-09-23T20:20:30Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5213/

1Copenut · 2021-09-23T22:13:47Z

@chandlerprall I tested the PR data grid in Safari + VoiceOver, and then tested the production version. The PR you are proposing created a much better experience, and some things we should check further in NVDA or JAWS if possible:

User experience wins

I was able to traverse through the data grid cell by cell. The grid announced itself properly (number of rows and columns) and announced column headers.
The grid cells were announced very specifically. There might be an opportunity to trim the extra Row and Column information being announced—I'm guessing these are SR-only blocks, if screen readers announce the data cells as we'd expect them to announce regular table cells.

Opportunities for improvement

The sortable column headers announce themselves after the data grid in VO. When I traversed vertically through a column, it announced the data cells, then the sortable column header, then the next column. I think this is because the sortable row is not owned, so it's not recognized in the data grid source order.
The grid controls (Columns, Density, Sort Fields, Full Screen) were not announced as part of the grid and didn't feel like they had a relationship to it (the grid). I think we could add a heading or informational text and explore using aria-describedby or aria-labelledby to relate the two items, but that's outside the scope of this PR.

1Copenut

👍 LGTM!

I'm picking a lot of this review comment from my previous, longer one. All screen reader testing so far was done on VoiceOver + Safari, MacOS Big Sur.

The grid announced itself properly (number of rows and columns) and announced column headers.
Grid cells were announced very specifically.
There might be an opportunity to trim the extra Row and Column information being announced—I'm guessing these are SR-only blocks, if screen readers announce the data cells as we'd expect them to announce regular table cells.
This is a complex enough component I'd like to test (or have it tested) with NVDA and JAWS for compatibility with the aria-owns attribute. There's not recent data for screen reader tests, but I'm confident the change is a net improvement.

kibanamachine · 2021-09-27T21:10:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5213/

cee-chen

This isn't a formal/blocking change request as I'm definitely open to being persuaded this is the best way forward (especially if, e.g. perf makes actual row wrappers impossible), but I wanted to make sure we at least discuss this with other row functionality requests in mind before moving forward

cee-chen · 2021-09-27T17:57:05Z

src/components/datagrid/data_grid_types.ts

 export interface EuiDataGridCellProps {
+  /**
+   * required when used outside of a role="row" element, so a row can own this cell
+   */
+  id?: string;


Would it be possible to create a new type that's more specific, i.e. something like type EuiDataGridRowProps = Omit<EuiDataGridCellProps, 'id'>?

cee-chen · 2021-09-27T23:39:55Z

src/components/datagrid/body/data_grid_body.tsx

@@ -686,12 +745,14 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
            mutationRef(el);
          }}
        >
+          {accessibilityRows}


I had a small "hol' up" moment here. If I'm understanding this correctly, we're still not actually rendering "rows" (as I understand them normally within tables) that wrap around a specific set of cells: we're rendering a set of totally empty divs outside of the grid that just happen to have an accessibility relationship with cells due to aria-owns?

I dunno if this is just me being overly in love with semantic markup, but I worry we're just kicking the can down the road in terms of row functionality with this approach. For example, we have several row-related PR requests around highlighting/hovering (#4401, #4483) that would be significantly easier if we actually had functional wrapping rows. It's likely also closer to what users would expect in terms of markup and would make it easier for them to access via CSS.

I know you said in your PR you thought cells either inserting rows into the DOM or moving themselves into a certain DOM felt hacky/worse, but I think it's worth re-evaluating that decision with other row functionality in mind. Thoughts? Am I way off base here? Totally open to that being a possibility, but I'd want in that case to discuss what our API and approach would be for row highlighting/styling/hover behavior, if we definitively knew we did not want wrapping DOM elements.

Also sorry for not looking closer at this before our 1:1 today where we could have discussed it in-depth, super happy to hop on a call or anything if needed to brainstorm!

Trying the method of injecting / removing rows on demand, I have a working example but it causes React to fall over when scrolling back up in a virtualized container. Trying some ideas, but not optimistic.

Got a fully-functioning version 🎉 but need to address some issues with unit tests

Amazing!! Let me know if I can help at all with unit tests, happy to do so

cee-chen · 2021-09-27T23:47:20Z

src/components/datagrid/body/data_grid_body.test.tsx

@@ -17,6 +17,7 @@ import { EuiDataGridBody, Cell, getParentCellContent } from './data_grid_body';

 describe('EuiDataGridBody', () => {
  const requiredProps = {
+    gridId: 'grid',


I'd like to see us add more to EuiDataGridBody for unit testing, something that helps devs understand what setRenderedRowIndices/onItemsRendered is doing and what accessibilityRows renders, e.g.:

describe('EuiDataGridBody', () => { // ... it('renders row elements for each visible/virtualized row of cells', () => { // I would expect an assertion for the # of row elements, e.g. const rows = component.find('[role="row"]'); expect(rows).toHaveLength(10); // + one assertion/examination of any row's `aria-owns` prop to make sure it has the right ID(s) expect(rows[0].prop('aria-owns')).toContain('datagrid-grid-cell-0,0'); }); });

cee-chen · 2021-09-27T23:51:09Z

scripts/a11y-testing.js

-    `${root}#/tabular-content/data-grid`,
-    `${root}#/tabular-content/data-grid-in-memory-settings`,
-    `${root}#/tabular-content/data-grid-schemas-and-popovers`,
-    `${root}#/tabular-content/data-grid-focus`,
-    `${root}#/tabular-content/data-grid-styling-and-control`,
-    `${root}#/tabular-content/data-grid-control-columns`,
-    `${root}#/tabular-content/data-grid-footer-row`,
-    `${root}#/tabular-content/data-grid-virtualization`,
-    `${root}#/tabular-content/data-grid-row-heights-options`,


🎉 🎉 🎉

cee-chen · 2021-09-28T00:00:42Z

src/components/datagrid/body/data_grid_body.tsx

+  const accessibilityRows: ReactElement[] = [];
+  for (let i = renderedRowsStartIndex; i <= renderedRowsStopIndex; i++) {
+    const rowId = `datagrid-${gridId}-row-${i}}`;
+    const ownedCells: string[] = [];
+    for (
+      let j = renderedColumnsStartIndex;
+      j <= renderedColumnsStopIndex;
+      j++
+    ) {
+      ownedCells.push(`datagrid-${gridId}-cell-${i},${j}`);
+    }
+
+    accessibilityRows.push(
+      <div key={rowId} id={rowId} role="row" aria-owns={ownedCells.join(' ')} />
+    );
+  }


Sorry, brainfarted and totally forgot to leave this in my original code pass. Should this be wrapped in a useMemo() with all 4 renderedRowX vars as dependencies, to reduce iterations if unrelated changes occur?

cchaos · 2021-10-07T23:16:29Z

Design request if possible, and feel free to say "shove off", but can we also get "highlight" rows (opt-in functionality) to highlight an entire row both on selection and hover?

chandlerprall · 2021-10-19T16:59:29Z

Superseded by #5285, keeping in draft mode for now to allow comparison.

Provide rows for datagrid cells to be owned by

3de3ce9

chandlerprall added accessibility data grid labels Sep 23, 2021

chandlerprall requested review from cee-chen and 1Copenut September 23, 2021 19:56

chandlerprall mentioned this pull request Sep 23, 2021

[Discover] Document explorer rollout by default elastic/kibana#85965

Closed

41 tasks

1Copenut approved these changes Sep 23, 2021

View reviewed changes

chandlerprall added 2 commits September 27, 2021 14:38

Merge branch 'master' into bug/4474-datagrid-row-accessibility

4594779

changelog

04d3ab5

cee-chen requested changes Sep 27, 2021

View reviewed changes

cee-chen reviewed Sep 28, 2021

View reviewed changes

chandlerprall marked this pull request as draft October 19, 2021 16:54

chandlerprall mentioned this pull request Oct 19, 2021

[EuiDataGrid] Provide row elements to wrap cells #5285

Merged

4 tasks

chandlerprall closed this Nov 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213

[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213

chandlerprall commented Sep 23, 2021 •

edited

Loading

kibanamachine commented Sep 23, 2021

1Copenut commented Sep 23, 2021

1Copenut left a comment

kibanamachine commented Sep 27, 2021

cee-chen left a comment

cee-chen Sep 27, 2021

cee-chen Sep 27, 2021

chandlerprall Oct 6, 2021

chandlerprall Oct 7, 2021

cee-chen Oct 8, 2021

cee-chen Sep 27, 2021

cee-chen Sep 27, 2021

cee-chen Sep 28, 2021

cchaos commented Oct 7, 2021

chandlerprall commented Oct 19, 2021

[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213

[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213

Conversation

chandlerprall commented Sep 23, 2021 • edited Loading

Summary

AXE before

AXE after

Checklist

kibanamachine commented Sep 23, 2021

1Copenut commented Sep 23, 2021

User experience wins

Opportunities for improvement

1Copenut left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 27, 2021

cee-chen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Oct 7, 2021

chandlerprall commented Oct 19, 2021

chandlerprall commented Sep 23, 2021 •

edited

Loading