EuiNavDrawer focus management, flyout auto-close#2749
EuiNavDrawer focus management, flyout auto-close#2749thompsongl merged 14 commits intoelastic:masterfrom
EuiNavDrawer focus management, flyout auto-close#2749Conversation
| this.closeFullScreen(); | ||
| // event.preventDefault(); | ||
| // event.stopPropagation(); | ||
| // this.closeFullScreen(); |
There was a problem hiding this comment.
Will revert this or make this not collide with new ESC behavior
Adding to that point, it seems likely we will rewrite the nav drawer for K8. |
snide
left a comment
There was a problem hiding this comment.
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.
|
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 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 |
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. |
There was a problem hiding this comment.
LGTM on merge of thompsongl#10
We noticed a small focus issue that @thompsongl is working through.
Docs example fixes
Fixed. Ready for you @myasonik |
myasonik
left a comment
There was a problem hiding this comment.
Looks great!
I spent some time trying to break the focus order and couldn't so it seems good to merge!
Summary
EuiNavDrawerFlyoutwhen a list item inside is clicked (ref: Grouped Kibana nav kibana#53545 (comment))EuiNavDrawerFlyoutupon 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 ofEuiListGroup) 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 Reactrefbehavior and idealaria-*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- [ ] Props have proper autodocs- [ ] Added documentation examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately