feat(components): use new focus styles for interactive elements

[Francois-Esquire]

Description

This PR introduces new focus ring styling using the focusStyles utility.

Detail

Interactive components throughout the component library have been updated with the new focus ring.

Notable changes

sweeping changes and additions

• transition for focus ring has been added to all affected components

focus with outline vs box-shadow

While most components use the conventional box-shadow focus ring, there are some exceptions:

• Anchor: outline was used to better visualize wrapping anchor text when encountering a line break.
• NavItem: outline worked with the transparent styling within the component

focusing within

We want to move components away from the isFocused prop in favor of modern CSS focus selectors (eg :focus-within) where possible. The Tiles.Tile component was updated with that in mind. This was made possible with the :focus-within CSS selector to allow these wrapping components to display focus when their children receive it.

Other components that were not affected:

• Accordion.Header: :focus-within is being used, however isFocused is needed due to nested interactive elements.
• FauxInput: due to isFocused being a public API, the prop was not removed, however :focus-within is used.

components not using the focusStyles utility

• colorpickers color range
• forms Range component

Examples:

Accordion

Forms

text Input

Checklist

• 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
• 🌐 demo is up-to-date (yarn start)
• ⬅️ renders as expected with reversed (RTL) direction
• 🤘 renders as expected with Bedrock CSS (?bedrock)
• 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
• ♿ tested for WCAG 2.1 AA accessibility compliance
• 📝 tested in Chrome, Firefox, Safari, and Edge

[coveralls]

coverage: 96.15% (-0.004%) from 96.154% when pulling a0b8541 on feat/focus-styles into 22d233b on main.

[lucijanblagonic]

I see some of the things that don't match. I've set the yellow background to be able to test it properly.

Breadcrumbs

Missing focus state on Anchors in Breadcrumbs.

Storybook:

Expected:

Danger buttons

Buttons with isDanger should have blue-600 border styling.

Storybook:

Expected:

Split buttons

Inset focus on Split buttons should match the button isPrimary inset styling.

Storybook:

Expected:

Anchors

Storybook:

Expected:
Implement focus with outline with outline-offset if possible (to ensure long links that break into the next row always have an encompassing outline.

Chrome

Storybook:

Expected:
Default focus state in blue-600like we have for buttons or anchors.

Skip links

Remove underline for focused Skip links.

Storybook:

Expected:

Multiselect

Focused Multiselect is showing two focus states. Only the outer one should be visible (same as now).

Storybook:

Expected:
Only outer focus state.

Select

Only the border change is visible with focus.

Storybook:

Expected:
Our default inset focus style.

Tag

Tags should have a 2px outset focus (1px white, 1px blue-600), not 3px.

Storybook:

Original Tag with 2px focus vs. Tag with 3px focus).

_Color should always be blue-600_

Expected:

2px indicator, it should always be 1p white, 1px blue-600

---

I will do another pass after everything is implemented.

[Francois-Esquire]

Thanks for the feedback @lucijanblagonic !

• Breadcrumb demo now allows the Anchors to receive focus
• Button in danger state now has correct border color
• Tag border size has been adjusted
• Select now has the correct focus inset styles
• Multiselect has been fixed to remove the inner focus
• SkipNavLink underline has been removed when focused

A few of the items that include the Chrome header navigation items and SplitButton did raise some concerns that I would appreciate your input on. Will reach out offline!

[Francois-Esquire]

@jzempel (RE: sub nav header item color) I believe the SubNavHeaderItem focus color difference was a previously existing issue. We would need to add useChromeContext within the CollapsibleSubNavItem (here) and pass in isLight to the internal styled component. Raised #1558 to address this!

[Francois-Esquire]

Thanks for the fixes. Nitpicking, but I see some weird sluggish animation style happening on Anchors/Breadcrumbs, but I think this is because of ease-in-out which makes it look "slow". I tested in the GIF below with that default animation and with removing the animation after. Do you see it also? Does this feel weird, or is it just me? 🤷

No you are spot on @lucijanblagonic, this was something I observed as well. It should be fixed now!

[jzempel]

Since my previous review runs counter to #1548 (review), I'm going to mark this approved and leave final design finesse to @lucijanblagonic. Nice work, @Francois-Esquire.

[steue]

heya, another nit relating to animation, for button group, looks like the selected button focus doesn't have a transition.
https://github.com/zendeskgarden/react-components/assets/39511661/e16eca62-0c7e-483c-a325-d969c8c14810

also for split button, when set to Basic, the focus border on the text button isn't aligned with the chevron focus.

nit for text area, a very edge case, when focus is set to inset, i would imagine the focus ring would sit over the resize grip?

noticed the subnav thing as well, but looks like that'll be resolved as a separate bug. other than that this looks great! thanks @Francois-Esquire!!

[Francois-Esquire]

Thanks for the feedback @steue ! I've corrected the ButtonGroup and SplitButton styling and it should be as intended. This does include the transition, which was indeed missing (good catch).

For the TextArea, good eye for catching however it isn't a trivial task because it is a built-in browser (and vendor) specific piece of UI. We will need to do some research to implement this correctly moving forward. Off the bat, the use of ::-webkit-resizer was a challenge to control and we will need to think this through.

[lucijanblagonic]

LGTM! 😍

[geotrev]

Other than remaining feedback, everything looks good.

[geotrev]

I don't know Garden's tolerance for this so maybe I'm just being paranoid, but setTimeout will make this code effectively non-deterministic without jest timer mocks. It may not be clear to consumers in tests I guess. But if that's a non-issue / we don't want them to be testing this logic then disregard my concern :)

[Francois-Esquire]

I think this may be a warp in git history 👽 because this was the original code within the Accordion.Header component being restored after it was removed early in this PR. Currently we don't modify this file at all, however this is something we can talk more about. Come to think of it, there is history I am unaware of of why we implemented in this fashion.

[steue]

awesome @Francois-Esquire ! this looks great! ✨