eui

eui copied to clipboard

Howdy-ho EUI!

So as I believe you know, we're using Context menu out of, well, context. We use it as the content for our sidenav in docsmobile! In researching the second issue below, I think the idea is we don't want to set our initialFocusedItemIndex that I mention in the first issue AND ALSO we want the ability to say "please stop focusing so hard" to the context menu.

Our 2 issues

1. Aggressive initial focus.

It's the most obvious when there's only 1 level of menus so that ends up being the whole nav gets selected:

But we also see the focus ring (prior to using the keyboard) being highlighted at the top of the nav.

2. Scroll reset

This one is more of a hypothesis. When a nav is super long, and you select a page, when the page loads the nav scrolls back up to the top. The want is that it stays put so you can continue to see the context of your page in the nav bar. @spalger helped us investigate this one and we're both pretty confident the issue is tied to EUI and not next.js or something. My theory is that there's some sort of "scroll to the thing in focus" and since the focus ring is always on the top item, it is always scrolling up to the top.

Making it so the auto-focus can be turned off would definitely fix number 1 and my guess is that it would fix number two. Demoed this behavior to @chandlerprall so he also has context.

Making it so the auto-focus can be turned off would definitely fix number 1 and my guess is that it would fix number two.

This sounds right to me

TBH, I just really don't think Context Menu should be used in this manner. All you're attempting to utilize is the nesting & animation it provides which is easily done through custom implementation. I'd worry continuing to customize this component is going to make it hard for general consumers to understand when/why to use these props and could have negative impacts on the accessibility that we're trying to enforce.

Yeah, I definitely get that feeling too @cchaos, would it be possible to get help with a custom implementation that offers similar design but without using the context menu? I'd be happy to PR the docsmobile repo and take care of integration, but if someone from EUI could help me with the baseline style/composition of components I think it would save a ton of time.

Sounds good @spalger - there's should be quite a lot in context menu & related components that can be avoided altogether for the docsmobile implementation. From the code/logic side, @constancecchen has the most recent experience in the context menu components; for design/animation specifics @miukimiu can help get you sorted.

Re:

1. Aggressive initial focus.

If you're using <EuiContextMenuPanel /> directly, add initialFocusedItemIndex={-1} to the component to prevent initial focus from occurring.

If you're using <EuiContextMenu />, use this API instead:

<EuiContextMenu
  panels={[
    {
      // ... whatever panel
      initialFocusedItemIndex: -1,
    },
    {
      // some other panel
      initialFocusedItemIndex: -1, // basically add `initialFocusedItemIndex: -1` for every single panel
    },
  ]}
/>

This should fix both the focus and scrolling issues - feel free to ping me if not!

Thanks @constancecchen. I'm trying that but getting an _this2.state.menuItems[_this2.state.focusedItemIndex] is undefined thrown when navigating to the root of the context menu and still seeing the focus fix to the context menu (when in a sub-panel) button after a click in the content component.

Doh sorry, I'm still on vacation brain 🤦 It turns out I removed the -1 functionality in https://github.com/elastic/eui/pull/5783/commits/3b74d36f1dcf936593ef2535913e4ce3906a0cab / during my many recent EuiContextMenu focus fixes thinking it wouldn't be needed anymore. I'll take a look and either add it back in as a use case or ensure it doesn't throw.

Alrighty, so while I do have a restoration of the initialFocusedItemIndex={-1} behavior (https://github.com/constancecchen/eui/commit/e8b3cb340678b10b6e518f11fc2aa06f7555c2df / branch), after discussing this a bit more with Chandler and seeing https://github.com/elastic/docsmobile/pull/188 (which looks great btw!) I think that's a much better solution and the direction we should prefer to go with.

I agree with Caroline's previous comments that allowing users to disable the autofocus functionality is a little worrisome in that it by default strands keyboard users and their focus back to the top of the page unless consumers deliberately set a new focus, and is a potential accessibility pitfall. If we ever do strongly need it in the future, we can revisit, but in this scenario the switch to a superselect looks much better to me personally.

👋 Thanks all for the feedback and diligence.

The proposed design in https://github.com/elastic/docsmobile/pull/188#pullrequestreview-998954692 was mentioned and well supported in this thread.

We've chosen not to accept the PR to preserve the intended design.

As Caroline mentioned earlier, it looks like we're using Context Menu in an undesirable way.

As such, we'll try to reconsider a path forward that preserves the design intent and does not bend Context Menu.

Our next step is likely then a customized implementation.

We will keep you posted. :)

Based off Kellen's last comment, we're closing this on our end.