-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adding focus-visible styles with polyfill #3695
Conversation
🟡 This pull request modifies 8 files and might impact 5 other files. This is an average splash zone for a change, remember to tophat areas that could be affected. Details:All files potentially affected (total: 5)📄
|
67ee8c4
to
7ba1d45
Compare
src/components/Tooltip/README.md
Outdated
<Link>Order #1001</Link> | ||
<TextStyle variation="strong">Order #1001</TextStyle> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we revert this example change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is actually useful to have one example that isn't a link here. The link never showed this being an issue because it had its own focus styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, the NPM package is nice and clean as well.
The problem with this is that for components not getting a specific focus style, just a tabindex like a tooltip activator, they would never have a focus style reapplied. I think the real problem here is we have two different ways of applying our focus styles. This one adds a box-shadow directly to the focused element, whereas or |
I think that is the reality of the focus ring. We need to make a conscious decision when applying styles to all focusable elements. I don't recall us adding it directly to an element. Do we do that anywhere? |
Not currently but that is part of the issue. We end up with inconsistent browser default focus styles on some elements because we don't do this (like the tooltip example above). In my opinion it would be better to have a better base focus style that we can apply globally that respects focus-visible. We could then tweak those focus styles where we need to in specific components, but we also wouldn't leave anything out then. The alternative is not apply anything globally but fix this up in each individual case but it is going to be difficult to do that in all cases. |
I pushed up one more commit showing what this would look like without any global styles and only fixing the tooltip activator focus for now. |
I think it's worth fixing globally and then going through the components and addressing any technical debt. It will remove a lot of code and hopefully make it a better experience when developing new components or patterns as they will have focus styles by default. |
I implemented a CSS-only solution to using @mixin interaction-focus-ring-suppress {
&::after {
box-shadow: 0 0 0 0 transparent;
@media (-ms-high-contrast: active) {
outline: none;
}
}
}
@mixin interaction-focus-ring($width: rem(0)) {
@include focus-ring($border-width: $width);
&:focus {
@include focus-ring($style: 'focused');
}
&:focus:not(:focus-visible) {
@include interaction-focus-ring-suppress;
}
&:focus-visible {
@include focus-ring($style: 'focused');
}
} So far this seems to have worked nicely. Curious what more the polyfill is affording you? |
That shouldn't work anywhere but chrome right now: https://caniuse.com/mdn-css_selectors_focus-visible |
Yep, exactly. That code would only work in browsers that support |
@dleroux seems to work as intended in the browsers I've tested. You can play around in our Chroma instance if you like - but you'd kind of need to know what components this code is applied to. Edit: I think the part that might not be obvious is the use of |
I was actually following that implementation as well when looking at this. This is only backwards compatible in the sense that you will still get focus styles in all browsers, not that they will only be applied with keyboard focus. For example, take a look a this codepen in Chrome, Safari, and Firefox. In Chrome where |
4eab9e5
to
9e09ba7
Compare
4055976
to
b3da2f3
Compare
* Adding focus-visible polyfill * Adding to UNRELEASED changelog * Removing global styles and fixing tooltip activator * Fixing test
WHY are these changes introduced?
Currently we use a hack for not showing focus rings on things like buttons by blurring on
mouseup
. This works, but it takes adding this behaviour specifically to every component that needs it.This adds the
focus-visible
polyfill so that we can start taking a CSS approach to this that will eventually be supported by browsers and we can remove the polyfill at that point.Right now this would be about a 1kb bundle increase, but I think we could get it to about the same once we start removing the custom click/blur code.
Here is an example of clicking or tabbing to a tooltip activator before this change:
And after this change clicking would show this:
![Screen Shot 2020-12-09 at 1 16 21 PM](https://user-images.githubusercontent.com/478990/101670232-e927db80-3a20-11eb-94d0-45439ac8f450.png)
And tabbing to would show this:
![Screen Shot 2020-12-09 at 1 16 13 PM](https://user-images.githubusercontent.com/478990/101670250-edec8f80-3a20-11eb-8b70-6faf5ed2e20f.png)
WHAT is this pull request doing?
This adds the polyfill package as well as some default
focus-visible
styles for all elements.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes