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

Stop removing focus outlines by default #66

Merged
merged 4 commits into from
Mar 5, 2021
Merged

Conversation

geoffrich
Copy link
Contributor

Alaska Airlines Pull Request

Fixes #63

Summary:

Focus outline changes

This PR stops removing focus outlines by default (see my rationale in the linked issue). The outline is only removed when the browser supports the native :focus-visible selector (or the polyfill is loaded) and the focus-visible state does not apply. I added a rule that uses native :focus-visible since support has landed in Chrome + Firefox, with the intention to eventually remove the polyfill selectors entirely once focus-visible lands in WebKit.

There were also some -moz-focus prefixes in normalize that I removed because they were interfering with the new focus rules. I tested with the latest Firefox (86) and could not discern any differences or rogue focus outlines with these rules removed. I believe -moz-focusring has been replaced with :focus-visible.

There is one potential breakage with this release -- outline is no longer removed by default when focusing an element and focus-visible does apply. Previously, this was happening because outline was removed in all focus cases. You will need to remove outline in your focus-visible styles yourself if you were using border or box-shadow for focus. I think this should mainly be a concern for custom elements that consume essentials.css, and we can make the necessary changes when updating the packages in those elements. CSS is generated at build time, so nothing should happen unexpectedly for existing consumers of these elements.

Other changes

  • added a local demo page to see how these rules affected native elements and auro-button.
  • fixed external link typo in docs

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

@@ -67,3 +60,15 @@ $focus: null !default;
// Only visible when Windows High-Contrast Mode is turned on
outline: 3px solid transparent;
Copy link

@littleninja littleninja Mar 4, 2021

Choose a reason for hiding this comment

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

Could we merge these two selectors?

.js-focus-visible :focus:not(.focus-visible),
:focus:not(:focus-visible) {
  outline: 3px solid transparent;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not — a single invalid selector invalidates the whole rule. So browsers that don’t support :focus-visible would be broken.

Choose a reason for hiding this comment

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

Ooo, did not realize that could happen if browsers do not support a selector (if I understand your explanation). Thank you!

@blackfalcon blackfalcon merged commit 45e0755 into master Mar 5, 2021
@blackfalcon blackfalcon deleted the focus-defaults branch March 5, 2021 03:18
@AuroDesignSystem
Copy link
Collaborator

🎉 This PR is included in version 3.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.

Do not remove focus styles by default
4 participants