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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Restore focus rings #230

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ec6887
Restore focus rings to dropdowns
gabalafou Oct 17, 2024
19095d2
Fix focus rings for tabbed panel
gabalafou Dec 2, 2024
94f15e1
Focus rings for clickable cards
gabalafou Dec 2, 2024
9f92b72
Outline offsets and colors
gabalafou Dec 3, 2024
bc60ffe
Equalize top and bottom tab padding
gabalafou Dec 3, 2024
b820e8d
Update style/_tabs.scss
gabalafou Dec 3, 2024
d2f1069
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sphinx_design/compiled/style.min.css
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions style/_button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
-moz-user-select: none;
-ms-user-select: none;
-webkit-user-select: none;
outline-offset: 0.25rem;
Copy link
Contributor Author

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.


&:hover {
text-decoration: none;
Expand Down Expand Up @@ -76,6 +77,12 @@
border-width: 1px !important;
border-style: solid !important;
}
.sd-btn-#{$color}:focus-visible {
outline-color: var(--sd-color-#{$color}) !important;
&::after {
outline-color: var(--sd-color-#{$color}) !important;
}
Comment on lines +82 to +84
Copy link
Contributor Author

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

}
}

// make parent clickable
Expand Down
9 changes: 9 additions & 0 deletions style/_cards.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@
margin-left: 0;
margin-right: 0;
}

.sd-stretched-link:focus-visible {
outline: none;

&::after {
outline-style: auto;
outline-color: var(--sd-color-primary);
}
Comment on lines +29 to +32
Copy link
Contributor Author

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.

}
}

.sd-card-hover:hover {
Expand Down
9 changes: 9 additions & 0 deletions style/_colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,18 @@ $semantic-colors: (
&.sd-bg-#{$color}:hover {
background-color: var(--sd-color-#{$color}-highlight) !important;
}
&.sd-bg-#{$color}:focus-visible {
outline-color: var(--sd-color-#{$color}-highlight) !important;
}
}
a {
&.sd-bg-#{$color}:focus,
&.sd-bg-#{$color}:hover {
background-color: var(--sd-color-#{$color}-highlight) !important;
}
&.sd-bg-#{$color}:focus-visible {
outline-color: var(--sd-color-#{$color}-highlight) !important;
}
}
}

Expand All @@ -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;
}
Copy link
Contributor Author

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.

}
}

Expand Down
7 changes: 2 additions & 5 deletions style/_dropdown.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,14 @@ details.sd-dropdown {
display: none;
}

&:focus {
outline: none;
}

Comment on lines -32 to -35
Copy link
Contributor Author

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.

// The title is split into three parts:
// 1. The icon (optional), which should be on the left
// 2. The text, which should be in the middle, and take all available space
// 3. The state marker, which should be on the right

display: inline-flex;
justify-content: space-between;
outline-offset: 0.25rem;

.sd-summary-icon {
margin-right: 0.6em;
Expand All @@ -64,7 +61,7 @@ details.sd-dropdown {
pointer-events: none;
// align the icon vertically within its container
display: inline-flex;
align-items: center;
align-self: center;
}

// make the state marker a bit more prominent when hovered
Expand Down
4 changes: 4 additions & 0 deletions style/_overrides.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@
.sd-sphinx-override p {
margin-top: 0;
}

.sd-sphinx-override:focus-visible {
outline-offset: 0.25rem;
Copy link
Contributor Author

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)

}
4 changes: 2 additions & 2 deletions style/_tabs.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
}

// Disable focus indicator for pointer devices
&:not(.focus-visible) + label {
&:not(:focus-visible) + label {
Copy link
Contributor Author

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).

outline: none;
-webkit-tap-highlight-color: transparent;
}
Expand All @@ -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;
Copy link
Contributor Author

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.

gabalafou marked this conversation as resolved.
Show resolved Hide resolved
transition: color 250ms;
width: auto;
z-index: 1;
Expand Down
Loading