Shift focus to accordion content when expanded

[chandlerprall]

Summary

Closes #4224 by adding a tabIndex={-1} to the accordion's children wrapper and calling .focus() when it expands.

This is a draft because I added outline: 1px solid blue; to the focus state on .euiAccordion__childWrapper and would like help finding the right style to apply.

Checklist

• [ ] Check against all themes for compatibility in both light and dark modes
• Checked in mobile
• Checked in Chrome, Safari, Edge, and Firefox
  - [ ] Props have proper autodocs
  - [ ] Added documentation
  - [ ] Checked Code Sandbox works for the any docs examples
• Added or updated jest 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

[kibanamachine]

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

[myasonik]

We don't generally put a focus ring over non-interactive content so I'm ok to ship this as is and revisit the idea of a focus ring later. (Popovers and modals, as an example, shift focus to their container when they open but have no focus ring.)

The focus ring would certainly be a welcome addition but it would be good to keep it consistent with everything else too...

[cchaos]

🤔 So what happens if there is interactive content within the accordion content? Should it still be focusing the whole container or shift focus to the first focusable element. Perhaps controlled with a prop similar to the EuiPopover?

As for focus rings, no let's not add any forced focus rings to non-interactive elements. 😄

[chandlerprall]

🤔 So what happens if there is interactive content within the accordion content? Should it still be focusing the whole container or shift focus to the first focusable element. Perhaps controlled with a prop similar to the EuiPopover?

Michail answered this in the issue, #4224 (comment), saying it should always move to the container.

We don't generally put a focus ring over non-interactive content

As for focus rings, no let's not add any forced focus rings to non-interactive elements. 😄

I'll remove the focus ring 👍

[cchaos]

Michail answered this in the issue

Sounds good!

Next question, when VO is on, and the content gets expanded it just starts rattling off all the content that exists inside. Is there something we should do better here?

[kibanamachine]

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

[myasonik]

Next question, when VO is on, and the content gets expanded it just starts rattling off all the content that exists inside. Is there something we should do better here?

It's not really a problem (and that's where I was going to leave it at) but your question did make do some more digging and we can do better!

TL;DR if we add a role="region" and aria-labelledby={idOfTheTrigger} it will prevent some screen reader combos from, to minimal benefit, reading out the whole of the content with no structure.

But what's the full story?

First, to be upfront, moving focus actually isn't the "default" recommendation. Often for this pattern, you'll leave focus on the button. But our auditor recommended that we do it and I agreed largely because of the amount of stuff that's on our typical pages and because this is such a generic component. This means that we don't know what sort of stuff is going to be thrown in the button trigger and we don't know in what context this will show up in. So by moving focus we're able to be super explicit to our end user in terms of "here is the thing that happened."

So, with that all as a preamble, we're looking at a component that has some similarities to popovers/modals/flyouts/etc. Looking at those as another situation where we move focus around, adding the region role and an accessible name (via aria-labelledby in this case) mimics a similar structure to those as well as mimicking their (usually present) heading.

[kibanamachine]

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

[kibanamachine]

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

[chandlerprall]

network flake - jenkins test this

[kibanamachine]

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

[chandlerprall]

There's a weird focus issue happening that may or may not be amsterdam specific, where sometimes the content receives a focus ring. Looking into this

[chandlerprall]

Potential solutions for the amsterdam theme + focus ring + accordion interaction, I don't have any preference and will implement whatever @cchaos and @myasonik say

• ignore this, if the browser decides to apply :focus-visible rules then maybe the focus ring is the right thing?
• force outline to a border-width of 0 on the accordion
• force outline to exist in amsterdam (maybe only enable it when keyboard is used to expand the accordion?)

[cchaos]

So to clarify on the interaction being specified, it's when the accordion being opened is triggered by pressing space/enter. Then the focus shifts to the accordion content and the focus ring is visible. This is actually working as intended because the interaction was triggered by a keyboard interaction and not the mouse. So when just using the mouse to open/close the accordion, you don't see the focus ring. 👍

[myasonik]

Ship it as is and we can always revisit if something starts to feel off about this.

There aren't any consumer changes to this so it should be pretty low risk to iterate on.