From 9ae02c4e40b2cd3d274266bbb00c4c5a40dc6760 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 23 Nov 2023 12:11:13 -0800 Subject: [PATCH 1/4] [cleanup] remove now-unnecessary cell close popover check - thanks to the moved anchor behavior, we no longer need this, and it's actually messing up other UX --- src/components/datagrid/body/data_grid_cell.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/components/datagrid/body/data_grid_cell.tsx b/src/components/datagrid/body/data_grid_cell.tsx index 0021a8cfc2f..61ad48987c0 100644 --- a/src/components/datagrid/body/data_grid_cell.tsx +++ b/src/components/datagrid/body/data_grid_cell.tsx @@ -457,10 +457,6 @@ export class EuiDataGridCell extends Component< this.setState({ disableCellTabIndex: true }); } }, 0); - // Close the cell popover if the popover was open and the user clicked the cell - if (this.props.popoverContext.popoverIsOpen) { - this.props.popoverContext.closeCellPopover(); - } } }; From a86a52394370ed91614f6a5eac9b1299194fc2cf Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 23 Nov 2023 12:12:27 -0800 Subject: [PATCH 2/4] Fix cell popovers not closing on outside grid click - I had the DOM markup/hierarchy wrong, so the cell popover was never closing + add bonus regresison test for ensuring that clicking other cell actions does not close the popover --- .../body/data_grid_cell_popover.spec.tsx | 77 ++++++++++++++++--- .../datagrid/body/data_grid_cell_popover.tsx | 6 +- 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/src/components/datagrid/body/data_grid_cell_popover.spec.tsx b/src/components/datagrid/body/data_grid_cell_popover.spec.tsx index 657e542661a..03d0271ffd9 100644 --- a/src/components/datagrid/body/data_grid_cell_popover.spec.tsx +++ b/src/components/datagrid/body/data_grid_cell_popover.spec.tsx @@ -87,21 +87,74 @@ describe('EuiDataGridCellPopover', () => { }); }); - it('closes the cell popover when the originating cell is clicked', () => { - cy.realMount(); - cy.get( - '[data-gridcell-row-index="0"][data-gridcell-column-index="0"]' - ).realClick(); + describe('closes the cell popover', () => { + // Mount and open the first cell popover + beforeEach(() => { + cy.realMount(); + cy.get( + '[data-gridcell-row-index="0"][data-gridcell-column-index="0"]' + ).click(); + cy.realPress('Enter'); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should('exist'); + }); - cy.get('[data-test-subj="euiDataGridCellExpandButton"]').click(); - cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should('exist'); + it('when the originating cell is clicked', () => { + cy.get( + '[data-gridcell-row-index="0"][data-gridcell-column-index="0"]' + ).realClick({ position: 'right' }); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should( + 'not.exist' + ); + }); - cy.get( - '[data-gridcell-row-index="0"][data-gridcell-column-index="0"]' - ).realClick({ position: 'right' }); - cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should( - 'not.exist' + it('when the cell expand action button is clicked', () => { + cy.get('[data-test-subj="euiDataGridCellExpandButton"]').click(); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should( + 'not.exist' + ); + }); + + it('when anywhere outside the grid is clicked', () => { + cy.get('body').realClick({ position: 'bottomRight' }); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should( + 'not.exist' + ); + }); + }); + + it('does not close the cell popover when other cell actions are clicked', () => { + const cellActions = [ + ({ Component }) => ( + + ), + ({ Component }) => ( + + ), + ]; + cy.realMount( + ); + cy.get('[data-test-subj="dataGridRowCell"]').first().click(); + cy.realPress('Enter'); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should('exist'); + + cy.get('[data-test-subj="cellActionA"]').first().realClick(); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should('exist'); + + // Close and re-open the cell popover by clicking + cy.get('[data-test-subj="euiDataGridCellExpandButton"]').click(); + cy.get('[data-test-subj="euiDataGridCellExpandButton"]').click(); + + cy.get('[data-test-subj="cellActionB"]').first().realClick(); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should('exist'); }); it('allows consumers to use setCellPopoverProps, passed from renderCellPopover, to customize popover props', () => { diff --git a/src/components/datagrid/body/data_grid_cell_popover.tsx b/src/components/datagrid/body/data_grid_cell_popover.tsx index e7f2e4f0d0a..c02f771fa5e 100644 --- a/src/components/datagrid/body/data_grid_cell_popover.tsx +++ b/src/components/datagrid/body/data_grid_cell_popover.tsx @@ -88,9 +88,9 @@ export const useCellPopover = (): { // clicking the expansion cell action triggers an outside click const onClickOutside = useCallback( (event: Event) => { - if (!popoverAnchor) return; - const cellActions = popoverAnchor.previousElementSibling; - if (cellActions?.contains(event.target as Node) === false) { + const cellActions = + popoverAnchor?.parentElement?.parentElement?.previousElementSibling; + if (!cellActions?.contains(event.target as Node)) { closeCellPopover(); } }, From 9f59e7237dcd1c4cf9e90d4bb84f29ce325ed405 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 23 Nov 2023 12:28:35 -0800 Subject: [PATCH 3/4] Fix popover/focus trap bug when clicking cell actions and then outside popover --- .../datagrid/body/data_grid_cell_popover.spec.tsx | 12 ++++++++++++ .../datagrid/body/data_grid_cell_popover.test.tsx | 1 + .../datagrid/body/data_grid_cell_popover.tsx | 2 +- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/components/datagrid/body/data_grid_cell_popover.spec.tsx b/src/components/datagrid/body/data_grid_cell_popover.spec.tsx index 03d0271ffd9..53e5e49982b 100644 --- a/src/components/datagrid/body/data_grid_cell_popover.spec.tsx +++ b/src/components/datagrid/body/data_grid_cell_popover.spec.tsx @@ -155,6 +155,18 @@ describe('EuiDataGridCellPopover', () => { cy.get('[data-test-subj="cellActionB"]').first().realClick(); cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should('exist'); + + // Clicking the cell actions outside the popover should not have disabled the focus trap + cy.repeatRealPress('Tab', 3); + cy.focused().should( + 'have.attr', + 'data-test-subj', + 'euiDataGridExpansionPopover' + ); + cy.get('body').realClick({ position: 'bottomRight' }); + cy.get('[data-test-subj="euiDataGridExpansionPopover"]').should( + 'not.exist' + ); }); it('allows consumers to use setCellPopoverProps, passed from renderCellPopover, to customize popover props', () => { diff --git a/src/components/datagrid/body/data_grid_cell_popover.test.tsx b/src/components/datagrid/body/data_grid_cell_popover.test.tsx index 2db3d3970fa..360b5e86157 100644 --- a/src/components/datagrid/body/data_grid_cell_popover.test.tsx +++ b/src/components/datagrid/body/data_grid_cell_popover.test.tsx @@ -101,6 +101,7 @@ describe('useCellPopover', () => { display="block" focusTrapProps={ Object { + "clickOutsideDisables": false, "onClickOutside": [Function], } } diff --git a/src/components/datagrid/body/data_grid_cell_popover.tsx b/src/components/datagrid/body/data_grid_cell_popover.tsx index c02f771fa5e..5deee06719d 100644 --- a/src/components/datagrid/body/data_grid_cell_popover.tsx +++ b/src/components/datagrid/body/data_grid_cell_popover.tsx @@ -107,7 +107,7 @@ export const useCellPopover = (): { anchorPosition={popoverAnchorPosition} repositionToCrossAxis={false} {...cellPopoverProps} - focusTrapProps={{ onClickOutside }} + focusTrapProps={{ onClickOutside, clickOutsideDisables: false }} panelProps={{ 'data-test-subj': 'euiDataGridExpansionPopover', ...(cellPopoverProps.panelProps || {}), From 49a1d124c30567569e93f882621e19eae9fc6593 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 23 Nov 2023 13:07:52 -0800 Subject: [PATCH 4/4] [misc UI/UX fix] Prevent cell animation flash for keyboard users - Escape key was causing a flash of animation if the mouse was not already hovered over the cell - this prevents it - DOM traversal is starting to feel pretty shaky, but not totally sure what to do about that --- src/components/datagrid/_data_grid_data_row.scss | 6 ++++-- .../datagrid/body/data_grid_cell_popover.tsx | 12 ++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/components/datagrid/_data_grid_data_row.scss b/src/components/datagrid/_data_grid_data_row.scss index f3287fd86f5..93ea8d1bcb5 100644 --- a/src/components/datagrid/_data_grid_data_row.scss +++ b/src/components/datagrid/_data_grid_data_row.scss @@ -57,7 +57,9 @@ &:focus, &:focus-within, // Always make the cell actions visible when the cell popover is open - &.euiDataGridRowCell--open { + &.euiDataGridRowCell--open, + // Prevents the animation from replaying when keyboard focus is moved from the popover back to the cell + &[data-keyboard-closing] { .euiDataGridRowCell__actions { animation-duration: $euiAnimSpeedExtraFast; animation-name: euiDataGridCellActionsSlideIn; @@ -67,7 +69,7 @@ } // if a cell is not hovered nor focused nor open via popover, don't show buttons in general - &:not(:hover):not(:focus):not(.euiDataGridRowCell--open) { + &:not(:hover):not(:focus):not(.euiDataGridRowCell--open):not([data-keyboard-closing]) { .euiDataGridRowCell__actions { display: none; } diff --git a/src/components/datagrid/body/data_grid_cell_popover.tsx b/src/components/datagrid/body/data_grid_cell_popover.tsx index 5deee06719d..66df03f09d4 100644 --- a/src/components/datagrid/body/data_grid_cell_popover.tsx +++ b/src/components/datagrid/body/data_grid_cell_popover.tsx @@ -128,8 +128,16 @@ export const useCellPopover = (): { event.preventDefault(); event.stopPropagation(); closeCellPopover(); - // Ensure focus is returned to the parent cell - requestAnimationFrame(() => popoverAnchor.parentElement!.focus()); + const cell = + popoverAnchor.parentElement?.parentElement?.parentElement; + + // Prevent cell animation flash while focus is being shifted between popover and cell + cell?.setAttribute('data-keyboard-closing', 'true'); + // Ensure focus is returned to the parent cell, and remove animation stopgap + requestAnimationFrame(() => { + popoverAnchor.parentElement!.focus(); + cell?.removeAttribute('data-keyboard-closing'); + }); } }} button={popoverAnchor}