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

feat: apply focus-visible polyfill to shadow root #102

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

geoffrich
Copy link
Contributor

Alaska Airlines Pull Request

Fixes #97

Summary:

This PR applies the focus-visible polyfill to the shadow root. Previously, we could only detect focus-visible by the class being applied to the <auro-button> host itself. By applying it to the shadow root, we now see the class applied to the inner button element. Since native :focus-visible is now supported in Chrome and Firefox, we only apply the polyfill if we detect that :focus-visible is not supported (i.e., WebKit + IE). This should improve performance.

This also fixes the linked issue where auro-buttons nested inside another web component were not receiving focus styles without any effort needed by the consumer of this component.

The focus state has also been updated to remove outline to prepare for upcoming WCSS changes (AlaskaAirlines/WebCoreStyleSheets#66)

Type of change:

Please delete options that are not relevant.

  • New capability
  • Revision of an existing capability

Checklist:

  • My update follows the CONTRIBUTING guidelines of this project
  • I have performed a self-review of my own update

By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.

Thank you for your submission!

-- Auro Design System Team

@blackfalcon
Copy link
Member

Thanks for this PR! I will dig into this ASAP! Very excited!

Copy link
Member

@blackfalcon blackfalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concern. This code only applied the polyfill if the browser does not yet support the feature?

If that is true, this UX will break. We are not writing styles to match :focus-visible, we are writing styles to .focus-visible. https://github.com/AlaskaAirlines/auro-button/blob/master/src/style.scss#L61

That being said, I like this direction, a lot! But there is more work. We need to write styles that match both selectors. .focus-visible and :focus-visible.

That being said.... Safari is the only real concern. Chrome, Edge and FF all support it. Safari, read the comments, Manuel is working on it.
https://bugs.webkit.org/show_bug.cgi?id=185859

Come EOQ2 we intend to drop IE support, so that tells me that we will be able to drop the polyfill support for this feature as well.

@geoffrich
Copy link
Contributor Author

Concern. This code only applied the polyfill if the browser does not yet support the feature?

If that is true, this UX will break. We are not writing styles to match :focus-visible, we are writing styles to .focus-visible. https://github.com/AlaskaAirlines/auro-button/blob/master/src/style.scss#L61

That being said, I like this direction, a lot! But there is more work. We need to write styles that match both selectors. .focus-visible and :focus-visible.

Do my changes in style.scss address your concern? I updated the selectors to include .focus-visible and :focus-visible.

We can follow the same method in other components — conditionally apply the polyfill for performance, and write styles to target both native and polyfilled selectors.

Copy link
Member

@blackfalcon blackfalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick... Sass opinion update.

src/style.scss Show resolved Hide resolved
src/style.scss Show resolved Hide resolved
src/style.scss Show resolved Hide resolved
src/style.scss Show resolved Hide resolved
@blackfalcon
Copy link
Member

Follow up to this is AlaskaAirlines/WC-Generator#69

@blackfalcon blackfalcon merged commit 0ffcb44 into master Mar 5, 2021
@blackfalcon blackfalcon deleted the native-focus-visible branch March 5, 2021 22:01
@AuroDesignSystem
Copy link
Collaborator

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Completed work has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auro-button: focus styles do not show when inside another shadow root
4 participants