[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784

cee-chen · 2022-04-13T01:28:50Z

Summary

closes #5683
closes #4973

After extensive discussion between several designers/developers, we've decided to introduce a wide-spread breaking change on EuiPopover that removes the default behavior of focusing the first tabbable child element within the popover. Instead, the parent panel will receive a the initial focus.

Before:

After:

Why is this a better keyboard / screen reader user experience?

As noted in [EuiBasicTable] Actions popover should announce it is a popup #1546 (comment), the You are in a dialogue screen reader text gets interrupted/hijacked by focus moving from the panel to the first child. Removing this allows the You are in a dialogue text to read out fully (including keyboard instructions to close the dialogue).
As noted in [EuiDataGrid] Expansion popover initial focus #5683, since we do not control the content of EuiPopover, trying to auto target the first tabbable child may lead to unexpected placement of focus if there's a button or link very far down in the content. It's a more consistent user experience for screen readers to start them at the top and let them navigate through the popover content themselves.

What if a consumer has a very specific scenario where they want to auto-focus a child and know it makes sense?

This is still totally doable/customizable using the initialFocus prop. This PR simply changes the default behavior. I've added a new documentation example in this PR to make that option more evident.

What does this affect?

Many components, which we should likely do a QA pass on:

(Feel free to add more to this list if I missed anything, and check items off!)

Checklist

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Props have proper autodocs and playground toggles
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

Checked in Chrome, Safari, Edge, and Firefox
Added documentation
Added ~~or updated **jest and~~ cypress tests**
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

+ clean up ID generation

kibanamachine · 2022-04-13T02:06:00Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

- 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

src-docs/src/views/popover/trap_focus.js

cee-chen · 2022-04-13T02:52:25Z

src/components/context_menu/context_menu_panel.tsx

+  // 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() {


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 <EuiPopover initialFocus={false} /> in conjunction with <EuiContextMenu />. I mean we can, but I think they'll forget to do it, and I'd rather use a workaround to ensure keyboard users aren't SOL.

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!

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 😂

kibanamachine · 2022-04-13T03:17:50Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

kibanamachine · 2022-04-13T15:55:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

cee-chen · 2022-04-13T17:21:16Z

jenkins test this

kibanamachine · 2022-04-13T17:50:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

kibanamachine · 2022-04-13T20:22:30Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

1Copenut

I had one 👍 comment and one small text suggestion to change. No need for another review. LGTM!

1Copenut · 2022-04-14T19:16:17Z

src-docs/src/views/popover/trap_focus.js

+  const focusId = useGeneratedHtmlId();
+  useEffect(() => {
+    if (isPopoverOpen) {
+      document.getElementById(focusId).focus({ preventScroll: true });


👍 on the { preventScroll: true } object here. It's a good UX buffer.

upcoming_changelogs/5784.md

Co-authored-by: Trevor Pierce <[email protected]>

kibanamachine · 2022-04-14T21:08:37Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

kibanamachine · 2022-04-15T16:12:13Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

cee-chen · 2022-04-15T18:45:58Z

jenkins test this

kibanamachine · 2022-04-15T19:14:44Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5784/

cee-chen · 2022-04-15T19:45:39Z

I've QA'd all the components listed in the PR description and they're all working either the same as prod or as expected given these changes.

Does anyone have any thoughts/more feedback? Should I wait for @zuhairmahd to chime in?

1Copenut · 2022-04-15T20:31:33Z

@constancecchen If you're okay waiting to Monday to merge, that should give Zuhair a chance to weigh in. I feel we're right on point with this solution.

cee-chen · 2022-04-15T20:32:02Z

Absolutely!

zuhairmahd · 2022-04-15T20:35:52Z

@constancecchen I plan to take a look at it Monday morning my time if not sooner, so it would be great if you could wait til Monday before merging. I'm seven time zones ahead of eastern, so you should have my feedback by morning your time.

zuhairmahd · 2022-04-18T12:01:03Z

The behavior of the screen reader text being interrupted appears in my testing to happen only with VoiceOver. NVDA and JAWS exhibited different behaviour. Essentially, they read as expected when the focus was moved to the first element, and when it was moved to the parent, they read the entire contents of the popover.
Having said this, I do agree that, overall, this approach is better than setting the focus on the first item for the reasons you stated. I would suggest, however, adding context specific text (if possible) advising what the user needs to do to get to the first focusable element (like pressing the tab key, using the arrows etc.). If there are no focusable elements, then the text should naturally not be spoken. This should at least prompt the user to initiate action to interact with the popover, letting them know that they don't have to listen to the entire content.

cee-chen · 2022-04-19T15:25:59Z

Thanks Zuhair! If no objection I'll address screen reader keyboard instructions in a follow-up PR, to keep this one contained for easier debugging / identification if something goes awry in the next release/upgrade. I've made a detailed follow-up issue here: #5813

cee-chen added 5 commits April 12, 2022 17:31

Remove logic that focuses first tabbable child by default

0d313ba

Add Cypress focus tests

b952a72

Add changelog entry

098b6c3

Add an initialFocus documentation example + warning

80be81e

Fix non-working trap focus initialFocus example

a605c21

+ clean up ID generation

cee-chen added the breaking change label Apr 13, 2022

cee-chen commented Apr 13, 2022

View reviewed changes

Merge branch 'main' into popover-focus

69fc7c9

Update EuiDataGrid E2E tests to account for new popover focus changes

b2a960f

thompsongl requested a review from 1Copenut April 14, 2022 18:19

1Copenut approved these changes Apr 14, 2022

View reviewed changes

PR feedback: copy

ad9cf10

Co-authored-by: Trevor Pierce <[email protected]>

Merge branch 'main' into popover-focus

42fd49e

cee-chen mentioned this pull request Apr 19, 2022

Add keyboard usage instructions to screen reader popover dialogue #5813

Closed

cee-chen merged commit 5043660 into elastic:main Apr 19, 2022

cee-chen deleted the popover-focus branch April 19, 2022 15:27

thompsongl mentioned this pull request Apr 19, 2022

Upgrade EUI to v55.0.1 elastic/kibana#130593

Merged

thompsongl mentioned this pull request Apr 29, 2022

[EuiPopover and EuiComboBox] Has issues ownFocus #1454

Closed

cee-chen mentioned this pull request May 5, 2022

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for watchedItemProps #5880

Merged

5 tasks

This was referenced Jul 11, 2022

[EuiPopover and parts] Converted to Emotion #5977

Merged

[EuiPopover] Removed false as an option for initialFocus, removed unnecessary focus logic #6044

Merged

1Copenut mentioned this pull request Feb 10, 2023

Added SR-only help text to EuiPopover and dependent components. #6589

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784

[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784

cee-chen commented Apr 13, 2022 •

edited

Loading

kibanamachine commented Apr 13, 2022

cee-chen Apr 13, 2022

cee-chen Apr 13, 2022

kibanamachine commented Apr 13, 2022

kibanamachine commented Apr 13, 2022

cee-chen commented Apr 13, 2022

kibanamachine commented Apr 13, 2022

kibanamachine commented Apr 13, 2022

1Copenut left a comment

1Copenut Apr 14, 2022

kibanamachine commented Apr 14, 2022

kibanamachine commented Apr 15, 2022

cee-chen commented Apr 15, 2022

kibanamachine commented Apr 15, 2022

cee-chen commented Apr 15, 2022

1Copenut commented Apr 15, 2022

cee-chen commented Apr 15, 2022

zuhairmahd commented Apr 15, 2022 •

edited

Loading

zuhairmahd commented Apr 18, 2022

cee-chen commented Apr 19, 2022

[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784

[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784

Conversation

cee-chen commented Apr 13, 2022 • edited Loading

Summary

Why is this a better keyboard / screen reader user experience?

What if a consumer has a very specific scenario where they want to auto-focus a child and know it makes sense?

What does this affect?

Checklist

kibanamachine commented Apr 13, 2022

cee-chen Apr 13, 2022

Choose a reason for hiding this comment

cee-chen Apr 13, 2022

Choose a reason for hiding this comment

kibanamachine commented Apr 13, 2022

kibanamachine commented Apr 13, 2022

cee-chen commented Apr 13, 2022

kibanamachine commented Apr 13, 2022

kibanamachine commented Apr 13, 2022

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Apr 14, 2022

Choose a reason for hiding this comment

kibanamachine commented Apr 14, 2022

kibanamachine commented Apr 15, 2022

cee-chen commented Apr 15, 2022

kibanamachine commented Apr 15, 2022

cee-chen commented Apr 15, 2022

1Copenut commented Apr 15, 2022

cee-chen commented Apr 15, 2022

zuhairmahd commented Apr 15, 2022 • edited Loading

zuhairmahd commented Apr 18, 2022

cee-chen commented Apr 19, 2022

cee-chen commented Apr 13, 2022 •

edited

Loading

zuhairmahd commented Apr 15, 2022 •

edited

Loading