Skip to content

Commit

Permalink
[EuiPopover] Focus on parent popover panel by default, instead of fir…
Browse files Browse the repository at this point in the history
…st tabbable child (#5784)

* Remove logic that focuses first tabbable child by default

* Add Cypress focus tests

* Add changelog entry

* Add an `initialFocus` documentation example + warning

* Fix non-working trap focus `initialFocus` example

+ clean up ID generation

* Fix EuiPopover continuing to hijack EuiContextMenu focus

- because EuiPopover's `updateFocus` is called after the transition ends (350ms currently), it supercedes EuiContextMenu's `updateFocus`

- I'm not a super huge fan of this DOM-heavy workaround but I can't think of any other options - very open to more ideas

* Update EuiDataGrid E2E tests to account for new popover focus changes

* PR feedback: copy

Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>

Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>
  • Loading branch information
Constance and 1Copenut authored Apr 19, 2022
1 parent e01a8eb commit 5043660
Show file tree
Hide file tree
Showing 9 changed files with 248 additions and 30 deletions.
40 changes: 40 additions & 0 deletions src-docs/src/views/popover/initial_focus.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import React, { useState } from 'react';

import {
EuiButton,
EuiFormRow,
EuiPopover,
EuiSpacer,
EuiFieldText,
} from '../../../../src/components';

export default () => {
const [isPopoverOpen, setIsPopoverOpen] = useState(false);

const onButtonClick = () =>
setIsPopoverOpen((isPopoverOpen) => !isPopoverOpen);
const closePopover = () => setIsPopoverOpen(false);

const button = (
<EuiButton iconType="arrowDown" iconSide="right" onClick={onButtonClick}>
Show popover
</EuiButton>
);

return (
<EuiPopover
initialFocus="#name"
button={button}
isOpen={isPopoverOpen}
closePopover={closePopover}
>
<EuiFormRow label="Enter name" id="name">
<EuiFieldText compressed name="input" />
</EuiFormRow>

<EuiSpacer />

<EuiButton fill>Submit</EuiButton>
</EuiPopover>
);
};
45 changes: 45 additions & 0 deletions src-docs/src/views/popover/popover_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import {
import Popover from './popover';
const popoverSource = require('!!raw-loader!./popover');

import InitialFocus from './initial_focus';
const initialFocusSource = require('!!raw-loader!./initial_focus');

import TrapFocus from './trap_focus';
const trapFocusSource = require('!!raw-loader!./trap_focus');

Expand Down Expand Up @@ -60,6 +63,14 @@ const trapFocusSnippet = `<EuiPopover
<!-- Popover content -->
</EuiPopover>`;

const initialFocusSnippet = `<EuiPopover
initialFocus=".someSelector"
button={button}
isOpen={isPopoverOpen}
closePopover={closePopover}>
<!-- Popover content -->
</EuiPopover>`;

const popoverAnchorSnippet = `<EuiPopover
button={button}
isOpen={isPopoverOpen}
Expand Down Expand Up @@ -356,6 +367,40 @@ export const PopoverExample = {
snippet: inputPopoverSnippet,
demo: <InputPopover />,
},
{
title: 'Setting an initial focus',
source: [
{
type: GuideSectionTypes.JS,
code: initialFocusSource,
},
],
text: (
<>
<p>
If you want a specific child element of the popover to immediately
gain focus when the popover is open, use the{' '}
<EuiCode language="ts">initialFocus</EuiCode> prop to pass either a
selector or DOM node.
</p>
<EuiCallOut
iconType="accessibility"
color="warning"
title={
<>
It can be jarring for keyboard and screen reader users to
immediately land on an element with no other context. To
alleviate this, ensure that your initial focus target makes
sense alone or is the primary goal of the popover.
</>
}
/>
</>
),
props: { EuiPopover },
snippet: initialFocusSnippet,
demo: <InitialFocus />,
},
{
title: 'Removing the focus trap',
source: [
Expand Down
27 changes: 11 additions & 16 deletions src-docs/src/views/popover/trap_focus.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState } from 'react';
import React, { useState, useEffect } from 'react';

import {
EuiButton,
Expand All @@ -13,15 +13,6 @@ import { useGeneratedHtmlId } from '../../../../src/services';
export default () => {
const [isPopoverOpen, setIsPopoverOpen] = useState(false);

const trapFocusFormRowId__1 = useGeneratedHtmlId({
prefix: 'trapFocusFormRow',
suffix: 'first',
});
const trapFocusFormRowId__2 = useGeneratedHtmlId({
prefix: 'trapFocusFormRow',
suffix: 'second',
});

const onButtonClick = () =>
setIsPopoverOpen((isPopoverOpen) => !isPopoverOpen);
const closePopover = () => setIsPopoverOpen(false);
Expand All @@ -32,17 +23,24 @@ export default () => {
</EuiButton>
);

// Since `hasFocus={false}` disables popover auto focus, we need to manually set it ourselves
const focusId = useGeneratedHtmlId();
useEffect(() => {
if (isPopoverOpen) {
document.getElementById(focusId).focus({ preventScroll: true });
}
}, [isPopoverOpen, focusId]);

return (
<EuiPopover
ownFocus={false}
button={button}
isOpen={isPopoverOpen}
closePopover={closePopover}
initialFocus={`[id=${trapFocusFormRowId__1}]`}
>
<EuiFormRow
label="Generate a public snapshot?"
id={trapFocusFormRowId__1}
id={focusId}
hasChildLabel={false}
>
<EuiSwitch
Expand All @@ -53,10 +51,7 @@ export default () => {
/>
</EuiFormRow>

<EuiFormRow
label="Include the following in the embed"
id={trapFocusFormRowId__2}
>
<EuiFormRow label="Include the following in the embed">
<EuiSwitch
name="switch"
label="Current time range"
Expand Down
14 changes: 14 additions & 0 deletions src/components/context_menu/context_menu_panel.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import React from 'react';

import { EuiPopover } from '../popover';
import { EuiContextMenu } from './context_menu';
import { EuiContextMenuItem } from './context_menu_item';
import { EuiContextMenuPanel } from './context_menu_panel';
Expand Down Expand Up @@ -121,6 +122,19 @@ describe('EuiContextMenuPanel', () => {
cy.focused().should('have.attr', 'data-test-subj', 'panelA');
});
});

describe('when inside an EuiPopover', () => {
it('reclaims focus from the parent popover panel', () => {
cy.mount(
<EuiPopover isOpen={true} button={<button />}>
<EuiContextMenuPanel items={items} />
</EuiPopover>
);
cy.wait(400); // EuiPopover's updateFocus() takes ~350ms to run
cy.focused().should('not.have.attr', 'class', 'euiPopover__panel');
cy.focused().should('have.attr', 'class', 'euiContextMenuPanel');
});
});
});

describe('Keyboard navigation of items', () => {
Expand Down
30 changes: 30 additions & 0 deletions src/components/context_menu/context_menu_panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,35 @@ export class EuiContextMenuPanel extends Component<Props, State> {
});
}

// If EuiContextMenu is used within an EuiPopover, EuiPopover's own
// `updateFocus()` method hijacks EuiContextMenuPanel's `updateFocus()`
// 350ms after the popover finishes transitioning in. This workaround
// reclaims focus from parent EuiPopovers that do not set an `initialFocus`
reclaimPopoverFocus() {
if (!this.panel) return;

const parent = this.panel.parentNode as HTMLElement;
if (!parent) return;
const hasEuiContextMenuParent = parent.classList.contains('euiContextMenu');

// It's possible to use an EuiContextMenuPanel directly in a popover without
// an EuiContextMenu, so we need to account for that when searching parent nodes
const popoverParent = hasEuiContextMenuParent
? (parent?.parentNode?.parentNode as HTMLElement)
: (parent?.parentNode as HTMLElement);
if (!popoverParent) return;

const hasPopoverParent = popoverParent.classList.contains(
'euiPopover__panel'
);
if (!hasPopoverParent) return;

// If the popover panel gains focus, switch it to the context menu panel instead
popoverParent.addEventListener('focus', () => {
this.updateFocus();
});
}

onTransitionComplete = () => {
if (this.props.onTransitionComplete) {
this.props.onTransitionComplete();
Expand All @@ -272,6 +301,7 @@ export class EuiContextMenuPanel extends Component<Props, State> {

componentDidMount() {
this.updateFocus();
this.reclaimPopoverFocus();
this._isMounted = true;
}

Expand Down
21 changes: 15 additions & 6 deletions src/components/datagrid/data_grid.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -366,12 +366,17 @@ describe('EuiDataGrid', () => {
cy.focused().type('{enter}');
cy.focused().should('have.attr', 'data-test-subj', 'focusOnMe');

// fourth cell is non-expandable with multiple interactives, click should focus on the cell
// fourth cell is expandable & interactive, click should focus on the popover
cy.get(
'[data-gridcell-column-index="3"][data-gridcell-row-index="0"]'
).click();
cy.focused().type('{enter}');
cy.focused().should('have.attr', 'data-test-subj', 'focusOnMe'); // focus trap focuses the link
// focus trap focuses the popover
cy.focused().should(
'have.attr',
'data-test-subj',
'euiDataGridExpansionPopover'
);
cy.focused().type('{esc}');
cy.focused()
.should('have.attr', 'data-gridcell-column-index', '3')
Expand Down Expand Up @@ -404,10 +409,14 @@ describe('EuiDataGrid', () => {
.should('have.attr', 'data-gridcell-column-index', '5')
.should('have.attr', 'data-gridcell-row-index', '0');
cy.focused().type('{enter}'); // trigger expansion popover
cy.focused().should('have.attr', 'data-test-subj', 'btn-yes'); // focus trap should move focus to the first button
cy.focused().parentsUntil(
'[data-test-subj="euiDataGridExpansionPopover"]'
); // ensure focus is in the popover
// focus trap focuses the popover
cy.focused().should(
'have.attr',
'data-test-subj',
'euiDataGridExpansionPopover'
);
cy.realPress('Tab');
cy.focused().should('have.attr', 'data-test-subj', 'btn-yes');
cy.realPress('Tab');
cy.focused().should('have.attr', 'data-test-subj', 'btn-no');
cy.realPress('Tab');
Expand Down
87 changes: 87 additions & 0 deletions src/components/popover/popover.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/// <reference types="../../../cypress/support"/>

import React, { useState } from 'react';

import { EuiButton } from '../button';
import { EuiPopover } from './popover';

const PopoverComponent = ({ children, ...rest }) => {
const [isPopoverOpen, setIsPopoverOpen] = useState(false);
const closePopover = () => setIsPopoverOpen(false);
const togglePopover = () =>
setIsPopoverOpen((isPopoverOpen) => !isPopoverOpen);

const button = (
<EuiButton onClick={togglePopover} data-test-subj="togglePopover">
Show popover
</EuiButton>
);

return (
<EuiPopover
panelProps={{ 'data-test-subj': 'popoverPanel' }}
button={button}
isOpen={isPopoverOpen}
closePopover={closePopover}
{...rest}
>
{children}
</EuiPopover>
);
};

describe('EuiPopover', () => {
describe('focus behavior', () => {
it('focuses the panel wrapper by default', () => {
cy.mount(<PopoverComponent>Test</PopoverComponent>);
cy.get('[data-test-subj="togglePopover"]').click();
cy.focused().should('have.attr', 'data-test-subj', 'popoverPanel');
});

it('does not focus anything if `ownFocus` is false', () => {
cy.mount(<PopoverComponent ownFocus={false}>Test</PopoverComponent>);
cy.get('[data-test-subj="togglePopover"]').click();
cy.focused().should('have.attr', 'data-test-subj', 'togglePopover');
});

describe('initialFocus', () => {
it('does not focus anything if `initialFocus` is false', () => {
cy.mount(
<PopoverComponent initialFocus={false}>Test</PopoverComponent>
);
cy.get('[data-test-subj="togglePopover"]').click();
cy.focused().should('have.attr', 'data-test-subj', 'togglePopover');
});

it('focuses selector strings', () => {
cy.mount(
<PopoverComponent initialFocus="#test">
<button id="test">Test</button>
</PopoverComponent>
);
cy.get('[data-test-subj="togglePopover"]').click();
cy.focused().should('have.attr', 'id', 'test');
});

it('focuses functions returning DOM Nodes', () => {
cy.mount(
<PopoverComponent
initialFocus={() => document.getElementById('test')}
>
<button id="test">Test</button>
</PopoverComponent>
);
cy.get('[data-test-subj="togglePopover"]').click();
cy.focused().should('have.attr', 'id', 'test');
});
});
});
});
Loading

0 comments on commit 5043660

Please sign in to comment.