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 13 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
2 changes: 1 addition & 1 deletion docs/examples/gallery.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Thanks for your support!
link: https://fairlearn.org/main/about/
- title: Feature-engine
link: https://feature-engine.readthedocs.io/
- title: idtracker.ai
- title: idtracker.ai
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 build step was auto-linking idtracker.ai, outputting two anchor tags instead of one.

link: https://idtracker.ai/
- title: MegEngine
link: https://www.megengine.org.cn/doc/stable/en/index.html
Expand Down
8 changes: 3 additions & 5 deletions docs/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@ A clean, Bootstrap-based Sphinx theme by and for [the PyData community](https://
- header: "{fas}`circle-half-stroke;pst-color-primary` Light / Dark theme"
content: "Users can toggle between light and dark themes interactively."
- header: "{fas}`palette;pst-color-primary` Customizable UI and themes"
content: "Customize colors and branding with CSS variables, and build custom UIs with [Sphinx Design](user_guide/web-components)."
content: "Customize colors and branding with CSS variables, and build custom UIs with [Sphinx Design components](user_guide/web-components)."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since there is another link on the home page to the same URL, it's considered good practice for accessibility to make both of them also have the same name: "Sphinx Design components."

- header: "{fab}`python;pst-color-primary` Supports PyData and Jupyter"
content: "CSS and UI support for Jupyter extensions and PyData execution outputs."
link: "examples/pydata.html"
content: "CSS and UI support for Jupyter extensions and [PyData execution outputs](examples/pydata.ipynb)."
drammock marked this conversation as resolved.
Show resolved Hide resolved
- header: "{fas}`lightbulb;pst-color-primary` Example Gallery"
content: "See our gallery of projects that use this theme."
link: "examples/gallery.html"
content: "See [our gallery](examples/gallery.md) of projects that use this theme."
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 HTML template for cards with links decouples the link text (which should be the card content) from the link element, causing them to appear to assistive tech as their hrefs, in this case "examples/pydata.html" and "examples/gallery.html."

I felt it would be better to bring the links into the cards for two reasons:

  1. Seeing/hearing "Pydata execution outputs" and "our gallery" is arguably nicer than hearing the hrefs
  2. It makes these two cards consistent with the other 4 cards, which do not have link fields. I always felt it was a little weird that the last two cards had hover and focus states while the others did not.

```

```{seealso}
Expand Down
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).

}
}
5 changes: 3 additions & 2 deletions src/pydata_sphinx_theme/assets/styles/content/_admonitions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
* Admonitions CSS originally inspired by https://squidfunk.github.io/mkdocs-material/getting-started/
*/
$admonition-border-radius: 0.25rem;
$admonition-left-border-width: 0.2rem;
div.admonition,
.admonition {
margin: 1.5625em auto;
padding: 0 0.6rem 0.8rem 0.6rem;
overflow: hidden;
page-break-inside: avoid;
border-left: 0.2rem solid;
border-left: $admonition-left-border-width solid;
border-color: var(--pst-color-info);
border-radius: $admonition-border-radius;
background-color: var(--pst-color-on-background);
Expand Down Expand Up @@ -226,7 +227,7 @@ div.admonition,
margin-top: 0;

// Undo the .sidebar directive border
border-width: 0 0 0 0.2rem;
border-width: 0 0 0 $admonition-left-border-width;

// TODO: these semantic-color-names border-color rules might no longer be
// needed when we drop support for Sphinx 4 / docutils 0.17
Expand Down
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;
}
}
22 changes: 22 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 @@ -149,6 +149,22 @@ html[data-theme="light"] {
.sd-card-body {
background-color: var(--pst-color-panel-background);
}

// Focus ring for link-cards
.sd-stretched-link:focus-visible {
// Don't put the focus ring on the <a> element (it has zero height in Sphinx Design cards)
outline: none;

// Put the focus ring on the <a> element's ::after pseudo-element
&:after {
outline: $focus-ring-outline;
border-radius: 0.25rem; // copied from Sphinx Design CSS for .sd-card
}
}

&.sd-card-hover:hover {
border-color: var(--pst-color-link-hover);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Snuck in a hover color correction with this commit.

}
}
/*******************************************************************************
* tabs
Expand Down Expand Up @@ -256,5 +272,11 @@ details.sd-dropdown {
.sd-summary-down {
top: 0.7rem;
}

// Focus ring
&:focus-visible {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These focus ring CSS rules that I had put under .sd-card more properly belong in this section of the CSS file.

outline: $focus-ring-outline;
outline-offset: -$focus-ring-width;
}
}
}
43 changes: 43 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 @@ -8,6 +8,43 @@
button.toggle-button {
color: inherit;
}

// Focus ring
//
// Sphinx-togglebutton makes the entire admonition header clickable, but
// only the button within the header is focusable. We want the entire
// clickable area to be surrounded with a focus ring, so that's why we use
// the :focus-within selector, rather than a :focus-visible selector on the
// button.
&:focus-within {
overflow: visible;

// The complicated focus ring styles here are a consequence of the markup
// and border styles for this particular admonition class. (For the other
// type of admonition on this site, the focus ring style is achieved with
// simple `outline` and `outline-offset` rules on the admonition's
// header.) The problem is that Sphinx-togglebutton puts the admonition's
// left border on the outermost container (rather than separately setting
// the left border on the container's children). This makes it complicated
// to get the focus ring to simultaneously cover the left border in the
// header and align perfectly on the right with the body.
.admonition-title:focus-within:before {
content: "";
transform: translateX(
-$admonition-left-border-width
); // align left edges of admonition and ring
width: calc(100% + $admonition-left-border-width); // align right edges
height: 100%;
border: $focus-ring-outline;
border-radius: $focus-ring-width;
}

// When expanded, sharpen the bottom left and right corners of the focus ring
&:not(.toggle-hidden) .admonition-title:focus-within:before {
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;
}
}
}

// Details buttons
Expand All @@ -16,5 +53,11 @@
summary {
border-left: 3px solid var(--pst-color-primary);
}

// When expanded, sharpen the bottom left and right corners of the focus ring
&[open] :focus-visible {
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;
}
}
}
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
Loading
Loading