MenuDivider does not handle escape key to close popover

[adidahiya]

Environment

• Package version(s): core v4.17.8, popover2 v1.13.12

Code Sandbox

https://codesandbox.io/p/sandbox/recursing-wave-ldgezp

Steps to reproduce

1. Render a Menu with MenuItem2 and MenuDivider items inside a popover
2. Make the MenuItem2 components inactive (disabled={true} or shouldDismissPopover={true})
3. Open the popover
4. Click on a MenuItem2, then press the escape key
5. Open the popover
6. Click on a MenuDivider, then press the escape key

Actual behavior

Popover closed after step 4
Popover stayed open after step 6

Expected behavior

Popover should close after step 4, and also after step 6

Possible solution

Seems related to #5947, where the suggestion of adding a tabIndex={0} to an element which should be focusable (and therefore handle the "escape key to close overlay" behavior) seems to work. Should be an easy fix to MenuDivider.tsx, probably just need to add tabIndex={0} here:

blueprint/packages/core/src/components/menu/menuDivider.tsx

Line 54 in 6d86a19

<li className={classNames(Classes.MENU_HEADER, className)} role="separator">

[bvandercar-vt]

Should be an easy fix to MenuDivider.tsx, probably just need to add tabIndex={0} here

tabIndex > -1 should only be applied to interactive elements, which the divider and its text are not. Definitely should not be tabb-able.

A better solution that accomplishes what you want would be to add onKeyDown={(event) => event.key === 'Escape' && closePopover()}

[adidahiya]

@bvandercar-vt that's a good point, I don't think we should use tabIndex={0}.

But the alternative solution gets tricky when you want to implement closePopover() as you suggested. We'd have to either:

• create new custom DOM event semantics, like a Blueprint-specific "popover close" event which is bubbled up through the DOM
• or create a new Popover2Context (this would require creating MenuDivider2 which knows about Popover2) which allows open / close context "actions" on the parent Popover2

Both of those require a bit of work which I'm not sure is worth the cost for this small feature.

I think a reasonable compromise would be to use tabIndex={-1}, which still solves the problem:

As long as this doesn't trigger any strict a11y validation failures, I'd prefer this simple approach.

[bvandercar-vt]

@adidahiya yep if tabIndex={-1} solves the problem, I'm not sure how, but sounds good!

[bvandercar-vt]

As long as this doesn't trigger any strict a11y validation failures, I'd prefer this simple approach.

Continuing discussion in #6457 -- it turns out this did cause a11y aXe failure.