ni / nimble

The NI Nimble Design System
https://nimble.ni.dev
MIT License
32 stars 8 forks source link

nimble-anchor-menu-item getting outlined when the dropdown is opened #2028

Open Varshini-Periyasamy opened 6 months ago

Varshini-Periyasamy commented 6 months ago

🐛 Bug Report

The first item in the menu option is outlined when it is an anchor-menu-item image

💻 Repro or Code Sample

🤔 Expected Behavior

No items should be highlighted when opening the menu.

😯 Current Behavior

The first item is outlined.

💁 Possible Solution

🔦 Context

We added two anchor-items Dash boards and Data Spaces under the Visualize menu to visualize the data tables

🌍 Your Environment

mollykreis commented 6 months ago

The green outline in your screenshot indicates that the first item in the menu has keyboard focus. Could you elaborate on why you believe that's incorrect?

According to ARIA guidelines, the first item in a menu should receive focus when the menu is opened (though there are a few exceptions in which the last item should receive focus instead). It is up to the browser whether or not the focused element gets the focus-visible styling, so it is possible that you sometimes see the green ring and sometimes you don't. That is likely based on how the menu was open (keyboard vs mouse) in combination with what browser you are using. Therefore, I don't inherently see anything wrong with the behavior you are describing or the screenshot you've included.

Varshini-Periyasamy commented 6 months ago

I could see some inconsistency. For other existing menu's, the first item is focused but not outlined when opening the menu. For example, the Download menu in assets and the Analyse menu in files grid.

image image

To be precise, if the first item is nimble-anchor-menu-item, it is outlined with a green ring, and for nimble-menu-item it's not outlined. Also, it's worth noticing that the menu item is focused even on the disabled state. This should not be the case.

image

I can see this behavior in different browsers and when I open the menu using a keyboard or mouse. To make it consistent, we should either show or hide the outline for both menu-item and anchor-menu-item

You can see this behavior in datatables grid of the result details page under ff-visualization-options.

fredvisser commented 6 months ago

Hey @Varshini-Periyasamy -

The intended behavior when opening a menu-button via keyboard interactions is as follows:

  1. Tab to Download v button (so the double focus ring is around the menu-button)
  2. Press enter or space to toggle the menu open.
  3. The first menu item is focused (single focus ring) and the pressing enter or space will act on that item
  4. Pressing up or down arrows will move the focus state up and down the menu.
  5. Pressing ESC will close the menu.

https://github.com/ni/nimble/assets/1458528/877f8a57-c607-448f-85eb-c9edc5f28040

When navigating via mouse, the double focus ring should not be visible when clicking on the menu-button, and while the first item is focused when opening the menu, it doesn't have a visible focus state either.

https://github.com/ni/nimble/assets/1458528/b99be2aa-8fa6-420e-a9b3-2cbfdafc41f8

However, if you press the up or down arrows when the menu is open, you can move the menu focus, and you will get a focus ring on the selected menu item. This is expected.

https://github.com/ni/nimble/assets/1458528/6aff0527-e5e6-4441-b714-6dfc10ecaa73

In my testing, everything worked correctly - with the exception of the Systems - Asset tab - where you can't select the only item in the menu with enter or space.

https://github.com/ni/nimble/assets/1458528/45bdf1a1-500b-443b-a22f-795410e70e53

If I haven't missed anything else, I'll file that bug and close this one out - 👍

Varshini-Periyasamy commented 6 months ago

@fredvisser, thank you for the comprehensive information.

From what I understand, when navigating with a mouse, the double focus ring will not appear upon clicking the menu button. Additionally, although the first item is focused upon opening the menu, it lacks a visible focus state. and the focus ring will not be there on the focused item when the menu is opened. The focus ring only appears on the selected menu item when we use the up or down arrows after the menu is open.

We are encountering an issue where the first item in the Visualize menu displays a focus ring when the menu button is clicked with the mouse, even before using the up or down arrows. No additional styling has been applied to cause this from our end.

menu item outline bug

Please inform me if you are unable to replicate this issue.

Varshini-Periyasamy commented 6 months ago

