Skip to content

Commit

Permalink
[EuiDataGrid] Fix sorting console errors + header cell focus cleanup/…
Browse files Browse the repository at this point in the history 
…refactors (#5209)

* [Fix] Fix console error when dragging to reorder columns

* [Fix] DRY out enableInteractives call

Setting isCellEntered to true already makes that same call, so *should* have that same effect as long as we're consistent about changing logic via isCellEntered

This also fixes #4384, which was a focus-lock issue caused focusin/focusout events firing multiple times. Now with the setIsCellEntered change, focus does not fire repeatedly

* [Refactor] Remove unnecessary useEffect dependencies

- now that we're not calling enableInteractives and focusInteractives directly but letting isCellEntered handle that for us, there's no need to include them

+ setIsCellEntered isn't necessary either/doesn't trigger the lint rule

* [Refactor] Improve focus unit tests

- Separate focus tests for isFocused logic vs focusin / focusout events

* [Refactor] Move interactive fns to separate/external fns vs. useCallback

- Removes the need for useCallback and setting the utility fns as dependencies, simplifying code

- refactor for loops to forEach's

- remove setIsCellEntered(true) in focusInteractables, since it now only gets called when isCellEntered is true

* [Refactor] Combine enable and focus interactives action

- they don't get called separately, so it makes sense to optimize the call and not make an extra tabbables call (+ reduces unnecessary branching)

* [Refactor] More complete multiple interactive children unit tests

- covers last uncovered branch in file

- move to bottom of the file rather than top, since after talking to Chandler this is an edge case that only applies to control header cells

+ remove `data-euigrid-tab-managed` attr - tests should pass without it

* [Refactor] Misc cleanup

- convert functions to arrow functions
- use shorter headerNode var instead of ref

* [Fix] Remove F2 key event

- it's had no effect since we switched header cells to use a popover for actions

* Add changelog entry

* Update changelog
  • Loading branch information
Constance authored Sep 27, 2021
1 parent 7d1bbc5 commit 6fe740c
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 165 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
**Bug fixes**

- Fixed logo icons with static SVG IDs causing accessibility errors when multiples of the same logo were present ([#5204](https://github.com/elastic/eui/pull/5204))
- Fixed several `EuiDataGrid` console errors that occur on column drag/drop reorder ([#5209](https://github.com/elastic/eui/pull/5209))

**Reverts**

Expand Down
191 changes: 104 additions & 87 deletions src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('EuiDataGridHeaderCellWrapper', () => {
id: 'someColumn',
index: 0,
headerIsInteractive: true,
children: <button data-euigrid-tab-managed="true" />,
children: <button />,
};

const focusContext = {
Expand Down Expand Up @@ -57,9 +57,7 @@ describe('EuiDataGridHeaderCellWrapper', () => {
style={Object {}}
tabIndex={-1}
>
<button
data-euigrid-tab-managed="true"
/>
<button />
</div>
</EuiDataGridHeaderCellWrapper>
`);
Expand All @@ -85,51 +83,56 @@ describe('EuiDataGridHeaderCellWrapper', () => {
}
tabIndex={-1}
>
<button
data-euigrid-tab-managed="true"
/>
<button />
</div>
`);
});

describe('focus behavior', () => {
it('warns when a header cell contains multiple interactive children', () => {
const consoleWarnSpy = jest
.spyOn(console, 'warn')
.mockImplementationOnce(() => {}); // Silence expected warning

mountWithContext({
children: (
<div>
<button data-euigrid-tab-managed="true" />
<button data-euigrid-tab-managed="true" />
<button data-euigrid-tab-managed="true" />
</div>
),
// Reusable assertions for DRYness
const expectCellFocused = (headerCell: Element) => {
expect(document.activeElement).toEqual(headerCell);
expect(headerCell.getAttribute('tabIndex')).toEqual('0');
};
const expectCellChildrenFocused = (headerCell: Element) => {
expect(document.activeElement).toEqual(
headerCell.querySelector('[data-euigrid-tab-managed]')
);
expect(headerCell.getAttribute('tabIndex')).toEqual('-1');
};
const expectCellNotFocused = (headerCell: Element) => {
expect(document.activeElement).toBeInstanceOf(HTMLBodyElement);
expect(headerCell.getAttribute('tabIndex')).toEqual('-1');
};

describe('isFocused context', () => {
describe('when true', () => {
it('focuses the interactive cell children when present', () => {
const isFocused = true;
const headerCell = mountWithContext({}, isFocused).getDOMNode();
expectCellChildrenFocused(headerCell);
});

it('focuses the cell when no interactive children are present', () => {
const isFocused = true;
const headerCell = mountWithContext(
{ children: <div /> },
isFocused
).getDOMNode();
expectCellFocused(headerCell);
});
});

expect(consoleWarnSpy).toHaveBeenCalledWith(
'EuiDataGridHeaderCell expects at most 1 tabbable element, 3 found instead'
);
describe('when false', () => {
it('sets isCellEntered to false and disables interactives', () => {
const isFocused = false;
const headerCell = mountWithContext({}, isFocused).getDOMNode();
expectCellNotFocused(headerCell);
});
});
});

describe('events', () => {
// Reusable assertions for DRYness
const expectCellFocused = (headerCell: Element) => {
expect(document.activeElement).toEqual(headerCell);
expect(headerCell.getAttribute('tabIndex')).toEqual('0');
};
const expectCellChildrenFocused = (headerCell: Element) => {
expect(document.activeElement).toEqual(
headerCell.querySelector('[data-euigrid-tab-managed]')
);
expect(headerCell.getAttribute('tabIndex')).toEqual('-1');
};
const expectCellNotFocused = (headerCell: Element) => {
expect(document.activeElement).toBeInstanceOf(HTMLBodyElement);
expect(headerCell.getAttribute('tabIndex')).toEqual('-1');
};

describe('keyup', () => {
test('enter key sets isCellEntered to true (which focuses the first interactive child in the header cell)', () => {
const headerCell = mountWithContext().getDOMNode();
Expand All @@ -150,22 +153,6 @@ describe('EuiDataGridHeaderCellWrapper', () => {
});
expectCellFocused(headerCell);
});

test('F2 key toggles between cell child focus and cell focus', () => {
const headerCell = mountWithContext().getDOMNode();
act(() => {
headerCell.dispatchEvent(
new KeyboardEvent('keyup', { key: keys.F2 })
);
});
expectCellFocused(headerCell);
act(() => {
headerCell.dispatchEvent(
new KeyboardEvent('keyup', { key: keys.F2 })
);
});
expectCellChildrenFocused(headerCell);
});
});

describe('focus', () => {
Expand All @@ -179,44 +166,36 @@ describe('EuiDataGridHeaderCellWrapper', () => {
jest.restoreAllMocks();
});

describe('when header is not interactive', () => {
it('does not focus in on the cell', () => {
const headerCell = mountWithContext(
{ headerIsInteractive: false },
false
).getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
describe('focusin', () => {
describe('when header is not interactive', () => {
it('does not focus in on the cell', () => {
const headerCell = mountWithContext(
{ headerIsInteractive: false },
false
).getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
});
expectCellNotFocused(headerCell);
});
expectCellNotFocused(headerCell);
});
});

describe('when header is interactive', () => {
it('calls setFocusedCell when not already focused', () => {
const headerCell = mountWithContext({}, false).getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
});
expect(focusContext.setFocusedCell).toHaveBeenCalled();
});

it('focuses the interactive cell children when present', () => {
const headerCell = mountWithContext().getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
describe('when header is interactive', () => {
it('calls setFocusedCell when not already focused', () => {
const headerCell = mountWithContext({}, false).getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
});
expect(focusContext.setFocusedCell).toHaveBeenCalled();
});
expectCellChildrenFocused(headerCell);
});

it('focuses the cell when no interactive children are present', () => {
const headerCell = mountWithContext({
children: <div />,
}).getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
it('re-enables and focuses cell interactives when already focused', () => {
const headerCell = mountWithContext({}, true).getDOMNode();
act(() => {
headerCell.dispatchEvent(new FocusEvent('focusin'));
});
expectCellChildrenFocused(headerCell);
});
expectCellFocused(headerCell);
});
});

Expand Down Expand Up @@ -251,5 +230,43 @@ describe('EuiDataGridHeaderCellWrapper', () => {
});
});
});

describe('multiple interactive children', () => {
const children = (
<div>
<button />
<button />
<button />
</div>
);

let consoleWarnSpy: jest.SpyInstance;

beforeEach(() => {
consoleWarnSpy = jest
.spyOn(console, 'warn')
.mockImplementation(() => {}); // Silence expected warning
});
afterEach(() => {
consoleWarnSpy.mockRestore();
});

it('emits a console warning', () => {
mountWithContext({ children });
expect(consoleWarnSpy).toHaveBeenCalledWith(
'EuiDataGridHeaderCell expects at most 1 tabbable element, 3 found instead'
);
});

it('will still focus in to the first interactable element on cell enter', () => {
const headerCell = mountWithContext({ children }).getDOMNode();
act(() => {
headerCell.dispatchEvent(
new KeyboardEvent('keyup', { key: keys.ENTER })
);
});
expectCellChildrenFocused(headerCell);
});
});
});
});
Loading

0 comments on commit 6fe740c

Please sign in to comment.