Make :focus-visible rules work when polyfill not present
#145
Conversation
There was a problem hiding this comment.
Yes, this makes more sense, the rule was too generic and I like this is a tiny tweak to fix it. I'm still wondering why Safari works without the polyfill, but I guess this means it has partial support for the pseudo selector. I wonder we can 100% ditch the polyfill then? Not a decision for today tho!
Great fix!
I tested on client + frontendshared with Chrome+Safari
| // Selector for browsers using JS polyfill, which adds the "focus-visible" | ||
| // class to a keyboard-focused element. | ||
| &:focus:not(.focus-visible) { | ||
| .js-focus-visible &:focus:not(.focus-visible) { |
There was a problem hiding this comment.
.js-focus-visible. Is a top level class correct? If thats the case, for clarify I think braces help here as well as putting the & on a second line.
.js-focus-visible {
&:focus:not(.focus-visible) {
...
}
}
When the `focus-visible` polyfill is loaded—which it is in our LMS and
client projects—that polyfill will assign a `.focus-visible` class to
elements whose heuristics match the intent of `:focus-visible`, whether
or not the browser natively supports `:focus-visible` or not.
This rule, as previously written, was meant to suppress outlines on
elements if they didn't have the `.focus-visible` class:
```
&:focus:not(.focus-visible) {
@include outline--hide;
}
```
The problem with this selector is that, if the polyfill is not
present, it will always match—because the `.focus-visible` class is
never present—causing all focus outlines to be suppressed, even when the
element is `:focus-visible`. That is, the previous selector was coupled
to the presence of the polyfill.
When present, the polyfill also applies a `.js-focus-visible` class to a
container element. We can make the selector here more specific, and
include that classname, such that it will only match when the polyfill
is actually present:
`.js-focus-visible &:focus:not(.focus-visible)`
Focus outlines "worked" before in the `lms` and `client` projects
because the polyfill was also present, but did not work in this project,
where we don't load the polyfill. Now this should work correctly in
both contexts.
ebf01b7 to
e941fd6
Compare
Codecov Report
@@ Coverage Diff @@
## main #145 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 12
Lines 149 149
Branches 50 50
=========================================
Hits 149 149 Continue to review full report at Codecov.
|
When the
focus-visiblepolyfill is loaded—which it is in our LMS and client projects—that polyfill will assign a.focus-visibleclass to elements whose heuristics match the intent of:focus-visible, whether or not the browser natively supports:focus-visibleor not.The following rule, as previously written, was meant to suppress outlines on elements if they didn't have the
.focus-visibleclass (which is applied/removed by the polyfill):The problem with this selector is that, if the polyfill is not present, it will always match—because the
.focus-visibleclass isnever present—causing all focus outlines to be suppressed, even when the element is
:focus-visible. That is, the previous selector was coupled to the presence of the polyfill.When present, the polyfill also applies a
.js-focus-visibleclass to a container element. We can make the selector here more specific, and include that classname, such that it will only match when the polyfill is actually present:.js-focus-visible &:focus:not(.focus-visible)Focus outlines "worked" before in the
lmsandclientprojects because the polyfill was also present, but did not work in this project, where we don't load the polyfill. Now this should work correctly in both contexts, and is independent of the polyfill.Fixes #126
Test it out
Pull this branch, visit the Buttons pattern-library page and try tabbing around between buttons. You should see a focus outline when you tab with a keyboard.
Compare against
mainwhere buttons don't show an outline.