[EuiDataGrid] Provide row elements to wrap cells #5285

chandlerprall · 2021-10-19T16:51:41Z

Replaces #5213

Summary

Fixes #4474 and fixes #4483 by providing div[role=row]s to wrap the cells. This is done by portalling the cells into divs that are created on-demand. Technically this adds a side-effect to rendering which is a no-no, but the timing works out and doesn't add a noticeable delay.

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

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

chandlerprall · 2021-10-19T17:07:32Z

from #5213 (comment)

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?

This PR brings back highlight-on-hover, the ability to highlight based on selection (& other criteria) can be followed up with, tracked by #4483 as that needs an addition to the API.

kibanamachine · 2021-10-19T17:51:44Z

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

src/components/datagrid/body/data_grid_body.tsx

1Copenut

This version tested a lot better than the other version. I have a few additional suggestions that fall outside the scope of this PR with regard to screen reader announcements, but I'll file those as separate issue tickets.

src/components/datagrid/body/data_grid_row_manager.ts

kibanamachine · 2021-11-01T19:35:38Z

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

chandlerprall · 2021-11-01T19:56:27Z

Pushed up a cypress test to validate all cells belong to a row or columnheader

kibanamachine · 2021-11-01T20:20:05Z

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

cee-chen · 2021-11-01T20:34:16Z

Looks like there's still 3 failing axe tests on datagrid pages that's causing the CI job to be red:

13:17:59 Errors on tp://localhost:9999/#/tabular-content/data-grid-styling-and-control
13:20:03 [landmark-unique]: Landmarks must have a unique role or role/label/title (i.e. accessible name) combination
13:20:03   Help: https://dequeuniversity.com/rules/axe/4.1/landmark-unique?application=axe-puppeteer
13:20:03   Elements:
13:20:03     - .euiPanel--paddingMedium.euiSplitPanel__inner.euiPanel--borderRadiusNone > div > .euiPanel--hasShadow.euiPanel--borderRadiusMedium.euiPanel--plain > .euiDataGrid__focusWrap[data-focus-lock-disabled="disabled"] > .euiDataGrid--bordersHorizontal.euiDataGrid--headerUnderline.euiDataGrid--rowHoverHighlight > .euiDataGrid__pagination > .euiFlexGroup--justifyContentSpaceBetween.euiFlexGroup--gutterLarge.euiFlexGroup--alignItemsCenter > .euiFlexItem--flexGrowZero.euiFlexItem:nth-child(2) > .euiPagination

13:20:03 Errors on tp://localhost:9999/#/tabular-content/data-grid-virtualization
13:20:16 [landmark-unique]: Landmarks must have a unique role or role/label/title (i.e. accessible name) combination
13:20:16   Help: https://dequeuniversity.com/rules/axe/4.1/landmark-unique?application=axe-puppeteer
13:20:16   Elements:
13:20:16     - .euiPanel--paddingMedium.euiSplitPanel__inner.euiPanel--plain > div > .euiDataGrid__focusWrap[data-focus-lock-disabled="disabled"] > .euiDataGrid.euiDataGrid--bordersAll.euiDataGrid--headerShade > .euiDataGrid__pagination > .euiFlexGroup--justifyContentSpaceBetween.euiFlexGroup--gutterLarge.euiFlexGroup--alignItemsCenter > .euiFlexItem--flexGrowZero.euiFlexItem:nth-child(2) > .euiPagination

13:20:16 Errors on tp://localhost:9999/#/tabular-content/data-grid-row-heights-options
13:20:26 [landmark-unique]: Landmarks must have a unique role or role/label/title (i.e. accessible name) combination
13:20:26   Help: https://dequeuniversity.com/rules/axe/4.1/landmark-unique?application=axe-puppeteer
13:20:26   Elements:
13:20:26     - #setting-a-default-height-and-line-height-for-rows > .euiPanel--borderRadiusMedium.euiPanel--hasBorder.euiPanel--flexGrowZero > .euiPanel--paddingMedium.euiSplitPanel__inner.euiPanel--plain > div > .euiDataGrid__focusWrap[data-focus-lock-disabled="disabled"] > .euiDataGrid.euiDataGrid--bordersAll.euiDataGrid--headerShade > .euiDataGrid__pagination > .euiFlexGroup--justifyContentSpaceBetween.euiFlexGroup--gutterLarge.euiFlexGroup--alignItemsCenter > .euiFlexItem--flexGrowZero.euiFlexItem:nth-child(2) > .euiPagination

They're likely related to pagination and not this PR however, so if you want feel free to re-add in these specific 3 datagrid pages to the axe ignore list and we can address the a11y issues in a separate PR!

