-
Notifications
You must be signed in to change notification settings - Fork 834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784
Changes from 6 commits
0d313ba
b952a72
098b6c3
80be81e
a605c21
4958d95
69fc7c9
b2a960f
ad9cf10
42fd49e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,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() { | ||
Comment on lines
+267
to
+271
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to add more context for this commit (4958d95), which attempts to resolve #4973: Unfortunately after testing both #5783 and #5784 together, focus still wasn't working as expected - EuiPopover was still taking focus away from EuiContextMenuPanel, and I don't think we can realistically ask every single consumer to always set I'm not a super huge fan of this DOM-heavy workaround but I think it's better than adding a hard-coded 350ms setTimeout to EuiContextMenuPanel 😬 Definitely open to other ideas/alternatives if folks can think of any! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also in hindisght I realize this comment is hilarious in light of #5760 (comment). EuiPopover and EuiContextMenuPanel, name a more iconic duo... for stealing focus from one another and then requiring separate workarounds 😂 |
||
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(); | ||
|
@@ -280,6 +309,7 @@ export class EuiContextMenuPanel extends Component<Props, State> { | |
|
||
componentDidMount() { | ||
this.updateFocus(); | ||
this.reclaimPopoverFocus(); | ||
this._isMounted = true; | ||
} | ||
|
||
|
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'); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
**Breaking changes** | ||
|
||
- `EuiPopover`s will no longer focus the first tabbable child by default - instead, the popover panel will be focused. This change should be a less better experience for both keyboard and screen reader users. Consumers who want to set an initial focus on specific popover element should use the `initialFocus` prop. | ||
cee-chen marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on the
{ preventScroll: true }
object here. It's a good UX buffer.