EuiNavDrawer focus management, flyout auto-close

[thompsongl]

Summary

1. Closes EuiNavDrawerFlyout when a list item inside is clicked (ref: Grouped Kibana nav kibana#53545 (comment))
2. Enables a focus trap inside of EuiNavDrawerFlyout upon opening. This allows for tab order to immediately move to the subnav list items. ESC will close the flyout and return focus to the main list. (ref: Nav drawer a11y problems #2252)

Note that both solutions take less-than-ideal implementations. As is, the composition of EuiNavDrawer (by way of EuiListGroup) does not allow for nested list elements, and sub-lists do not exist in the DOM until explicit 'open' action(s) occur. This means that normal React ref behavior and ideal aria-* DOM attributes can't be used. The refactor to allow for both would be more akin to a full rewrite of several components, which, given the 7.6 FF date, we can't really approach.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

• Checked in IE11 and Firefox

- [ ] Props have proper autodocs
- [ ] Added documentation 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

[thompsongl]

Will revert this or make this not collide with new ESC behavior

[ryankeairns]

The refactor to allow for both would be more akin to a full rewrite of several components, which, given the 7.6 FF date, we can't really approach.

Adding to that point, it seems likely we will rewrite the nav drawer for K8.

[snide]

Functionality seems good for mouse / keyboard / trapping.

I'd suggest maybe doing a pass on the aria labels you have for the example. I can give you a gist of the groupings I had, which might make more sense, but i think we probably want labels on the top level menu items to describe themselves a such. As a basic example.

If no sub menu

"Go to SIEM app"

If does have sub menu

"List 4 Analytics apps"

I'm sure @myasonik can provide some better direction here. Likely it's something we just need to add to the examples, and not touch in the components themselves.

[myasonik]

For regular links, let's not touch them. They're a regular element that users will be familiar with and their assistive tech will expose.

For buttons that open a sub-menu, let's add aria-expanded="true|false" to them which should give screen reader users the hint that something's going to happen and we shouldn't have to rely on custom strings.

Going through the example, the focus trap right now isn't announced to screen readers at all. If we can, could we actually get rid of the focus trap? After tabbing from the last item, return focus to the button that opened it and close the sub-menu. (If that's not feasible in the time span we have left, we can steal some modal semantics from flyouts, I think. It won't be very pretty but it should work.)

And, to sneak in one more nice-to-have, could we add the heading of the sub-menus as an aria-labelledby={idOfTheHeading} to the ul that follows it?

[snide]

Going through the example, the focus trap right now isn't announced to screen readers at all. If we can, could we actually get rid of the focus trap? After tabbing from the last item, return focus to the button that opened it and close the sub-menu. (If that's not feasible in the time span we have left, we can steal some modal semantics from flyouts, I think. It won't be very pretty but it should work.)

A role="dialog" would likely fix this fairly easily, and makes the most sense I think? It would act like a popover.

[thompsongl]

A role="dialog" would likely fix this fairly easily, and makes the most sense I think? It would act like a popover.

I'm going to do my due diligence on tracking tab inside the flyout, but I think the likely solution will be adding dialog/role semantics, yes.

[thompsongl]

@snide This is ready for another look.

• I was able to get tab-through behavior to work
• I left in esc to close behavior
• I removed dialog semantics based on recommendation from @myasonik given the above items

[snide]

LGTM on merge of thompsongl#10

We noticed a small focus issue that @thompsongl is working through.

[thompsongl]

We noticed a small focus issue that @thompsongl is working through.

Fixed. Ready for you @myasonik

[myasonik]

Looks great!

I spent some time trying to break the focus order and couldn't so it seems good to merge!