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

Jump to bottom

Consistent focus ring (first pass) #1549

Merged
merged 14 commits into from
Nov 13, 2023
Merged

Consistent focus ring (first pass) #1549

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4062f89
wip
gabalafou Oct 30, 2023
1d5af58
Style focus state in header nav
gabalafou Oct 31, 2023
5856c9e
update focus ring style on all focussable elements
gabalafou Oct 31, 2023
99dcef3
simplify
gabalafou Oct 31, 2023
850fc9c
fix links in mobile sidebar overlay
gabalafou Nov 6, 2023
357b34c
put focus rings around a few more focusable elements
gabalafou Nov 6, 2023
7470f40
polish
gabalafou Nov 6, 2023
37640dd
update comment
gabalafou Nov 6, 2023
e2d1810
review
gabalafou Nov 7, 2023
94e6804
better align focus ring on collapsible admonitions
gabalafou Nov 9, 2023
f3f53c0
comment and simplify sphinx-togglebutton focus ring
gabalafou Nov 9, 2023
029d178
make css override more explicit
gabalafou Nov 9, 2023
b4c08a9
Fix SD link-card focus ring and on homepage, bring links inside card
gabalafou Nov 13, 2023
e67849e
Update docs/index.md
drammock Nov 13, 2023
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
15 changes: 2 additions & 13 deletions src/pydata_sphinx_theme/assets/styles/abstracts/_links.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ $link-hover-decoration-thickness: unquote("max(3px, .1875rem, .12em)") !default;
color: var(--pst-color-link-hover);
}
}
@include focus-indicator;
}

// Text link styles
Expand All @@ -102,7 +101,6 @@ $link-hover-decoration-thickness: unquote("max(3px, .1875rem, .12em)") !default;
@include link-decoration;
@include link-decoration-hover;
}
@include focus-indicator;
}

// Sidebar and TOC links
Expand Down Expand Up @@ -137,10 +135,8 @@ $link-hover-decoration-thickness: unquote("max(3px, .1875rem, .12em)") !default;
font-weight: 600;
color: var(--pst-color-primary);
@if $link-hover-decoration-thickness {
box-shadow: inset
$link-hover-decoration-thickness
0px
0px
border-left: $link-hover-decoration-thickness
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bootstrap uses box-shadow for focus rings, so I changed the code here to use border-left instead of using a box shadow to draw a vertical line or notch on the left side of the link in the TOC that corresponds to the current page that the user is on.

Copy link
Member

Choose a reason for hiding this comment

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

I think this caused a small glitch, because the border has an actual width compared to the box-shadow?

Comparing the readthedocs preview of this PR vs the latest docs:

https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/layout.html#templates-and-components:

image

And on the PR (https://pydata-sphinx-theme--1549.org.readthedocs.build/en/1549/user_guide/layout.html#templates-and-components):

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

ooh, good catch @jorisvandenbossche

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks for catching that! Follow up PR: #1561

solid
var(--pst-color-primary);
}
}
Expand Down Expand Up @@ -175,10 +171,3 @@ $link-hover-decoration-thickness: unquote("max(3px, .1875rem, .12em)") !default;
}
}
}

// Focus indicator
@mixin focus-indicator {
&:focus-visible {
outline: 2px solid var(--pst-color-accent);
}
}
10 changes: 10 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/base/_base.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,13 @@ pre {
background-color: var(--pst-color-secondary);
border: none;
}

// Focus ring
//
// Note: The Bootstrap stylesheet provides the focus ring (customized via Sass
// variables in _bootstrap.scss) in some cases. This rules covers all other
// cases.
:focus-visible {
outline: $focus-ring-outline;
box-shadow: none; // override Bootstrap
}
19 changes: 12 additions & 7 deletions src/pydata_sphinx_theme/assets/styles/components/_search.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@
&:focus,
&:focus-visible {
border: none;
box-shadow: none;
outline: 3px solid var(--pst-color-accent);
Comment on lines -61 to -62
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new :focus-visible rule in the base stylesheet now provides this focus ring.

background-color: var(--pst-color-background);
color: var(--pst-color-text-muted);
}
Expand All @@ -75,11 +73,16 @@
align-items: center;
align-content: center;
color: var(--pst-color-text-muted);
// Needed to match other icons hover
padding: 0 0 0.25rem 0;
border-radius: 0;
border: none; // Override Bootstrap button border
font-size: 1rem; // Override Bootstrap button font size
Comment on lines +77 to +78
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For both the search button and the theme switcher button, I decided to go ahead and make them consistent with the other icons at the end of the nav bar.


// Override Bootstrap button padding-x. Whitespace in nav bar is controlled
// via column gap rule on the container.
padding-left: 0;
padding-right: 0;

