From 37042bbbe1d4cde5a8e4c0e9627660afe94b7267 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 17 Apr 2024 15:48:51 -0700 Subject: [PATCH 1/3] Fix column header move left/right action focus bug - index being passed to `setFocusCell` wasn't accounting for leading control columns --- .../body/header/column_actions.test.tsx | 20 +++++++++++++++++++ .../datagrid/body/header/column_actions.tsx | 14 ++++++++++--- .../body/header/data_grid_header_cell.tsx | 2 ++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/components/datagrid/body/header/column_actions.test.tsx b/src/components/datagrid/body/header/column_actions.test.tsx index 824060b885b..12a3513074b 100644 --- a/src/components/datagrid/body/header/column_actions.test.tsx +++ b/src/components/datagrid/body/header/column_actions.test.tsx @@ -27,6 +27,7 @@ describe('getColumnActions', () => { const testArgs = { column: { id: 'B' }, columns: [{ id: 'A' }, { id: 'B' }, { id: 'C' }], + columnFocusIndex: 1, schema: {}, schemaDetectors, setVisibleColumns, @@ -202,6 +203,25 @@ describe('getColumnActions', () => { }); }); + describe('focus behavior with leading control columns', () => { + const items = getColumnActions({ + ...testArgs, + columnFocusIndex: testArgs.columnFocusIndex + 2, // "Add" 2 leading control columns + }); + const moveLeft = items[1]; + const moveRight = items[2]; + + it('correctly calls the focused cell x index accounting for leading control columns', () => { + callActionOnClick(moveLeft); + expect(switchColumnPos).toHaveBeenCalledWith('B', 'A'); + expect(setFocusedCell).toHaveBeenLastCalledWith([2, -1]); + + callActionOnClick(moveRight); + expect(switchColumnPos).toHaveBeenCalledWith('B', 'C'); + expect(setFocusedCell).toHaveBeenLastCalledWith([4, -1]); + }); + }); + describe('disabled behavior', () => { it('renders "Move left" as disabled if already on the first/left-most column', () => { const items = getColumnActions({ diff --git a/src/components/datagrid/body/header/column_actions.tsx b/src/components/datagrid/body/header/column_actions.tsx index 3f594ac0703..9638a958dd8 100644 --- a/src/components/datagrid/body/header/column_actions.tsx +++ b/src/components/datagrid/body/header/column_actions.tsx @@ -34,6 +34,7 @@ interface GetColumnActions { sorting: EuiDataGridSorting | undefined; switchColumnPos: (colFromId: string, colToId: string) => void; setFocusedCell: DataGridFocusContextShape['setFocusedCell']; + columnFocusIndex: number; // Index including leadingControlColumns } export const getColumnActions = ({ @@ -47,6 +48,7 @@ export const getColumnActions = ({ sorting, switchColumnPos, setFocusedCell, + columnFocusIndex, }: GetColumnActions): EuiListGroupItemProps[] => { if (column.actions === false) { return []; @@ -70,6 +72,7 @@ export const getColumnActions = ({ columns, switchColumnPos, setFocusedCell, + columnFocusIndex, }), ...(column.actions?.additional || []), ]; @@ -136,7 +139,11 @@ export const getHideColumnAction = ({ */ type MoveColumnActions = Pick< GetColumnActions, - 'column' | 'columns' | 'switchColumnPos' | 'setFocusedCell' + | 'column' + | 'columns' + | 'switchColumnPos' + | 'setFocusedCell' + | 'columnFocusIndex' >; const getMoveColumnActions = ({ @@ -144,6 +151,7 @@ const getMoveColumnActions = ({ columns, switchColumnPos, setFocusedCell, + columnFocusIndex, }: MoveColumnActions): EuiListGroupItemProps[] => { const items = []; @@ -154,7 +162,7 @@ const getMoveColumnActions = ({ const targetCol = columns[colIdx - 1]; if (targetCol) { switchColumnPos(column.id, targetCol.id); - setFocusedCell([colIdx - 1, -1]); + setFocusedCell([columnFocusIndex - 1, -1]); } }; const action = { @@ -174,7 +182,7 @@ const getMoveColumnActions = ({ const targetCol = columns[colIdx + 1]; if (targetCol) { switchColumnPos(column.id, targetCol.id); - setFocusedCell([colIdx + 1, -1]); + setFocusedCell([columnFocusIndex + 1, -1]); } }; const action = { diff --git a/src/components/datagrid/body/header/data_grid_header_cell.tsx b/src/components/datagrid/body/header/data_grid_header_cell.tsx index af1d7a9b953..9aeef041114 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell.tsx @@ -87,6 +87,7 @@ export const EuiDataGridHeaderCell: FunctionComponent 0; From 66a4955886e7d908b513d64d0a620a2d7df8c58b Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 17 Apr 2024 17:45:37 -0700 Subject: [PATCH 2/3] =?UTF-8?q?Fix=20it=20harder=20=F0=9F=A5=B2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - fix doesn't work in Kibana without pulling `setFocusedCell()` out to async, either a timeout or rAF work --- .../datagrid/body/header/column_actions.test.tsx | 6 ++++++ .../datagrid/body/header/column_actions.tsx | 13 +++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/components/datagrid/body/header/column_actions.test.tsx b/src/components/datagrid/body/header/column_actions.test.tsx index 12a3513074b..4f659ef275d 100644 --- a/src/components/datagrid/body/header/column_actions.test.tsx +++ b/src/components/datagrid/body/header/column_actions.test.tsx @@ -151,6 +151,8 @@ describe('getColumnActions', () => { }); describe('column reordering', () => { + jest.useFakeTimers(); + describe('default enabled behavior', () => { const items = getColumnActions(testArgs); const moveLeft = items[1]; @@ -190,10 +192,12 @@ describe('getColumnActions', () => { it('calls switchColumnPos and updates the focused cell column index on click', () => { callActionOnClick(moveLeft); + jest.runAllTimers(); expect(switchColumnPos).toHaveBeenCalledWith('B', 'A'); expect(setFocusedCell).toHaveBeenLastCalledWith([0, -1]); callActionOnClick(moveRight); + jest.runAllTimers(); expect(switchColumnPos).toHaveBeenCalledWith('B', 'C'); expect(setFocusedCell).toHaveBeenLastCalledWith([2, -1]); @@ -213,10 +217,12 @@ describe('getColumnActions', () => { it('correctly calls the focused cell x index accounting for leading control columns', () => { callActionOnClick(moveLeft); + jest.runAllTimers(); expect(switchColumnPos).toHaveBeenCalledWith('B', 'A'); expect(setFocusedCell).toHaveBeenLastCalledWith([2, -1]); callActionOnClick(moveRight); + jest.runAllTimers(); expect(switchColumnPos).toHaveBeenCalledWith('B', 'C'); expect(setFocusedCell).toHaveBeenLastCalledWith([4, -1]); }); diff --git a/src/components/datagrid/body/header/column_actions.tsx b/src/components/datagrid/body/header/column_actions.tsx index 9638a958dd8..cd702168fe5 100644 --- a/src/components/datagrid/body/header/column_actions.tsx +++ b/src/components/datagrid/body/header/column_actions.tsx @@ -157,12 +157,21 @@ const getMoveColumnActions = ({ const colIdx = columns.findIndex((col) => col.id === column.id); + const moveFocus = (direction: 'left' | 'right') => { + const newIndex = direction === 'left' ? -1 : 1; + // Wait a beat to move focus, otherwise the EuiPopover's EuiFocusTrap's + // returnFocus logic sometimes steals it (depending on rerenders) + setTimeout(() => { + setFocusedCell([columnFocusIndex + newIndex, -1]); // -1 is the static y-index of the header + }); + }; + if (isColumnActionEnabled('showMoveLeft', column.actions)) { const onClickMoveLeft = () => { const targetCol = columns[colIdx - 1]; if (targetCol) { switchColumnPos(column.id, targetCol.id); - setFocusedCell([columnFocusIndex - 1, -1]); + moveFocus('left'); } }; const action = { @@ -182,7 +191,7 @@ const getMoveColumnActions = ({ const targetCol = columns[colIdx + 1]; if (targetCol) { switchColumnPos(column.id, targetCol.id); - setFocusedCell([columnFocusIndex + 1, -1]); + moveFocus('right'); } }; const action = { From bfa6004b5a8951acd0d9a398ba3b795d40cc0920 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 17 Apr 2024 17:58:53 -0700 Subject: [PATCH 3/3] changelog --- changelogs/upcoming/7698.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelogs/upcoming/7698.md diff --git a/changelogs/upcoming/7698.md b/changelogs/upcoming/7698.md new file mode 100644 index 00000000000..de8dbcb613c --- /dev/null +++ b/changelogs/upcoming/7698.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns` when moving columns to the left/right