cee-chen · 2021-11-01T21:15:00Z

src/components/datagrid/body/data_grid_body.tsx

@@ -673,13 +688,27 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
    finalWidth = window.innerWidth;
  }

+  const [dataItemsRendered, setDataItemsRendered] = useState('');


I'm not totally clear on what this dataItemsRendered or the data-itemsrendered attribute is being used for - is it just for Cypress testing? TBH, I'd almost rather do visual snapshot testing over this approach. 1) it feels like it'd be more intuitive to compare outputs & state, and 2) it wouldn't impact realtime performance

It was to provide a way for the test to determine when the rendering is stable. The hooks + virtualization means the shell of the grid + header row is rendered at the initial pass, but then a second (maybe even a third) pass is needed to draw the cells. I couldn't think of a non-hacky way to do this and everything in Cypress's docs + googling said there should be some signal in the DOM to wait for, so I added the data attribute with the virtualization state to hook into.

Totally open to other ideas!

Oh gotcha, sorry! I was totally off on what this was doing in that case, apologies (I thought it was a way of determining cell content 🙈)

I think my worry is potential impact for production grids with lots of data / if this bit of code is overengineered for its purpose. Alternatives:

Can we opt for a simple true/false bool after the first time onItemsRendered gets called instead?

Can we put a data-test-subj on our individual cells and check that the expected number of cells exists instead?

I was anticipating this full information would be useful in future tests against virtualization. Scroll & verify the row&column indices, set the overscanCount prop and verify, etc.

Maybe, to handle the perf impacts as this does update the attribute quite aggressively (though I didn't notice any lag when testing), we enable this with a debug flag on the component? This could open/enable other console logs or other ways of exposing info around the inner workings.

A true/false flag may work, but some tests may need that flag to toggle back to false until reaching stability again.

Can we put a data-test-subj on our individual cells and check that the expected number of cells exists instead?

I can't find a (non-horrible hacky) way to have Cypress wait for a number of elements. We can select for [role=gridcell], but from what I can tell trying to wait for N of them is an anti-pattern. We could add a data-test-subj with ${row},${col} and select/wait for that.

We could add a data-test-subj with ${row},${col} and select/wait for that.

Oo, I like this option a lot! A debug/cypress flag would be fine, but between the two I think the row,col data-test-subj is elegant and could be helpful in general / potentially even in prod debugging.

trying to wait for N of them is an anti-pattern

Ha I have yolo'd too many Cypress anti-patterns in my day attempting to wait for the flakily-un-waitable (recursive xhr polls waiting server side asynchronous cron jobs probably being my least favorite)... 🙈 I definitely appreciate the dialogue and compromise and think we have a few solid approaches here (for the future as well)!

kibanamachine · 2021-11-02T15:37:20Z

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

cee-chen · 2021-11-03T17:43:54Z

@chandlerprall Shoot sorry, you'll need to grab latest master to fix the failing a11y CI test as well to fix the merge conflicts I just made for you 🙈

cee-chen

This LGTM once latest master is merged in - really appreciate the new direction taken with the cell data-test-subjs, the diffs feel super clean to me now :)

src/components/datagrid/data_grid.spec.tsx

src/components/datagrid/body/data_grid_row_manager.ts

src/components/datagrid/body/data_grid_cell.tsx

CHANGELOG.md

kibanamachine · 2021-11-04T21:11:47Z

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

kibanamachine · 2021-11-05T15:15:37Z

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

chandlerprall added 14 commits September 23, 2021 13:51

Provide rows for datagrid cells to be owned by

3de3ce9

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

4594779

changelog

04d3ab5

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

9a6dc07

undoing things

f786f5f

working except virtualized container

06ac499

working row wrapper implementation

0129ea4

Create datagrid row elements on-demand and render cells via portals

c05b457

Provide rows for datagrid cells to be owned by

179da4a

changelog

24f5c53

undoing things

a2ccb72

working except virtualized container

eb4344c

working row wrapper implementation

535a2a9

Create datagrid row elements on-demand and render cells via portals

9151350

chandlerprall added bug accessibility data grid accessibility - markup labels Oct 19, 2021

chandlerprall requested review from cee-chen and 1Copenut October 19, 2021 16:51

chandlerprall mentioned this pull request Oct 19, 2021

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

Closed

4 tasks

chandlerprall changed the title ~~Bug/4474 datagrid row accessibility alternate~~ [EuiDataGrid] Provide row elements to wrap cells Oct 19, 2021

Small style cleanup

