diff --git a/src/components/context_menu/context_menu_panel.spec.tsx b/src/components/context_menu/context_menu_panel.spec.tsx index d6e61118ec8..8e85784792e 100644 --- a/src/components/context_menu/context_menu_panel.spec.tsx +++ b/src/components/context_menu/context_menu_panel.spec.tsx @@ -187,7 +187,7 @@ describe('EuiContextMenuPanel', () => { const mountAndOpenPopover = (component = ) => { cy.realMount(component); cy.get('[data-test-subj="popoverToggle"]').click(); - cy.wait(350); // EuiPopover's updateFocus() takes ~350ms to run + cy.wait(350); // EuiPopover's initial/autoFocus takes ~350ms to run }; it('reclaims focus from the parent popover panel', () => { diff --git a/src/components/context_menu/context_menu_panel.tsx b/src/components/context_menu/context_menu_panel.tsx index 8691238fb4d..97f77e5b612 100644 --- a/src/components/context_menu/context_menu_panel.tsx +++ b/src/components/context_menu/context_menu_panel.tsx @@ -316,7 +316,7 @@ export class EuiContextMenuPanel extends Component { // If EuiContextMenu is used within an EuiPopover, we need to wait for EuiPopover to: // 1. Correctly set its `returnFocus` to the toggling button, // so focus is correctly restored to the popover toggle on close - // 2. Finish its own `updateFocus()` call 350ms after transitioning in, + // 2. Finish its react-focus-on `autoFocus` behavior after transitioning in, // so the panel can handle its own focus without focus fighting if (this.initialPopoverParent) { this.initialPopoverParent.addEventListener( diff --git a/src/components/focus_trap/focus_trap.tsx b/src/components/focus_trap/focus_trap.tsx index b6403ff39de..6619c90ac6f 100644 --- a/src/components/focus_trap/focus_trap.tsx +++ b/src/components/focus_trap/focus_trap.tsx @@ -75,6 +75,7 @@ export class EuiFocusTrap extends Component { // Programmatically sets focus on a nested DOM node; optional setInitialFocus = (initialFocus?: FocusTarget) => { + if (!initialFocus) return; const node = findElementBySelectorOrRef(initialFocus); if (!node) return; // `data-autofocus` is part of the 'react-focus-on' API diff --git a/src/components/popover/popover.spec.tsx b/src/components/popover/popover.spec.tsx index 73f30c2140d..4f8851ec654 100644 --- a/src/components/popover/popover.spec.tsx +++ b/src/components/popover/popover.spec.tsx @@ -43,24 +43,22 @@ describe('EuiPopover', () => { it('focuses the panel wrapper by default', () => { cy.mount(Test); cy.get('[data-test-subj="togglePopover"]').click(); - cy.focused().should('have.attr', 'data-test-subj', 'popoverPanel'); + cy.focused() + .should('have.attr', 'data-test-subj', 'popoverPanel') + .should('have.attr', 'data-autofocus', 'true'); // set by react-focus-on on the initially focused node }); it('does not focus anything if `ownFocus` is false', () => { cy.mount(Test); cy.get('[data-test-subj="togglePopover"]').click(); cy.focused().should('have.attr', 'data-test-subj', 'togglePopover'); + cy.get('[data-test-subj="popoverPanel"]').should( + 'not.have.attr', + 'data-autofocus' + ); }); describe('initialFocus', () => { - it('does not focus anything if `initialFocus` is false', () => { - cy.mount( - Test - ); - cy.get('[data-test-subj="togglePopover"]').click(); - cy.focused().should('have.attr', 'data-test-subj', 'togglePopover'); - }); - it('focuses selector strings', () => { cy.mount( @@ -68,7 +66,13 @@ describe('EuiPopover', () => { ); cy.get('[data-test-subj="togglePopover"]').click(); - cy.focused().should('have.attr', 'id', 'test'); + cy.focused() + .should('have.attr', 'id', 'test') + .should('have.attr', 'data-autofocus', 'true'); + cy.get('[data-test-subj="popoverPanel"]').should( + 'not.have.attr', + 'data-autofocus' + ); }); it('focuses functions returning DOM Nodes', () => { @@ -80,7 +84,13 @@ describe('EuiPopover', () => { ); cy.get('[data-test-subj="togglePopover"]').click(); - cy.focused().should('have.attr', 'id', 'test'); + cy.focused() + .should('have.attr', 'id', 'test') + .should('have.attr', 'data-autofocus', 'true'); + cy.get('[data-test-subj="popoverPanel"]').should( + 'not.have.attr', + 'data-autofocus' + ); }); }); }); diff --git a/src/components/popover/popover.test.tsx b/src/components/popover/popover.test.tsx index f49150f6218..d485b96cac2 100644 --- a/src/components/popover/popover.test.tsx +++ b/src/components/popover/popover.test.tsx @@ -450,13 +450,9 @@ describe('EuiPopover', () => { expect(rafSpy).toHaveBeenCalledTimes(1); expect(activeAnimationFrames.size).toEqual(1); - jest.advanceTimersByTime(10); - expect(rafSpy).toHaveBeenCalledTimes(2); - expect(activeAnimationFrames.size).toEqual(2); - component.unmount(); - expect(window.clearTimeout).toHaveBeenCalledTimes(10); - expect(cafSpy).toHaveBeenCalledTimes(2); + expect(window.clearTimeout).toHaveBeenCalledTimes(9); + expect(cafSpy).toHaveBeenCalledTimes(1); expect(activeAnimationFrames.size).toEqual(0); // EUI's jest configuration throws an error if there are any console.error calls, like diff --git a/src/components/popover/popover.tsx b/src/components/popover/popover.tsx index 9a2f642fb35..e5944678772 100644 --- a/src/components/popover/popover.tsx +++ b/src/components/popover/popover.tsx @@ -111,11 +111,11 @@ export interface EuiPopoverProps { * Specifies what element should initially have focus; Can be a DOM * node, or a selector string (which will be passed to * document.querySelector() to find the DOM node), or a function that - * returns a DOM node - * Set to `false` to prevent initial auto-focus. Use only - * when your app handles setting initial focus state. + * returns a DOM node. + * + * If not passed, initial focus defaults to the popover panel. */ - initialFocus?: FocusTarget | false; + initialFocus?: FocusTarget; /** * Passed directly to EuiPortal for DOM positioning. Both properties are * required if prop is specified @@ -271,22 +271,6 @@ const DEFAULT_POPOVER_STYLES = { left: 50, }; -function getElementFromInitialFocus( - initialFocus?: FocusTarget -): HTMLElement | null { - const initialFocusType = typeof initialFocus; - - if (initialFocusType === 'string') { - return document.querySelector(initialFocus as string); - } - - if (initialFocusType === 'function') { - return (initialFocus as () => HTMLElement | null)(); - } - - return initialFocus as HTMLElement | null; -} - const returnFocusConfig = { preventScroll: true }; const closingTransitionTime = 250; // TODO: DRY out var when converting to CSS-in-JS @@ -357,10 +341,8 @@ export class EuiPopover extends Component { private strandedFocusTimeout: number | undefined; private closingTransitionTimeout: number | undefined; private closingTransitionAnimationFrame: number | undefined; - private updateFocusAnimationFrame: number | undefined; private button: HTMLElement | null = null; private panel: HTMLElement | null = null; - private hasSetInitialFocus: boolean = false; private descriptionId: string = htmlIdGenerator()(); constructor(props: Props) { @@ -426,63 +408,6 @@ export class EuiPopover extends Component { } }; - updateFocus() { - // Wait for the DOM to update. - this.updateFocusAnimationFrame = window.requestAnimationFrame(() => { - if ( - !this.props.ownFocus || - !this.panel || - this.props.initialFocus === false - ) { - return; - } - - // If we've already focused on something inside the panel, everything's fine. - if ( - this.hasSetInitialFocus && - this.panel.contains(document.activeElement) - ) { - return; - } - - // Otherwise focus either `initialFocus` or the panel - let focusTarget; - - if (this.props.initialFocus != null) { - focusTarget = getElementFromInitialFocus(this.props.initialFocus); - } - - // there's a race condition between the popover content becoming visible and this function call - // if the element isn't visible yet (due to css styling) then it can't accept focus - // so wait for another render and try again - if (focusTarget == null) { - // there isn't a focus target, one of two reasons: - // #1 is the whole panel hidden? If so, schedule another check - // #2 panel is visible and no `initialFocus` was set, move focus to the panel - const panelVisibility = window.getComputedStyle(this.panel).opacity; - if (panelVisibility === '0') { - // #1 - this.updateFocus(); - } else { - // #2 - focusTarget = this.panel; - } - } else { - // found an element to focus, but is it visible? - const visibility = window.getComputedStyle(focusTarget).visibility; - if (visibility === 'hidden') { - // not visible, check again next render frame - this.updateFocus(); - } - } - - if (focusTarget != null) { - this.hasSetInitialFocus = true; - focusTarget.focus(); - } - }); - } - onOpenPopover = () => { clearTimeout(this.strandedFocusTimeout); clearTimeout(this.closingTransitionTimeout); @@ -519,7 +444,6 @@ export class EuiPopover extends Component { this.respositionTimeout = window.setTimeout(() => { this.setState({ isOpenStable: true }, () => { this.positionPopoverFixed(); - this.updateFocus(); }); }, durationMatch + delayMatch); }; @@ -559,7 +483,6 @@ export class EuiPopover extends Component { // If the user has just closed the popover, queue up the removal of the content after the // transition is complete. this.closingTransitionTimeout = window.setTimeout(() => { - this.hasSetInitialFocus = false; this.setState({ isClosing: false, }); @@ -573,7 +496,6 @@ export class EuiPopover extends Component { clearTimeout(this.strandedFocusTimeout); clearTimeout(this.closingTransitionTimeout); cancelAnimationFrame(this.closingTransitionAnimationFrame!); - cancelAnimationFrame(this.updateFocusAnimationFrame!); } onMutation = (records: MutationRecord[]) => { @@ -712,7 +634,6 @@ export class EuiPopover extends Component { arrowChildren, repositionOnScroll, zIndex, - initialFocus, attachToAnchor, display, offset, @@ -722,6 +643,7 @@ export class EuiPopover extends Component { 'aria-labelledby': ariaLabelledBy, container, focusTrapProps, + initialFocus: initialFocusProp, tabIndex: tabIndexProp, ...rest } = this.props; @@ -752,7 +674,7 @@ export class EuiPopover extends Component { if (!this.state.suppressingPopover && (isOpen || this.state.isClosing)) { let tabIndex = tabIndexProp; - let initialFocus; + let initialFocus = initialFocusProp; let ariaDescribedby; let ariaLive: HTMLAttributes['aria-live']; @@ -766,8 +688,9 @@ export class EuiPopover extends Component { if (ownFocus || panelAriaModal !== 'true') { tabIndex = tabIndexProp ?? 0; ariaLive = 'off'; - - initialFocus = () => this.panel!; + if (!initialFocus) { + initialFocus = () => this.panel!; + } } else { ariaLive = 'assertive'; } @@ -854,7 +777,7 @@ export class EuiPopover extends Component { ); } - // react-focus-on and relataed do not register outside click detection + // react-focus-on and related do not register outside click detection // when disabled, so we still need to conditionally check for that ourselves if (ownFocus) { return ( diff --git a/upcoming_changelogs/6044.md b/upcoming_changelogs/6044.md new file mode 100644 index 00000000000..9e13bf846e5 --- /dev/null +++ b/upcoming_changelogs/6044.md @@ -0,0 +1,3 @@ +**Breaking changes** + +- `EuiPopover`: Removed `false` as an option from `initialFocus`