ni / nimble

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

Anchor menu item focus styling fix #2050

Closed mollykreis closed 5 months ago

mollykreis commented 5 months ago

Pull Request

🀨 Rationale

Fixes #2028

πŸ‘©β€πŸ’» Implementation

The root of the issue is that the focus outline was being shown any time focus was within the anchor menu item rather than being styled based on focus-visible. The difficulty is that the anchor menu item delegates focus to the inner anchor element, which means the anchor becomes focus-visible but the anchor menu item does not. Therefore, I reworked the styling a little bit so that the anchor element is the same size as the anchor menu item to allow the anchor to have the outline. A beneficial side effect of this change is that the click area of the anchor now exactly matches the hover & focus area of the menu item.

πŸ§ͺ Testing

βœ… Checklist

fredvisser commented 5 months ago

In my SLE testing, I thought the keyboard focus ring on menu-items was consistent, on second look the keyboard focus takes a stop between menu items, and some of the items don't end up getting focus rings. Is this a separate issue?

I.e.

  1. Click to open a menu-button
  2. Press down arrow - the second menu-item should have a focus ring
  3. Press down arrow - the third menu-item should have a focus ring
  4. Press up arrow 2x - the first menu-item should have a focus ring

(All regardless of the menu-item disabled state)

https://github.com/ni/nimble/assets/1458528/80358148-3eaf-4b54-ad01-fdcba4f40a9e

fredvisser commented 5 months ago

@mollykreis - Yep, it's a Safari specific issue… and not related to your change. I'll create a new bug to track it.

mollykreis commented 5 months ago

@m-akinc, will you buddy this PR for me?