-
Notifications
You must be signed in to change notification settings - Fork 839
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
[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213
[EuiDataGrid] Provide rows for datagrid cells to be owned by #5213
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5213/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to create a new type that's more specific, i.e. something like type EuiDataGridRowProps = Omit<EuiDataGridCellProps, 'id'>
?
@@ -686,12 +745,14 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = ( | |||
mutationRef(el); | |||
}} | |||
> | |||
{accessibilityRows} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a fully-functioning version 🎉 but need to address some issues with unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!! Let me know if I can help at all with unit tests, happy to do so
@@ -17,6 +17,7 @@ import { EuiDataGridBody, Cell, getParentCellContent } from './data_grid_body'; | |||
|
|||
describe('EuiDataGridBody', () => { | |||
const requiredProps = { | |||
gridId: 'grid', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 🎉
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Summary
Fixes #4474 by providing
div[role=row]
s whicharia-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 foundreact-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 anaria-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