From 6be8f53b5851b87d6e393161a9442db216c241fb Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 21 Sep 2021 15:30:02 -0700 Subject: [PATCH 01/11] [Fix] Fix console error when dragging to reorder columns --- src/components/datagrid/column_selector.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/components/datagrid/column_selector.tsx b/src/components/datagrid/column_selector.tsx index d3210fdd23e..f87f152d130 100644 --- a/src/components/datagrid/column_selector.tsx +++ b/src/components/datagrid/column_selector.tsx @@ -94,13 +94,15 @@ export const useDataGridColumnSelector = ( source: { index: sourceIndex }, destination, }: DropResult) { - const destinationIndex = destination!.index; - const nextSortedColumns = euiDragDropReorder( - sortedColumns, - sourceIndex, - destinationIndex - ); - setColumns(nextSortedColumns); + if (destination) { + const destinationIndex = destination.index; + const nextSortedColumns = euiDragDropReorder( + sortedColumns, + sourceIndex, + destinationIndex + ); + setColumns(nextSortedColumns); + } } const numberOfHiddenFields = availableColumns.length - visibleColumns.length; From ac5e0587b9333c31dc59a0c678d39399ef3bae7c Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 21 Sep 2021 15:51:02 -0700 Subject: [PATCH 02/11] [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 --- .../datagrid/body/header/data_grid_header_cell_wrapper.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx index 92ab59cf53e..76c9b61d5a7 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx @@ -122,10 +122,8 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent Date: Tue, 21 Sep 2021 15:55:01 -0700 Subject: [PATCH 03/11] [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 --- .../body/header/data_grid_header_cell_wrapper.tsx | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx index 76c9b61d5a7..f91af14de73 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx @@ -175,15 +175,7 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent Date: Tue, 21 Sep 2021 13:32:19 -0700 Subject: [PATCH 04/11] [Refactor] Improve focus unit tests - Separate focus tests for isFocused logic vs focusin / focusout events --- .../data_grid_header_cell_wrapper.test.tsx | 115 ++++++++++-------- 1 file changed, 67 insertions(+), 48 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx index 4e5a54bb515..2688a576c48 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx @@ -113,23 +113,50 @@ describe('EuiDataGridHeaderCellWrapper', () => { ); }); - 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'); - }; + // 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:
}, + isFocused + ).getDOMNode(); + expectCellFocused(headerCell); + }); + }); + + describe('when false', () => { + it('sets isCellEntered to false and disables interactives', () => { + const isFocused = false; + const headerCell = mountWithContext({}, isFocused).getDOMNode(); + expectCellNotFocused(headerCell); + }); + }); + }); + + describe('events', () => { describe('keyup', () => { test('enter key sets isCellEntered to true (which focuses the first interactive child in the header cell)', () => { const headerCell = mountWithContext().getDOMNode(); @@ -179,44 +206,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')); - }); - 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')); + 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); }); - 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:
, - }).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); }); }); From 541359aa1479ab878cef59fc3108efeeeab1b7d8 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 21 Sep 2021 14:37:46 -0700 Subject: [PATCH 05/11] [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 --- .../header/data_grid_header_cell_wrapper.tsx | 73 +++++++++---------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx index f91af14de73..4cfb57c3f30 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx @@ -13,7 +13,6 @@ import React, { useEffect, useRef, useState, - useCallback, } from 'react'; import tabbable from 'tabbable'; import { keys } from '../../../../services'; @@ -47,37 +46,6 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent(null); const [isCellEntered, setIsCellEntered] = useState(false); - const focusInteractives = useCallback((headerNode: Element) => { - const tabbables = tabbable(headerNode); - if (tabbables.length === 1) { - tabbables[0].focus(); - setIsCellEntered(true); - } - }, []); - - const enableInteractives = useCallback((headerNode: Element) => { - const interactiveElements = headerNode.querySelectorAll( - '[data-euigrid-tab-managed]' - ); - for (let i = 0; i < interactiveElements.length; i++) { - interactiveElements[i].setAttribute('tabIndex', '0'); - } - }, []); - - const disableInteractives = useCallback((headerNode: Element) => { - const tababbles = tabbable(headerNode); - if (tababbles.length > 1) { - console.warn( - `EuiDataGridHeaderCell expects at most 1 tabbable element, ${tababbles.length} found instead` - ); - } - for (let i = 0; i < tababbles.length; i++) { - const element = tababbles[i]; - element.setAttribute('data-euigrid-tab-managed', 'true'); - element.setAttribute('tabIndex', '-1'); - } - }, []); - useEffect(() => { const headerNode = headerRef.current!; @@ -87,12 +55,7 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent { const headerNode = headerRef.current!; @@ -191,3 +154,37 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent ); }; + +/** + * Utility fns for managing child interactive tabIndex state + */ + +const disableInteractives = (headerNode: Element) => { + const tabbables = tabbable(headerNode); + if (tabbables.length > 1) { + console.warn( + `EuiDataGridHeaderCell expects at most 1 tabbable element, ${tabbables.length} found instead` + ); + } + tabbables.forEach((element) => { + element.setAttribute('data-euigrid-tab-managed', 'true'); + element.setAttribute('tabIndex', '-1'); + }); +}; + +const enableInteractives = (headerNode: Element) => { + const interactiveElements = headerNode.querySelectorAll( + '[data-euigrid-tab-managed]' + ); + interactiveElements.forEach((element) => { + element.setAttribute('tabIndex', '0'); + }); +}; + +const focusInteractives = (headerNode: Element) => { + const tabbables = tabbable(headerNode); + + if (tabbables.length === 1) { + tabbables[0].focus(); + } +}; From 2df815279bde98e41f2c79e12614f53e2f78f735 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 21 Sep 2021 16:03:16 -0700 Subject: [PATCH 06/11] [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) --- .../header/data_grid_header_cell_wrapper.tsx | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx index 4cfb57c3f30..06f1d088cf7 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx @@ -50,8 +50,7 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent { }); }; -const enableInteractives = (headerNode: Element) => { +const enableAndFocusInteractives = (headerNode: Element) => { const interactiveElements = headerNode.querySelectorAll( '[data-euigrid-tab-managed]' ); - interactiveElements.forEach((element) => { + interactiveElements.forEach((element, i) => { element.setAttribute('tabIndex', '0'); + if (i === 0) { + (element as HTMLElement).focus(); + } }); }; - -const focusInteractives = (headerNode: Element) => { - const tabbables = tabbable(headerNode); - - if (tabbables.length === 1) { - tabbables[0].focus(); - } -}; From 7fdef44308fe2f4fe4b70e5734b07d78daf8d913 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 09:27:17 -0700 Subject: [PATCH 07/11] [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 --- .../data_grid_header_cell_wrapper.test.tsx | 68 +++++++++++-------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx index 2688a576c48..e9a47347a1f 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx @@ -20,7 +20,7 @@ describe('EuiDataGridHeaderCellWrapper', () => { id: 'someColumn', index: 0, headerIsInteractive: true, - children:
`); @@ -85,34 +83,12 @@ describe('EuiDataGridHeaderCellWrapper', () => { } tabIndex={-1} > -
`); }); 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: ( -
-
- ), - }); - - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'EuiDataGridHeaderCell expects at most 1 tabbable element, 3 found instead' - ); - }); - // Reusable assertions for DRYness const expectCellFocused = (headerCell: Element) => { expect(document.activeElement).toEqual(headerCell); @@ -270,5 +246,43 @@ describe('EuiDataGridHeaderCellWrapper', () => { }); }); }); + + describe('multiple interactive children', () => { + const children = ( +
+
+ ); + + 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); + }); + }); }); }); From a6f5c3be766f710be1a97bf1ae687a473f6e82a5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 21 Sep 2021 14:42:31 -0700 Subject: [PATCH 08/11] [Refactor] Misc cleanup - convert functions to arrow functions - use shorter headerNode var instead of ref --- .../body/header/data_grid_header_cell_wrapper.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx index 06f1d088cf7..ffd323dc9b5 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx @@ -73,7 +73,7 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent { if (!headerIsInteractive) { // header is not interactive, avoid focusing requestAnimationFrame(() => headerNode.blur()); @@ -88,19 +88,19 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent { // wait for the next element to receive focus, then update interactives' state requestAnimationFrame(() => { if (!headerNode.contains(document.activeElement)) { setIsCellEntered(false); } }); - } + }; - function onKeyUp(event: KeyboardEvent) { + const onKeyUp = (event: KeyboardEvent) => { switch (event.key) { case keys.ENTER: { event.preventDefault(); @@ -116,7 +116,7 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent Date: Tue, 21 Sep 2021 14:49:13 -0700 Subject: [PATCH 09/11] [Fix] Remove F2 key event - it's had no effect since we switched header cells to use a popover for actions --- .../data_grid_header_cell_wrapper.test.tsx | 16 ---------------- .../header/data_grid_header_cell_wrapper.tsx | 12 ------------ 2 files changed, 28 deletions(-) diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx index e9a47347a1f..e68327abd22 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx @@ -153,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', () => { diff --git a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx index ffd323dc9b5..9ba11f7edb4 100644 --- a/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx +++ b/src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx @@ -114,18 +114,6 @@ export const EuiDataGridHeaderCellWrapper: FunctionComponent Date: Wed, 22 Sep 2021 10:04:23 -0700 Subject: [PATCH 10/11] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae33aff8b1d..4920b64d7e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Fixed `EuiDataGrid` focus ring to be contained in the cell ([#5194](https://github.com/elastic/eui/pull/5194)) - Fixed `EuiDataGrid` cells when focused getting a higher `z-index` which was causing long content to overlap surrounding cells ([#5194](https://github.com/elastic/eui/pull/5194)) - Fixed multiple components unnecessarily rerendering generated IDs on every update ([#5195](https://github.com/elastic/eui/pull/5195), [#5196](https://github.com/elastic/eui/pull/5196), [#5197](https://github.com/elastic/eui/pull/5197), [#5200](https://github.com/elastic/eui/pull/#5200), [#5201](https://github.com/elastic/eui/pull/#5201)) +- Fixed several `EuiDataGrid` console errors that occur on column drag/drop reorder ([#5209](https://github.com/elastic/eui/pull/5209)) **Theme: Amsterdam** From c042b1d5e5ea988ecbc6379f9b7214a309b3dc38 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Mon, 27 Sep 2021 12:47:51 -0700 Subject: [PATCH 11/11] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42d53cc8c55..4439faa251f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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** @@ -25,7 +26,6 @@ - Fixed `EuiDataGrid` cells when focused getting a higher `z-index` which was causing long content to overlap surrounding cells ([#5194](https://github.com/elastic/eui/pull/5194)) - Replaced the `EuiMarkdownEditor` help syntax modal with a popover when no custom plugins are available ([#5147](https://github.com/elastic/eui/pull/5147)) - Fixed multiple components unnecessarily rerendering generated IDs on every update ([#5195](https://github.com/elastic/eui/pull/5195), [#5196](https://github.com/elastic/eui/pull/5196), [#5197](https://github.com/elastic/eui/pull/5197), [#5200](https://github.com/elastic/eui/pull/#5200), [#5201](https://github.com/elastic/eui/pull/#5201)) -- Fixed several `EuiDataGrid` console errors that occur on column drag/drop reorder ([#5209](https://github.com/elastic/eui/pull/5209)) **Theme: Amsterdam**