Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEXT: Resolve global focus states for native elements and Zag-based components #2806

Closed
endigo9740 opened this issue Aug 13, 2024 · 7 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@endigo9740
Copy link
Contributor

endigo9740 commented Aug 13, 2024

Following our recent integration of Zag.js, we've been alerted to the fact that our focus states styles are not consistent within Skeleton. Additionally, Zag handles focus state different than native elements. So we should review our options for providing a unified focus solution, ideally via a global style. Though we may need to replicate some focus states locally due to how Zag handles focus state with data-focus attributes.

Goal

  1. Implement a global focus styles within the Skeleton Tailwind plugin global stylesheet
  2. Handle focus states per component

Resources

@endigo9740 endigo9740 added bug Something isn't working enhancement New feature or request labels Aug 13, 2024
@endigo9740 endigo9740 added this to the v3.0 (Next) milestone Aug 13, 2024
@endigo9740 endigo9740 self-assigned this Aug 13, 2024
@endigo9740
Copy link
Contributor Author

Noting a related bug here: React Ratings components cannot be tab focused when value is 0

@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 16, 2024

Ok, so here's a quick rundown of what I've discovered regarding Tailwind and focus states:

What I've done is go ahead and implement this pull request that does two things:

/* Global Focus */
:focus {
	@apply focus:outline-surface-950 dark:focus:outline-surface-50;
}
/* Focus Class (for components) */
.focused {
	@apply ring-[2px] ring-surface-950 dark:ring-surface-50 ring-inset;
}
  1. Provides an opinionated set of outline color styles the global :focus state styles using theme colors
  2. Implement a new .focused class that can be used by Zag-based components to display when api.focused is true

The result is more consistent focus styling across the board.

Note that the global style does NOT fix the outline issue that appears in select browsers where you may see a double outline (neutral + system OS color outline). Present in in Chrome/Firefox, but not Safari. It merely modifies the outline using the system color to a fixed light/dark theme Surface color for light/dark mode. This results in what may look like a thicker outline depending on the colors - for example white + white.

Here's a before vs after comparison...

Before (default styles):

Screenshot 2024-08-16 at 2 23 34 PM

Screenshot 2024-08-16 at 2 25 24 PM

After (modified):

Screenshot 2024-08-16 at 2 23 50 PM

Screenshot 2024-08-16 at 2 25 33 PM

Note this affect is not present in Safari, which appears to overwrite and control focus state in unique fasion:

Screenshot 2024-08-16 at 2 31 37 PM

Note the only two components that required manual intervention was the Segment Control and Switch. Other components use the global focus state because they use elements like <button> tags.

@bartduisters
Copy link
Contributor

The default approach does work in Safari. Also, since so many folks are using Tailwind these days, isn't the default behavior what users are used to?

The default approach with the white and blue will work both on light and dark borders/backgrounds. While the after shown only has white. I guess it is less clear what would have focus when tabbing?

I guess the question is: would the thicker outline that has one color be sufficient enough if it focuses a button that has the same color as background color?

@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 16, 2024

@bartduisters this is my concern as well. I'm tempted to drop the global style. However, we will have a hard requirement for the .focused class in order for these Zag-powered components to work as expected.

@bartduisters
Copy link
Contributor

My suggestion would be to take over the Tailwind approach, it might be less visually pleasing. But to me, functionality/accessibility is more important when I'm doing keyboard navigation. I rather clearly see what is selected when tabbing and it being visually less pleasing, than the other way around.

@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 16, 2024

@bartduisters if you mean to default to Tailwind's default - then we definitely can for the global focus styles. But I don't think we replicate these for the component focus class. Those will likely never be in sync.

I know you're on Discord atm, so we can bump this convo over there.

@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 16, 2024

Just a quick update - after reviewing on Discord we've decided to forego the global :focus styles. This is likely too opinionated and could easily introduce issues.

Currently we are still using the .focused class styles for components. I'd welcome a review for this!

Furthermore, we may follow up with the Zag team in the near future and bring forth some of the pain points we're facing. See if they have any suggestions or are open to some suggestions for improvement in this regard.

@endigo9740 endigo9740 unpinned this issue Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants