[EuiDataGrid] Provide rows for datagrid cells to be owned by#5213
[EuiDataGrid] Provide rows for datagrid cells to be owned by#5213chandlerprall wants to merge 3 commits intoelastic:mainfrom
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5213/ |
|
@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
Opportunities for improvement
|
1Copenut
left a comment
1Copenut
left a comment
There was a problem hiding this comment.
👍 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-ownsattribute. There's not recent data for screen reader tests, but I'm confident the change is a net improvement.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5213/ |
cee-chen
left a comment
cee-chen
left a comment
There was a problem hiding this comment.
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
| export interface EuiDataGridCellProps { | ||
| /** | ||
| * required when used outside of a role="row" element, so a row can own this cell | ||
| */ | ||
| id?: string; |
There was a problem hiding this comment.
Would it be possible to create a new type that's more specific, i.e. something like type EuiDataGridRowProps = Omit<EuiDataGridCellProps, 'id'>?
| mutationRef(el); | ||
| }} | ||
| > | ||
| {accessibilityRows} |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Got a fully-functioning version 🎉 but need to address some issues with unit tests
There was a problem hiding this comment.
Amazing!! Let me know if I can help at all with unit tests, happy to do so
|
|
||
| describe('EuiDataGridBody', () => { | ||
| const requiredProps = { | ||
| gridId: 'grid', |
There was a problem hiding this comment.
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');
});
});| `${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`, |
| 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(' ')} /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
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?
|
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? |
|
Superseded by #5285, keeping in draft mode for now to allow comparison. |
5 participants
Summary
Fixes #4474 by providing
div[role=row]s whicharia-ownscells.@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 foundreact-windowprovides 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 anaria-ownsattribute 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