@include icon-navbar-hover;
@include focus-indicator;

i {
font-size: 1.3rem;
Expand Down Expand Up @@ -136,19 +139,21 @@
* Lives at components/search-button-field.html
*/
.search-button-field {
$search-button-border-radius: 1.5em;
display: inline-flex;
align-items: center;
border: var(--pst-color-border) solid 1px;
border-radius: 1.5em;
border-radius: $search-button-border-radius;
color: var(--pst-color-text-muted);
padding: 0.5em;
background-color: var(--pst-color-surface);

&:hover {
border: 2px solid var(--pst-color-link-hover);
}

&:focus-visible {
border: 2px solid var(--pst-color-accent);
border-radius: $search-button-border-radius;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is needed to override a rule I added in _header.scss that slightly rounds the focus rings for the links in the header, but the search button we want the focus ring to "hug" its border, so the border-radius must match.

}

// The keyboard shotcut text
Expand Down
17 changes: 11 additions & 6 deletions src/pydata_sphinx_theme/assets/styles/components/_switcher-theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,23 @@
*/

.theme-switch-button {
// overide bootstrap settings
margin: 0 -0.5rem;
padding: 0; // We pad the `span` not the container
color: var(--pst-color-text-muted);
border-radius: 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This now gets a rounded focus ring so I decided to remove this border-radius rule.

@include focus-indicator;
border: none; // Override Bootstrap button border
font-size: 1rem; // Override Bootstrap's button font size

// Override Bootstrap button padding-x. Whitespace in nav bar is controlled
// via column gap rule on the container.
padding-left: 0;
padding-right: 0;

&:hover {
@include icon-navbar-hover;
}

span {
display: none;
padding: 0.5em;

@include icon-navbar-hover;
&:active {
text-decoration: none;
color: var(--pst-color-link-hover);
Expand Down
1 change: 0 additions & 1 deletion src/pydata_sphinx_theme/assets/styles/components/_switcher-version.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ button.btn.version-switcher__button {
}

@include link-style-hover;
@include focus-indicator;
&:active {
color: var(--pst-color-text-base);
border-color: var(--pst-color-border);
Expand Down
4 changes: 4 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/components/_toc-inpage.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,8 @@ nav.page-toc {
}
}
}

:focus-visible {
border-radius: 0.125rem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In several places where we want the focus ring to be rounded at the corners, I use this rule. I only apply the border-radius rule on :focus-visible because if I apply it even when the element doesn't have focus, it will change the way borders appear, which will likely mess up other styles (for example, the vertical notch when the link corresponds to the current page that the user is on).

}
}
9 changes: 9 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/extensions/_copybutton.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,13 @@ div.highlight button.copybtn {
color: var(--pst-color-text);
background-color: var(--pst-color-surface);
}

&:focus {
// For keyboard users, make the copy button visible when focussed.
opacity: 1;
}

&:focus-visible {
outline: $focus-ring-outline;
}
}
4 changes: 4 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ html[data-theme="light"] {
.sd-card-header {
background-color: var(--pst-color-panel-background);
border-bottom: 1px solid var(--pst-color-border);

&:focus-visible {
outline: $focus-ring-outline;
}
}
.sd-card-footer {
background-color: var(--pst-color-panel-background);
Expand Down
5 changes: 5 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/extensions/_togglebutton.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
.admonition {
button.toggle-button {
color: inherit;

&:focus-visible {
outline: $focus-ring-outline;
outline-offset: $focus-ring-width;
}
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/pydata_sphinx_theme/assets/styles/sections/_header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
padding-right: 1rem;
}

:focus-visible {
border-radius: 0.125rem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want all of the focusable links and buttons in the header nav to have rounded corners, so I decided to use this blanket rule, but this means I have to override the border-radius for the search button.

I thought about writing this rule as:

:focus-visible:not(.search-button-field) {
  border-radius: 0.125rem;
}

But I noticed that this file has no other reference to anything search related. It's completely ignorant of the fact that it contains a search button, so I decided that it would be better to add an override rule in the search stylesheet rather than couple this stylesheet to the search button. Just to keep things separate.

}

// These items will define the height of the header
.navbar-item {
height: var(--pst-header-height);
Expand Down Expand Up @@ -99,7 +103,6 @@
color: var(--pst-color-text-muted);
border: none;
@include link-style-hover;
@include focus-indicator;
}

.dropdown-menu {
Expand Down Expand Up @@ -190,7 +193,6 @@
}
}
@include icon-navbar-hover;
@include focus-indicator;
}

// Hide the navbar header items on mobile because they're in the sidebar
Expand Down
29 changes: 26 additions & 3 deletions src/pydata_sphinx_theme/assets/styles/sections/_sidebar-primary.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@
font-size: var(--pst-sidebar-font-size);
}

// Focus styles
:focus-visible {
border-radius: 0.125rem;
}

// override bootstrap when navlink are displayed in the sidebar
.nav-link {
font-size: var(--pst-sidebar-font-size-mobile);
Expand Down Expand Up @@ -80,6 +85,26 @@
border: none;
background-color: inherit;
font-size: inherit;

.dropdown-item {
&:hover,
&:focus {
// In the mobile sidebar, the dropdown menu is inlined with the
// other links, which do not have background-color changes on hover
// and focus
background-color: unset;
}
}
}
}

.bd-navbar-elements {
.nav-link {
&:focus-visible {
box-shadow: none; // Override Bootstrap
outline: $focus-ring-outline;
outline-offset: $focus-ring-width;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default focus ring for the links at the top of the mobile sidebar (under "Site Navigation") was hugging the edges too closely, so this rule just adds a gap, or offset.

}
}
}

Expand All @@ -93,7 +118,7 @@
.sidebar-header-items__end {
display: flex;
align-items: center;
gap: 0.5rem;
gap: 1rem;
}

@include media-breakpoint-up($breakpoint-sidebar-primary) {
Expand Down Expand Up @@ -171,8 +196,6 @@

/* Between-page links and captions */
nav.bd-links {
margin-right: -1rem;

Comment on lines -174 to -175
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't figure out why this rule was here but it was clipping the focus ring, so I removed it. I asked @12rambau if he could remember why he added it and if I could remove it, and he replied, "I guess the only way to find out is to remove it 😄"

@include media-breakpoint-up($breakpoint-sidebar-primary) {
display: block;
}
Expand Down
20 changes: 13 additions & 7 deletions src/pydata_sphinx_theme/assets/styles/sections/_skip-link.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/***
* Rules for the UX/UI of skip navigation link btn.
*It's only visible to people
* Rules for the UX/UI of skip navigation link btn.
* It's only visible to people
* navigating with keyboard for accessibility purposes
* ref: https://www.youtube.com/watch?v=VUR0I5mqq7I
***/
Expand All @@ -12,18 +12,24 @@
right: 0;
text-align: center;
background-color: var(--pst-color-warning);
// Ensure we are using a WCAG conformant colour
color: var(--pst-color-warning-text) !important;
padding: 0.5rem;
z-index: $zindex-modal;
border-bottom: 1px solid var(--pst-color-border);

// This shows / hides the button
transform: translateY(-100%);
transition: transform 150ms ease-in-out;
&:focus {
&:focus-within {
transform: translateY(0%);
// ensure this is visible
outline: 3px solid $foundation-black;
}

a {
// Ensure we are using a WCAG conformant colour
color: var(--pst-color-warning-text) !important;

&:focus-visible {
// use color with sufficient contrast
outline-color: $foundation-black;
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
13 changes: 13 additions & 0 deletions src/pydata_sphinx_theme/assets/styles/variables/_bootstrap.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,16 @@ $dropdown-link-hover-bg: var(--pst-color-surface);
$dropdown-dark-link-hover-bg: var(--pst-color-surface);
$dropdown-link-active-bg: var(--pst-color-surface);
$dropdown-dark-link-active-bg: var(--pst-color-surface);

$focus-ring-width: 0.1875rem; // 3px at 100% zoom (0.1875 * 16px base font = 3px)
$focus-ring-opacity: 1;
$focus-ring-color: var(--pst-color-accent);
$focus-ring-blur: 0;
$focus-ring-box-shadow: 0 0 $focus-ring-blur $focus-ring-width $focus-ring-color;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how to customize the focus ring styles that Bootstrap provides.

// outline creates the same style of focus ring, it just uses CSS outline instead of box shadow
$focus-ring-outline: $focus-ring-color solid $focus-ring-width;
$btn-focus-box-shadow: $focus-ring-box-shadow;

.btn {
--bs-btn-focus-box-shadow: #{$btn-focus-box-shadow};
}
2 changes: 1 addition & 1 deletion src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@

{# A button hidden by default to help assistive devices quickly jump to main content #}
{# ref: https://www.youtube.com/watch?v=VUR0I5mqq7I #}
<a class="skip-link" href="#main-content">{{ _("Skip to main content") }}</a>
<div class="skip-link"><a href="#main-content">{{ _("Skip to main content") }}</a></div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I re-factored the skip link markup because if we use a single HTML tag then we have a focus ring that spans the entire width of the page, rather than just the link text.


{%- endblock %}

Expand Down
Loading