fix(navigation): improve focus style to meet WCAG 2.2 AA

dancormier commented 7 months ago

netlify[bot] commented 7 months ago

Deploy Preview for stacks ready!

Name	Link
Latest commit	698097ff119e2bcfb108683fe88414ba793c74cc
Latest deploy log	https://app.netlify.com/sites/stacks/deploys/65b91806098efe00083b2281
Deploy Preview	https://deploy-preview-1620--stacks.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dancormier commented 6 months ago

@CGuindon nice catch! If we use --white, the ring matches the background color in all cases. I've made the change in this PR, and I've removed the change to topbar for now so I can handle all of the topbar focus states at the same time.

-	Light	Dark
Default
HC

dancormier commented 6 months ago

This looks good. I understand navigation-item in the context of the topbar will be handled separately. I also found it interesting that the topbar override the focus ring behavior of navigation-item. Is there any reason we do that?
.s-topbar .s-navigation .s-navigation--item:focus-visible {
    box-shadow: var(--theme-topbar-search-shadow-focus, 0 0 0 var(--su-static4)) var(--focus-ring));
}
Could we rely only on the focus-visible style of the navigation-item in the topbar? Just trying to understand if we can delete extra code. 😀

Navigation within the topbar includes some custom theme that we'll need to figure out how to deal with. We'll have the following options:

1) Remove the custom styling altogether and just rely on the default styling 2) Keep the custom styling but apply it in a way that matches the navigation focus styling

There are other ways we could deal with it, but those are the two best options IMO. I originally included topbar modifications in this PR but changed my mind and figured we should deal with those when we focus on the topbar.

Related: I'm currently in the process of writing visual tests for topbar so we can more safely change the component. I've also opened a new branch to update the topbar to use PPCPs which should benefit from the baseline test images if we can get them merged in soon.

StackExchange / Stacks