814feb1

chandlerprall added 4 commits October 19, 2021 11:08

updated changelog

95b7008

updated changelog

0ba3a02

fixing a bad changelog merge

1569aa0

oh that style was important

9a6df52

cee-chen reviewed Oct 25, 2021

View reviewed changes

src/components/datagrid/body/data_grid_body.tsx Show resolved Hide resolved

renovate bot and others added 2 commits October 26, 2021 11:41

Update dependency @elastic/charts to ^38.1.0 (elastic#5321)

a60d734

PR feedback

ae515b6

1Copenut reviewed Oct 27, 2021

View reviewed changes

src/components/datagrid/body/data_grid_row_manager.ts Outdated Show resolved Hide resolved

src/components/datagrid/body/data_grid_row_manager.ts Show resolved Hide resolved

chandlerprall added 2 commits November 1, 2021 12:58

Added a cypress test for datagrid row rendering

f5b02ef

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

82614cf

chandlerprall added 3 commits November 1, 2021 13:37

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

8336957

Revert changes(?) to docs/

baa7b25

revert changelog reformats

937403e

chandlerprall requested a review from cee-chen November 1, 2021 19:55

cee-chen reviewed Nov 1, 2021

View reviewed changes

re-ignore a couple datagrid example pages when running a11y tests

705edad

Alternate way to know if the datagrid cells have rendered

55a5f95

chandlerprall requested a review from cee-chen November 3, 2021 16:59

cee-chen approved these changes Nov 3, 2021

View reviewed changes

src/components/datagrid/data_grid.spec.tsx Outdated Show resolved Hide resolved

src/components/datagrid/body/data_grid_row_manager.ts Outdated Show resolved Hide resolved

src/components/datagrid/body/data_grid_cell.tsx Outdated Show resolved Hide resolved

chandlerprall added 2 commits November 4, 2021 13:48

Merge branch 'main' into bug/4474-datagrid-row-accessibility-alternate

420b37d

PR feedback

ad5c72b

cee-chen reviewed Nov 4, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

changelog

6d10f54

chandlerprall merged commit 51cfbb7 into elastic:main Nov 5, 2021

chandlerprall deleted the bug/4474-datagrid-row-accessibility-alternate branch November 5, 2021 16:01

1Copenut mentioned this pull request Nov 19, 2021

Bumping EUI to v41.2.2 elastic/kibana#119205

Merged

cee-chen mentioned this pull request Nov 22, 2021

[EuiDataGrid] Provide a way to highlight a row #4401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Provide row elements to wrap cells #5285

[EuiDataGrid] Provide row elements to wrap cells #5285

chandlerprall commented Oct 19, 2021 •

edited

Loading

chandlerprall commented Oct 19, 2021

kibanamachine commented Oct 19, 2021

1Copenut left a comment

kibanamachine commented Nov 1, 2021

chandlerprall commented Nov 1, 2021

kibanamachine commented Nov 1, 2021

cee-chen commented Nov 1, 2021 •

edited

Loading

cee-chen Nov 1, 2021

chandlerprall Nov 1, 2021

cee-chen Nov 1, 2021

chandlerprall Nov 2, 2021

cee-chen Nov 2, 2021

kibanamachine commented Nov 2, 2021

cee-chen commented Nov 3, 2021

cee-chen left a comment

kibanamachine commented Nov 4, 2021

kibanamachine commented Nov 5, 2021

[EuiDataGrid] Provide row elements to wrap cells #5285

[EuiDataGrid] Provide row elements to wrap cells #5285

Conversation

chandlerprall commented Oct 19, 2021 • edited Loading

Summary

AXE before

AXE after

Checklist

chandlerprall commented Oct 19, 2021

kibanamachine commented Oct 19, 2021

1Copenut left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 1, 2021

chandlerprall commented Nov 1, 2021

kibanamachine commented Nov 1, 2021

cee-chen commented Nov 1, 2021 • edited Loading

cee-chen Nov 1, 2021

Choose a reason for hiding this comment

chandlerprall Nov 1, 2021

Choose a reason for hiding this comment

cee-chen Nov 1, 2021

Choose a reason for hiding this comment

chandlerprall Nov 2, 2021

Choose a reason for hiding this comment

cee-chen Nov 2, 2021

Choose a reason for hiding this comment

kibanamachine commented Nov 2, 2021

cee-chen commented Nov 3, 2021

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 4, 2021

kibanamachine commented Nov 5, 2021

chandlerprall commented Oct 19, 2021 •

edited

Loading

cee-chen commented Nov 1, 2021 •

edited

Loading