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.

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] Fix multiple grid focus restoration issues #5530

Merged
merged 11 commits into from
Jan 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
- Added virtulized rendering option to `EuiSelectableList` with `isVirtualized` ([#5521](https://github.com/elastic/eui/pull/5521))
- Added expanded option properties to `EuiSelectableOption` with `data` ([#5521](https://github.com/elastic/eui/pull/5521))

**Bug fixes**

- Fixed multiple bugs with `EuiDataGrid` keyboard focus restoration ([#5530](https://github.com/elastic/eui/pull/5530))

**Breaking changes**

- Changed `EuiSearchBar` to preserve phrases with leading and trailing spaces, instead of dropping surrounding whitespace ([#5514](https://github.com/elastic/eui/pull/5514))
Expand Down
1 change: 1 addition & 0 deletions src/components/datagrid/body/data_grid_body.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe('EuiDataGridBody', () => {
resetAfterRowIndex: jest.fn(),
} as any,
},
gridItemsRendered: {} as any,
wrapperRef: { current: document.createElement('div') },
};

Expand Down
4 changes: 4 additions & 0 deletions src/components/datagrid/body/data_grid_body.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
gridStyles,
gridWidth,
gridRef,
gridItemsRendered,
wrapperRef,
} = props;

Expand Down Expand Up @@ -442,6 +443,9 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
<Grid
{...(virtualizationOptions ? virtualizationOptions : {})}
ref={gridRef}
onItemsRendered={(itemsRendered) => {
gridItemsRendered.current = itemsRendered;
}}
chandlerprall marked this conversation as resolved.
Show resolved Hide resolved
innerElementType={InnerElement}
outerRef={outerGridRef}
innerRef={innerGridRef}
Expand Down
51 changes: 42 additions & 9 deletions src/components/datagrid/body/data_grid_cell.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import React from 'react';
import { mount, ReactWrapper } from 'enzyme';
import { keys } from '../../../services';
import { mockRowHeightUtils } from '../utils/__mocks__/row_heights';
import { mockFocusContext } from '../utils/__mocks__/focus_context';
import { DataGridFocusContext } from '../utils/focus';

import { EuiDataGridCell } from './data_grid_cell';
Expand Down Expand Up @@ -166,27 +167,21 @@ describe('EuiDataGridCell', () => {
});

describe('componentDidMount', () => {
const focusContext = {
focusedCell: undefined,
onFocusUpdate: jest.fn(),
setFocusedCell: jest.fn(),
};

it('creates an onFocusUpdate subscription', () => {
mount(
<DataGridFocusContext.Provider value={focusContext}>
<DataGridFocusContext.Provider value={mockFocusContext}>
<EuiDataGridCell {...requiredProps} />
</DataGridFocusContext.Provider>
);

expect(focusContext.onFocusUpdate).toHaveBeenCalled();
expect(mockFocusContext.onFocusUpdate).toHaveBeenCalled();
});

it('mounts the cell with focus state if the current cell should be focused', () => {
const focusSpy = jest.spyOn(HTMLElement.prototype, 'focus');
const component = mount(
<DataGridFocusContext.Provider
value={{ ...focusContext, focusedCell: [3, 3] }}
value={{ ...mockFocusContext, focusedCell: [3, 3] }}
>
<EuiDataGridCell
{...requiredProps}
Expand All @@ -198,6 +193,44 @@ describe('EuiDataGridCell', () => {

expect((component.instance().state as any).isFocused).toEqual(true);
expect(focusSpy).toHaveBeenCalledWith({ preventScroll: true });
expect(mockFocusContext.setIsFocusedCellInView).toHaveBeenCalledWith(
true
);
});
});

describe('componentWillUnmount', () => {
it('unsubscribes from the onFocusUpdate subscription', () => {
const unsubscribeCellMock = jest.fn();
mockFocusContext.onFocusUpdate.mockReturnValueOnce(unsubscribeCellMock);

const component = mount(
<DataGridFocusContext.Provider value={mockFocusContext}>
<EuiDataGridCell {...requiredProps} />
</DataGridFocusContext.Provider>
);
component.unmount();

expect(unsubscribeCellMock).toHaveBeenCalled();
});

it('sets isFocusedCellInView to false if the current cell is focused and unmounting due to being scrolled out of view', () => {
const component = mount(
<DataGridFocusContext.Provider
value={{ ...mockFocusContext, focusedCell: [3, 3] }}
>
<EuiDataGridCell
{...requiredProps}
colIndex={3}
visibleRowIndex={3}
/>
</DataGridFocusContext.Provider>
);
component.unmount();

expect(mockFocusContext.setIsFocusedCellInView).toHaveBeenCalledWith(
false
);
});
});

Expand Down
19 changes: 14 additions & 5 deletions src/components/datagrid/body/data_grid_cell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export class EuiDataGridCell extends Component<
enableInteractions: false,
disableCellTabIndex: false,
};
unsubscribeCell?: Function = () => {};
unsubscribeCell?: Function;
focusTimeout: number | undefined;
style = null;

Expand Down Expand Up @@ -234,16 +234,21 @@ export class EuiDataGridCell extends Component<

// Account for virtualization - when a cell unmounts when scrolled out of view
// and then remounts when scrolled back into view, it should retain focus state
if (
this.context.focusedCell?.[0] === colIndex &&
this.context.focusedCell?.[1] === visibleRowIndex
) {
if (this.isFocusedCell()) {
// The second flag sets preventScroll: true as a focus option, which prevents
// hijacking the user's scroll behavior when the cell re-mounts on scroll
this.onFocusUpdate(true, true);
this.context.setIsFocusedCellInView(true);
}
}

isFocusedCell = () => {
return (
this.context.focusedCell?.[0] === this.props.colIndex &&
this.context.focusedCell?.[1] === this.props.visibleRowIndex
);
};

onFocusUpdate = (isFocused: boolean, preventScroll = false) => {
this.setState({ isFocused }, () => {
if (isFocused) {
Expand All @@ -257,6 +262,10 @@ export class EuiDataGridCell extends Component<
if (this.unsubscribeCell) {
this.unsubscribeCell();
}

if (this.isFocusedCell()) {
this.context.setIsFocusedCellInView(false);
}
}

componentDidUpdate(prevProps: EuiDataGridCellProps) {
Expand Down
15 changes: 13 additions & 2 deletions src/components/datagrid/body/header/column_actions.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,22 @@ import {

describe('getColumnActions', () => {
const setVisibleColumns = jest.fn();
const focusFirstVisibleInteractiveCell = jest.fn();
const setIsPopoverOpen = jest.fn();
const switchColumnPos = jest.fn();
const setFocusedCell = jest.fn();

const testArgs = {
column: { id: 'B' },
columns: [{ id: 'A' }, { id: 'B' }, { id: 'C' }],
schema: {},
schemaDetectors,
setVisibleColumns,
focusFirstVisibleInteractiveCell,
setIsPopoverOpen,
sorting: undefined,
switchColumnPos,
setFocusedCell,
};

// DRY test helper
Expand Down Expand Up @@ -104,9 +108,10 @@ describe('getColumnActions', () => {
`);
});

it('sets column visibility on click', () => {
it('hides the current column on click and refocuses into the grid', () => {
callActionOnClick(hideColumn);
expect(setVisibleColumns).toHaveBeenCalledWith(['A', 'C']);
expect(focusFirstVisibleInteractiveCell).toHaveBeenCalled();
});
});

Expand Down Expand Up @@ -182,12 +187,18 @@ describe('getColumnActions', () => {
`);
});

it('calls switchColumnPos on click', () => {
it('calls switchColumnPos and updates the focused cell column index on click', () => {
callActionOnClick(moveLeft);
expect(switchColumnPos).toHaveBeenCalledWith('B', 'A');
expect(setFocusedCell).toHaveBeenLastCalledWith([0, -1]);

callActionOnClick(moveRight);
expect(switchColumnPos).toHaveBeenCalledWith('B', 'C');
expect(setFocusedCell).toHaveBeenLastCalledWith([2, -1]);

// Quick note about the -1 row index above: `-1` is the index we set for our
// sticky header row, and these column actions can only ever be called by an
// interactive header cell, so we can safely and statically set the -1 index
});
});

Expand Down
23 changes: 20 additions & 3 deletions src/components/datagrid/body/header/column_actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
EuiDataGridSchema,
EuiDataGridSchemaDetector,
EuiDataGridSorting,
DataGridFocusContextShape,
} from '../../data_grid_types';
import { EuiI18n } from '../../../i18n';
import { EuiListGroupItemProps } from '../../../list_group';
Expand All @@ -28,9 +29,11 @@ interface GetColumnActions {
schema: EuiDataGridSchema;
schemaDetectors: EuiDataGridSchemaDetector[];
setVisibleColumns: (columnId: string[]) => void;
focusFirstVisibleInteractiveCell: DataGridFocusContextShape['focusFirstVisibleInteractiveCell'];
setIsPopoverOpen: (value: boolean) => void;
sorting: EuiDataGridSorting | undefined;
switchColumnPos: (colFromId: string, colToId: string) => void;
setFocusedCell: DataGridFocusContextShape['setFocusedCell'];
}

export const getColumnActions = ({
Expand All @@ -39,9 +42,11 @@ export const getColumnActions = ({
schema,
schemaDetectors,
setVisibleColumns,
focusFirstVisibleInteractiveCell,
setIsPopoverOpen,
sorting,
switchColumnPos,
setFocusedCell,
}: GetColumnActions): EuiListGroupItemProps[] => {
if (column.actions === false) {
return [];
Expand All @@ -52,6 +57,7 @@ export const getColumnActions = ({
column,
columns,
setVisibleColumns,
focusFirstVisibleInteractiveCell,
}),
...getSortColumnActions({
column,
Expand All @@ -63,6 +69,7 @@ export const getColumnActions = ({
column,
columns,
switchColumnPos,
setFocusedCell,
}),
...(column.actions?.additional || []),
];
Expand All @@ -85,20 +92,27 @@ export const getColumnActions = ({
*/
type HideColumnAction = Pick<
GetColumnActions,
'column' | 'columns' | 'setVisibleColumns'
| 'column'
| 'columns'
| 'setVisibleColumns'
| 'focusFirstVisibleInteractiveCell'
>;

export const getHideColumnAction = ({
column,
columns,
setVisibleColumns,
focusFirstVisibleInteractiveCell,
}: HideColumnAction): EuiListGroupItemProps[] => {
const items = [];

const onClickHideColumn = () =>
const onClickHideColumn = () => {
setVisibleColumns(
columns.filter((col) => col.id !== column.id).map((col) => col.id)
);
// Since we hid the current column, we need to manually set focus back onto the grid
focusFirstVisibleInteractiveCell();
};

const action = {
label: (
Expand All @@ -122,13 +136,14 @@ export const getHideColumnAction = ({
*/
type MoveColumnActions = Pick<
GetColumnActions,
'column' | 'columns' | 'switchColumnPos'
'column' | 'columns' | 'switchColumnPos' | 'setFocusedCell'
>;

const getMoveColumnActions = ({
column,
columns,
switchColumnPos,
setFocusedCell,
}: MoveColumnActions): EuiListGroupItemProps[] => {
const items = [];

Expand All @@ -139,6 +154,7 @@ const getMoveColumnActions = ({
const targetCol = columns[colIdx - 1];
if (targetCol) {
switchColumnPos(column.id, targetCol.id);
setFocusedCell([colIdx - 1, -1]);
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
}
};
const action = {
Expand All @@ -158,6 +174,7 @@ const getMoveColumnActions = ({
const targetCol = columns[colIdx + 1];
if (targetCol) {
switchColumnPos(column.id, targetCol.id);
setFocusedCell([colIdx + 1, -1]);
}
};
const action = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { EuiIcon } from '../../../icon';
import { EuiListGroup } from '../../../list_group';
import { EuiPopover } from '../../../popover';
import { DataGridSortingContext } from '../../utils/sorting';
import { DataGridFocusContext } from '../../utils/focus';
import { EuiDataGridHeaderCellProps } from '../../data_grid_types';

import { getColumnActions } from './column_actions';
Expand Down Expand Up @@ -58,6 +59,10 @@ export const EuiDataGridHeaderCell: FunctionComponent<EuiDataGridHeaderCellProps
} = {};
const screenReaderId = useGeneratedHtmlId();

const { setFocusedCell, focusFirstVisibleInteractiveCell } = useContext(
DataGridFocusContext
);

const { sorting } = useContext(DataGridSortingContext);
let sortString;
if (sorting) {
Expand Down Expand Up @@ -92,9 +97,11 @@ export const EuiDataGridHeaderCell: FunctionComponent<EuiDataGridHeaderCellProps
schema,
schemaDetectors,
setVisibleColumns,
focusFirstVisibleInteractiveCell,
setIsPopoverOpen,
sorting,
switchColumnPos,
setFocusedCell,
});

const showColumnActions = columnActions && columnActions.length > 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { mount } from 'enzyme';

import { keys } from '../../../../services';
import { DataGridFocusContext } from '../../utils/focus';
import { mockFocusContext } from '../../utils/__mocks__/focus_context';

import { EuiDataGridHeaderCellWrapper } from './data_grid_header_cell_wrapper';

Expand All @@ -23,16 +24,12 @@ describe('EuiDataGridHeaderCellWrapper', () => {
children: <button />,
};

const focusContext = {
setFocusedCell: jest.fn(),
onFocusUpdate: jest.fn(),
};
const mountWithContext = (props = {}, isFocused = true) => {
(focusContext.onFocusUpdate as jest.Mock).mockImplementation(
(mockFocusContext.onFocusUpdate as jest.Mock).mockImplementation(
(_, callback) => callback(isFocused) // allows us to mock isFocused state
);
return mount(
<DataGridFocusContext.Provider value={focusContext}>
<DataGridFocusContext.Provider value={mockFocusContext}>
<EuiDataGridHeaderCellWrapper {...requiredProps} {...props} />
</DataGridFocusContext.Provider>
);
Expand Down Expand Up @@ -186,7 +183,7 @@ describe('EuiDataGridHeaderCellWrapper', () => {
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
});
expect(focusContext.setFocusedCell).toHaveBeenCalled();
expect(mockFocusContext.setFocusedCell).toHaveBeenCalled();
});

it('re-enables and focuses cell interactives when already focused', () => {
Expand Down
Loading