-
Notifications
You must be signed in to change notification settings - Fork 62
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
Restore focus rings #230
base: main
Are you sure you want to change the base?
Restore focus rings #230
Conversation
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.
self review
&::after { | ||
outline-color: var(--sd-color-#{$color}) !important; | ||
} |
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.
This is mainly for link-cards where the <a>
tag has a class like sd-btn-info
applied to it
@@ -23,6 +23,7 @@ | |||
-moz-user-select: none; | |||
-ms-user-select: none; | |||
-webkit-user-select: none; | |||
outline-offset: 0.25rem; |
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.
It's important to put some space between the button and the focus ring in case the two have matching colors.
&::after { | ||
outline-style: auto; | ||
outline-color: var(--sd-color-primary); | ||
} |
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.
The trick of using ::after
for clickable link-cards has an unfortunate drawback.
The browser's default stylesheet has some :focus-visible
rules that help ensure the focus ring is visible. However the ::after
pseudo-element does not receive browser focus. Its parent element, the anchor element, is what receives focus. Therefore the pseudo-element does not match the browser's default :focus-visible
rules.
This is why I set the outline color here explicitly, because the ::after
pseudo-element will not pick up any of the browser default styles, and without those styles or explicitly setting the outline color, the focus ring color would default to currentcolor
. When you have a link with white text, this results in a white focus ring, which is not visible for a white-background site.
@@ -90,6 +96,9 @@ $semantic-colors: ( | |||
&.sd-outline-#{$color}:hover { | |||
border-color: var(--sd-color-#{$color}-highlight) !important; | |||
} | |||
&.sd-outline-#{$color}:focus-visible { | |||
outline-color: var(--sd-color-#{$color}-highlight) !important; | |||
} |
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.
This file sets the focus ring color to match either the border color or the background color of links and buttons.
&:focus { | ||
outline: none; | ||
} | ||
|
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.
As mentioned in the PR description, I really don't understand why this rule was ever put in place.
@@ -33,7 +33,7 @@ | |||
} | |||
|
|||
// Disable focus indicator for pointer devices | |||
&:not(.focus-visible) + label { | |||
&:not(:focus-visible) + label { |
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'm almost certain this was a typo. It has the unfortunate consequence of turning off focus rings on tabs. Here's how. The &:not(.focus-visible)
will match the input
element because there is no .focus-visible
class on it (I searched the code base thoroughly). So this selector might as well be .sd-tab-set > input + label
, which will match the tab, set its outline to none.
I think this rule was intended to prevent an effect on touchscreen devices where you tap an element and it gets briefly highlighted (or darklighted).
style/_tabs.scss
Outdated
@@ -48,7 +48,7 @@ | |||
cursor: pointer; | |||
font-size: var(--sd-fontsize-tabs-label); | |||
font-weight: 700; | |||
padding: 1em 1.25em 0.5em; | |||
padding: 0.5em 1.25em 0.5em; |
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.
As mentioned in the PR, this makes the top and bottom padding symmetrical. It's unclear to me why there was more top padding to begin with. I thought maybe it was to create some white space above the tabbed panel, but it already has top margin, so this value is a bit puzzling to me.
I may be missing something here.
@@ -10,3 +10,7 @@ | |||
.sd-sphinx-override p { | |||
margin-top: 0; | |||
} | |||
|
|||
.sd-sphinx-override:focus-visible { | |||
outline-offset: 0.25rem; |
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.
This helps ensure that this extension's focus rings are accessible. If the focus ring hugs the element, if there is no gap, and the background or border color of the element matches the focus ring, then it will be hard to perceive the focus ring, and therefore not accessible.
For reference, see WCAG 2.2 Understanding Docs - Understanding SC 2.4.13:
Focus Appearance (Level AAA)
for more information, see https://pre-commit.ci
With this pull request, I have tried to fix all of the focus rings in Sphinx Design to meet accessibility standards.
To that end, this pull request:
:focus-visible
).sd-stretched-link
in the code diff)Background
Ever since the v6 release of Sphinx Design broke the focus rings in the PyData Sphinx Theme back in July, I have wanted to fix this issue upstream.
47f6796 is the commit that first removed focus rings from the
<summary>
element. It's not at all clear to me why this decision was made. In all modern browsers,<summary>
elements do not display outlines when they are focussed with a mouse, only when they are focussed with a keyboard. We fixed this in the PyData Sphinx Theme by simply overriding theoutline: none
rule, but PR #192 (which was released in v6) restructured the CSS, leading it to have higher specificity than our override, thus overriding our override.So it seemed that going forward, it would be better to revert the code in Sphinx Design that removes focus rings (for seemingly no good reason).