We have added a temporary fix for this focus ring issue in this PR. You can see the issue by uncheck this additional style image

Please verify this behavior and post you thoughts here. If we need to remove this additional styling, we will create a new task to track it.

fredvisser commented 6 months ago

Filed Bug 2725182: Assets - Download menu-items are not keyboard accessible

kroeschl commented 4 months ago

I think this is broken again or still:

https://github.com/ni/nimble/assets/60117794/669237e7-ab2b-4a19-b2fc-dd3d0fa824df

I'm using nimble-angular 24.4.6, which is after the above fix was submitted. Should I open a new issue for this?

mollykreis commented 4 months ago

@kroeschl, I wasn't able to reproduce what you're seeing in the latest version of nimble. Could you provide more details, such as what browser you are using? Can you also check to see if you can get the issue to reproduce in our published storybook example of a menu button with an anchor menu item? https://nimble.ni.dev/storybook/?path=/story/components-menu-button--ghost-button.

kroeschl commented 4 months ago

I was able to reproduce the behavior using the storybook link using Chrome 127.0.6533.26 on Debian and Chrome 126.0.6478.127 on Windows, but not in Firefox on either OS. So, this may be a browser-specific issue.

mollykreis commented 4 months ago

What's also interesting is that I can't reproduce this in Edge either, so I'm wondering if a recent change in chromium is causing this behavior. I'll reopen the issue so our team can evaluate what to do with it.

m-akinc commented 4 months ago

We should make sure we are styling properly, but if the browser incorrectly says it should be "focus visible", we don't need to hack around it, but if it is behaving inconsistently across browsers we should file webcompat issues.

m-akinc commented 2 months ago

Chrome 128.0.6613.85: Image

Edge 128.0.2739.42 (Chromium 128.0.6613.85): Image

So apparently this is a Chrome (not Chromium) bug. I have filed a Web Compat bug. Closing this, since we don't intend to put in some jank workaround.

rajsite commented 2 months ago

The created webcompat GitHub issue: https://github.com/webcompat/web-bugs/issues/140917

I don't think historically we have closed issues where we have filed upstream bugs and instead have marked them blocked. Also think that giving a small reproducing case leads to a higher quality issue, in particular linking to the latest storybook build isn't a strong stability guarantee.

I'll re-open and mark team review for the following Qs @jattasNI @m-akinc: Do we want to keep this issue closed or mark it blocked so we can follow-up on the webcompat? Do we want to pre-emptively create a more self contained example?

m-akinc commented 2 months ago

Updated bug with simple codepen example

rajsite commented 2 months ago

Updated bug with simple codepen example

@m-akinc The concerns expressed were about size and pinning versions for reproducibility. The example is now well-scoped but does not pin versions. Also pinning versions on esm.sh via esm loading isn't stable (the transitive dependencies change).

Using the all-components-bundle is the most reliable since all deps are bundled and can even use the unminified bundle for nicer debugging. Based on the static webpage template and adding version numbers:

<!doctype html>
<html lang="en">
    <head>
        <meta charset="utf-8" />
        <meta name="viewport" content="width=device-width" />
        <title>Nimble Example</title>
        <link
            rel="stylesheet"
            href="https://unpkg.com/@ni/nimble-tokens@8.1.0/dist/fonts/css/fonts.css"
        />
        <script src="https://unpkg.com/@ni/nimble-components@32.0.0/dist/all-components-bundle.js"></script>
    </head>
    <body>
      <nimble-menu-button>
        Menu Button
        <nimble-menu slot="menu">
          <nimble-anchor-menu-item href="#">Anchor Menu Item</nimble-anchor-menu-item>
        </nimble-menu>
      </nimble-menu-button>
    </body>
</html>
m-akinc commented 2 months ago

Sorry, I was messing around with options to figure out the right way to pull in the package, and that esm.sh import was accidentally left over. The all components bundle is actually being pulled in via the JS settings: image I guess I'm still missing the fonts.css, but that's just cosmetic.

rajsite commented 2 months ago

Ah perfect, sounds good 👍