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 column header move left focus bug #7698

Merged
merged 3 commits into from
Apr 18, 2024
Merged

[EuiDataGrid] Fix column header move left focus bug #7698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37042bb
Fix column header move left/right action focus bug
cee-chen Apr 17, 2024
66a4955
Fix it harder 🥲
cee-chen Apr 18, 2024
bfa6004
changelog
cee-chen Apr 18, 2024
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
3 changes: 3 additions & 0 deletions changelogs/upcoming/7698.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns` when moving columns to the left/right
26 changes: 26 additions & 0 deletions src/components/datagrid/body/header/column_actions.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('getColumnActions', () => {
const testArgs = {
column: { id: 'B' },
columns: [{ id: 'A' }, { id: 'B' }, { id: 'C' }],
columnFocusIndex: 1,
schema: {},
schemaDetectors,
setVisibleColumns,
Expand Down Expand Up @@ -150,6 +151,8 @@ describe('getColumnActions', () => {
});

describe('column reordering', () => {
jest.useFakeTimers();

describe('default enabled behavior', () => {
const items = getColumnActions(testArgs);
const moveLeft = items[1];
Expand Down Expand Up @@ -189,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]);

Expand All @@ -202,6 +207,27 @@ 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);
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]);
});
});

describe('disabled behavior', () => {
it('renders "Move left" as disabled if already on the first/left-most column', () => {
const items = getColumnActions({
Expand Down
23 changes: 20 additions & 3 deletions src/components/datagrid/body/header/column_actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ({
Expand All @@ -47,6 +48,7 @@ export const getColumnActions = ({
sorting,
switchColumnPos,
setFocusedCell,
columnFocusIndex,
}: GetColumnActions): EuiListGroupItemProps[] => {
if (column.actions === false) {
return [];
Expand All @@ -70,6 +72,7 @@ export const getColumnActions = ({
columns,
switchColumnPos,
setFocusedCell,
columnFocusIndex,
}),
...(column.actions?.additional || []),
];
Expand Down Expand Up @@ -136,25 +139,39 @@ export const getHideColumnAction = ({
*/
type MoveColumnActions = Pick<
GetColumnActions,
'column' | 'columns' | 'switchColumnPos' | 'setFocusedCell'
| 'column'
| 'columns'
| 'switchColumnPos'
| 'setFocusedCell'
| 'columnFocusIndex'
>;

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

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
Comment on lines +162 to +165
Copy link
Member Author

Choose a reason for hiding this comment

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

🥲 I honestly don't know why the fix doesn't work in Kibana without the setTimeout. But I also don't know why we can't repro this in EUI local, so this probably isn't the worst??

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something on Kibana is executed with a setTimeout or some delay too which interferes? 🤔
I think it's ok a use setTimeout here for this case. We might just need to keep an eye out if there are any additional ripple effects or if there are similar issues popping up due to some general usage issue on Kibana?

});
};

if (isColumnActionEnabled('showMoveLeft', column.actions)) {
const onClickMoveLeft = () => {
const targetCol = columns[colIdx - 1];
if (targetCol) {
switchColumnPos(column.id, targetCol.id);
setFocusedCell([colIdx - 1, -1]);
moveFocus('left');
}
};
const action = {
Expand All @@ -174,7 +191,7 @@ const getMoveColumnActions = ({
const targetCol = columns[colIdx + 1];
if (targetCol) {
switchColumnPos(column.id, targetCol.id);
setFocusedCell([colIdx + 1, -1]);
moveFocus('right');
}
};
const action = {
Expand Down
2 changes: 2 additions & 0 deletions src/components/datagrid/body/header/data_grid_header_cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export const EuiDataGridHeaderCell: FunctionComponent<EuiDataGridHeaderCellProps
sorting,
switchColumnPos,
setFocusedCell,
columnFocusIndex: index,
});
}, [
column,
Expand All @@ -99,6 +100,7 @@ export const EuiDataGridHeaderCell: FunctionComponent<EuiDataGridHeaderCellProps
sorting,
switchColumnPos,
setFocusedCell,
index,
]);

const showColumnActions = columnActions && columnActions.length > 0;
Expand Down
Loading