Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

Merged
merged 17 commits into from
Jan 18, 2022
Merged

[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4068925
Workaround forcing the datagrid to fully scroll cells into view when …
cee-chen Jan 5, 2022
961677c
Set up test helper for unit testing custom hooks
cee-chen Jan 5, 2022
99b370f
Add unit tests for focus keyboard scroll workaround
cee-chen Jan 5, 2022
e72953e
Add changelog entry
cee-chen Jan 5, 2022
51e405d
Revert previous approach
cee-chen Jan 13, 2022
1e5999b
[REVERT] Test sticky footer behavior
cee-chen Jan 13, 2022
e3db4f8
[Setup] Add `data-gridcell-id` to header cells + Fix coord order to m…
cee-chen Jan 13, 2022
bbd7587
Write separate new scroll helper and logic
cee-chen Jan 14, 2022
b1cd532
Write tests
cee-chen Jan 14, 2022
2c307ed
[misc] Update comment
cee-chen Jan 14, 2022
91b0e00
[bugfix] Correctly account for cells that are wider or taller than th…
cee-chen Jan 14, 2022
9d950a9
Merge branch 'main' into datagrid/3151/4456
cee-chen Jan 14, 2022
d218634
Comment copy tweak
constancecchen Jan 14, 2022
f444d56
[PR feedback] Separate cell row/column data into individual data-attrs
cee-chen Jan 18, 2022
a410c56
[PR feedback] Clarify clientWidth/Height comment
cee-chen Jan 18, 2022
d4f5293
Revert "[REVERT] Test sticky footer behavior"
cee-chen Jan 18, 2022
b7d4cee
Merge branch 'main' into datagrid/3151/4456
cee-chen Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
## [`main`](https://github.com/elastic/eui/tree/main)

- Updated the outline color in `euiCustomControlFocused` mixin to use
`$euiFocusRingColor` instead of `currentColor` ([#5479](https://github.com/elastic/eui/pull/5479))
**Bug fixes**

- Updated the outline color in `euiCustomControlFocused` mixin to use `$euiFocusRingColor` instead of `currentColor` ([#5479](https://github.com/elastic/eui/pull/5479))
- Fixed keyboard navigation in `EuiDataGrid` not fully scrolling cells into view ([#5515](https://github.com/elastic/eui/pull/5515))

## [`45.0.0`](https://github.com/elastic/eui/tree/v45.0.0)

Expand Down
2 changes: 2 additions & 0 deletions src-docs/src/views/datagrid/footer_row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ export default () => {
rowHover: 'highlight',
header: 'underline',
}}
width={330}
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
height={330}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
52 changes: 31 additions & 21 deletions src/components/datagrid/__snapshots__/data_grid.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,7 @@ Array [
>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-A"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -1087,6 +1088,7 @@ Array [
</div>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="1,-1"
data-test-subj="dataGridHeaderCell-B"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -1154,7 +1156,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn"
data-gridcell-id="0,1"
data-gridcell-id="1,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:0px;height:34px;width:100px"
Expand Down Expand Up @@ -1182,7 +1184,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--stripe euiDataGridRowCell--firstColumn"
data-gridcell-id="1,0"
data-gridcell-id="0,1"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:34px;height:34px;width:100px"
Expand Down Expand Up @@ -1238,7 +1240,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--firstColumn"
data-gridcell-id="2,0"
data-gridcell-id="0,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:68px;height:34px;width:100px"
Expand Down Expand Up @@ -1266,7 +1268,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn"
data-gridcell-id="2,1"
data-gridcell-id="1,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:68px;height:34px;width:100px"
Expand Down Expand Up @@ -1441,6 +1443,7 @@ Array [
>
<div
class="euiDataGridHeaderCell euiDataGridHeaderCell--controlColumn"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-leading"
role="columnheader"
style="width:50px"
Expand All @@ -1456,6 +1459,7 @@ Array [
</div>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="1,-1"
data-test-subj="dataGridHeaderCell-A"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -1494,6 +1498,7 @@ Array [
</div>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="2,-1"
data-test-subj="dataGridHeaderCell-B"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -1532,6 +1537,7 @@ Array [
</div>
<div
class="euiDataGridHeaderCell euiDataGridHeaderCell--controlColumn"
data-gridcell-id="3,-1"
data-test-subj="dataGridHeaderCell-trailing"
role="columnheader"
style="width:50px"
Expand Down Expand Up @@ -1595,7 +1601,7 @@ Array [
</div>
<div
class="euiDataGridRowCell"
data-gridcell-id="0,1"
data-gridcell-id="1,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:50px;top:0px;height:34px;width:100px"
Expand Down Expand Up @@ -1623,7 +1629,7 @@ Array [
</div>
<div
class="euiDataGridRowCell"
data-gridcell-id="0,2"
data-gridcell-id="2,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:150px;top:0px;height:34px;width:100px"
Expand Down Expand Up @@ -1651,7 +1657,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn euiDataGridRowCell--controlColumn"
data-gridcell-id="0,3"
data-gridcell-id="3,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:250px;top:0px;height:34px;width:50px"
Expand Down Expand Up @@ -1698,7 +1704,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--stripe euiDataGridRowCell--firstColumn euiDataGridRowCell--controlColumn"
data-gridcell-id="1,0"
data-gridcell-id="0,1"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:34px;height:34px;width:50px"
Expand Down Expand Up @@ -1773,7 +1779,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--stripe"
data-gridcell-id="1,2"
data-gridcell-id="2,1"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:150px;top:34px;height:34px;width:100px"
Expand Down Expand Up @@ -1801,7 +1807,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--stripe euiDataGridRowCell--lastColumn euiDataGridRowCell--controlColumn"
data-gridcell-id="1,3"
data-gridcell-id="3,1"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:250px;top:34px;height:34px;width:50px"
Expand Down Expand Up @@ -1848,7 +1854,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--firstColumn euiDataGridRowCell--controlColumn"
data-gridcell-id="2,0"
data-gridcell-id="0,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:68px;height:34px;width:50px"
Expand Down Expand Up @@ -1895,7 +1901,7 @@ Array [
</div>
<div
class="euiDataGridRowCell"
data-gridcell-id="2,1"
data-gridcell-id="1,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:50px;top:68px;height:34px;width:100px"
Expand Down Expand Up @@ -1951,7 +1957,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn euiDataGridRowCell--controlColumn"
data-gridcell-id="2,3"
data-gridcell-id="3,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:250px;top:68px;height:34px;width:50px"
Expand Down Expand Up @@ -2142,6 +2148,7 @@ Array [
>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-A"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -2180,6 +2187,7 @@ Array [
</div>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="1,-1"
data-test-subj="dataGridHeaderCell-B"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -2249,7 +2257,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn"
data-gridcell-id="0,1"
data-gridcell-id="1,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:0px;height:34px;width:100px"
Expand Down Expand Up @@ -2277,7 +2285,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--stripe euiDataGridRowCell--firstColumn"
data-gridcell-id="1,0"
data-gridcell-id="0,1"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:34px;height:34px;width:100px"
Expand Down Expand Up @@ -2333,7 +2341,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--firstColumn"
data-gridcell-id="2,0"
data-gridcell-id="0,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:68px;height:34px;width:100px"
Expand Down Expand Up @@ -2361,7 +2369,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn"
data-gridcell-id="2,1"
data-gridcell-id="1,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:68px;height:34px;width:100px"
Expand Down Expand Up @@ -2533,6 +2541,7 @@ Array [
>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-A"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -2571,6 +2580,7 @@ Array [
</div>
<div
class="euiDataGridHeaderCell"
data-gridcell-id="1,-1"
data-test-subj="dataGridHeaderCell-B"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -2638,7 +2648,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn"
data-gridcell-id="0,1"
data-gridcell-id="1,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:0px;height:34px;width:100px"
Expand Down Expand Up @@ -2666,7 +2676,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--stripe euiDataGridRowCell--firstColumn"
data-gridcell-id="1,0"
data-gridcell-id="0,1"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:34px;height:34px;width:100px"
Expand Down Expand Up @@ -2722,7 +2732,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--firstColumn"
data-gridcell-id="2,0"
data-gridcell-id="0,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:0;top:68px;height:34px;width:100px"
Expand Down Expand Up @@ -2750,7 +2760,7 @@ Array [
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--lastColumn"
data-gridcell-id="2,1"
data-gridcell-id="1,2"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:68px;height:34px;width:100px"
Expand Down
4 changes: 3 additions & 1 deletion src/components/datagrid/body/__snapshots__/data_grid_body.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ exports[`EuiDataGridBody renders 1`] = `
>
<div
class="euiDataGridHeaderCell euiDataGridHeaderCell--boolean"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-columnA"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -53,6 +54,7 @@ exports[`EuiDataGridBody renders 1`] = `
</div>
<div
class="euiDataGridHeaderCell euiDataGridHeaderCell--string"
data-gridcell-id="1,-1"
data-test-subj="dataGridHeaderCell-columnB"
role="columnheader"
style="width:100px"
Expand Down Expand Up @@ -122,7 +124,7 @@ exports[`EuiDataGridBody renders 1`] = `
</div>
<div
class="euiDataGridRowCell euiDataGridRowCell--string euiDataGridRowCell--lastColumn"
data-gridcell-id="0,1"
data-gridcell-id="1,0"
data-test-subj="dataGridRowCell"
role="gridcell"
style="position:absolute;left:100px;top:0px;height:34px;width:100px"
Expand Down
14 changes: 14 additions & 0 deletions src/components/datagrid/body/data_grid_body.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
import { useDefaultColumnWidth, useColumnWidths } from '../utils/col_widths';
import { useRowHeightUtils, useDefaultRowHeight } from '../utils/row_heights';
import { useHeaderFocusWorkaround } from '../utils/focus';
import { useScroll } from '../utils/scrolling';
import { DataGridSortingContext } from '../utils/sorting';
import { IS_JEST_ENVIRONMENT } from '../../../test';

Expand Down Expand Up @@ -359,6 +360,19 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
visibleRowCount,
]);

/**
* Handles scroll cells fully into view
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
*/
useScroll({
gridRef,
outerGridRef,
innerGridRef,
headerRowHeight,
footerRowHeight,
visibleRowCount,
hasStickyFooter: !!(renderFooterCellValue && gridStyles.stickyFooter),
});

/**
* Row manager
*/
Expand Down
2 changes: 1 addition & 1 deletion src/components/datagrid/body/data_grid_cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ export class EuiDataGridCell extends Component<
ref={this.cellRef}
{...cellProps}
data-test-subj="dataGridRowCell"
data-gridcell-id={`${this.props.rowIndex},${this.props.colIndex}`}
data-gridcell-id={`${this.props.colIndex},${this.props.visibleRowIndex}`}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chandlerprall This is possibly the dicey change in this approach (but unfortunately is necessary, as my entire approach requires being able to obtain the outermost cell DOM node).

I changed the order because it's confusingly the opposite of setFocusedCell which is [colIndex, rowIndex], and I thought the x,y coordinates made a bit more sense.

I also changed this from rowIndex to visibleRowIndex because that's what the focused cell behavior uses for indices, and it feels somewhat more accurate to how react-window behaves - that is to say, on pagination it doesn't actually destroy the cells that exist but instead replaces its contents.

That being said, if that's too big of a change for this PR, maybe we should add a new data attr instead like data-gridcell-location? Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can swap to x,y , but it would be a breaking change (+ Kibana has two affected unit tests) which is okay, and I agree that the x,y pattern fits better.

Instead of changing this attribute to be the visible row index, let's use that second data-gridcell-location idea and provide both.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f444d56

Per our sync call:

  • I've added 4 new props, data-gridcell-column-id, data-gridcell-column-index, data-gridcell-row-index, and data-gridcell-visible-row-index (with code comments for each prop to help indicate their use-cases).
  • This allows for flexibility in choosing specific cells by the cell location vs cell data, or even choosing entire rows or columns.
  • data-gridcell-id will be deprecated in 3 months in favor of using each 4 separated data attrs

onKeyDown={handleCellKeyDown}
onFocus={this.onFocus}
onMouseEnter={() => {
Expand Down
2 changes: 2 additions & 0 deletions src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ describe('EuiDataGridHeaderCellWrapper', () => {
>
<div
className="euiDataGridHeaderCell"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-someColumn"
role="columnheader"
style={Object {}}
Expand All @@ -71,6 +72,7 @@ describe('EuiDataGridHeaderCellWrapper', () => {
<div
aria-label="test"
className="euiDataGridHeaderCell euiDataGridHeaderCell--test"
data-gridcell-id="0,-1"
data-test-subj="dataGridHeaderCell-someColumn"
role="columnheader"
style={
Expand Down
1 change: 1 addition & 0 deletions src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent<EuiDataGridHeaderCe
tabIndex={isFocused && !isCellEntered ? 0 : -1}
className={classes}
data-test-subj={`dataGridHeaderCell-${id}`}
data-gridcell-id={`${index},-1`}
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
style={width != null ? { width: `${width}px` } : {}}
{...rest}
>
Expand Down
Loading