ENH - Update links styling

[trallard]

This PR introduces changes to the theme's link styling to meet WCAG SC 1.4.1

Particularly by implementing the technique G182 - which introduces a redundant visual cue for folks (i.e., underline, sidebar vertical borders)

Note this PR does not fix focus visible affordances, as this will be done in a subsequent PR (this includes all focus states: links, dropdowns, buttons, etc., as I would like @smeragoel to have a look at the proposed implementation we have in the Figma file).

🤔 Question: does anyone know where I have to change the behavior of the navbar links? I have tried a couple of places/approaches and cannot get the hover and active underline visible for my life.

closes #835

[drammock]

does anyone know where I have to change the behavior of the navbar links?

weird, I see potentially 2 places, maybe that's the problem?

pydata-sphinx-theme/src/pydata_sphinx_theme/assets/styles/sections/_header.scss

Lines 74 to 97 in cb23a7a

	.navbar-nav {
	display: flex;
	@include media-breakpoint-up($breakpoint-sidebar-primary) {
	// Center align on wide screens so the dropdown button is centered properly
	align-items: center;
	}
	li a.nav-link {
	color: var(--pst-color-text-muted);
	&:hover {
	color: var(--pst-color-primary);
	}
	&:focus {
	color: var(--pst-color-primary);
	}
	}
	> .active > .nav-link {
	font-weight: 600;
	color: var(--pst-color-primary);
	}

pydata-sphinx-theme/src/pydata_sphinx_theme/assets/styles/components/_navbar-links.scss

Lines 15 to 43 in cb23a7a

	// Default link styles
	li {
	display: flex;
	flex-direction: column;
	a {
	display: flex;
	align-items: center;
	height: 100%;
	color: var(--pst-color-text-muted);
	padding-top: 0.25rem;
	padding-bottom: 0.25rem;
	&:hover {
	color: var(--pst-color-primary);
	border-style: none;
	text-decoration-line: none;
	}
	&:focus {
	color: var(--pst-color-primary);
	}
	}
	// Active page is always highlighted
	&.current > a {
	font-weight: 600;
	color: var(--pst-color-primary);
	}
	}

or do you mean the icon links? Those are in this file:

pydata-sphinx-theme/src/pydata_sphinx_theme/assets/styles/components/_icon-links.scss

Line 5 in cb23a7a

.navbar-icon-links {

[trallard]

The two first ones are the ones I am after. I might have only changed in one place and did not notice the second file. Thanks.

I was planning to leave the icons until a later iteration but it might be best to have at least the default and hover states updated while I re-evaluate focus states.

[trallard]

Thank you @drammock you pointed me in the right direction.

This should be ready for review.

[drammock]

thanks for this @trallard, and sorry for the slow review.

In addition to the detailed comments below, I wonder if you could offer an opinion about whether we should merge (parts of?) the two source documents (styles/components/_navbar-links and styles/sections/_header) that were the cause of some frustration earlier when trying to change the navbar styles. WDYT?

[drammock]

doesn't skip-ink aid legibility? why disable it on hover?

[trallard]

It does and this is "just" a personal preference so I can be persuaded either way. But since the thickness of the underline changes on hover, it makes a more evident non-colour distinction that a hover is taking place. See:

with skip ink - hover

without skip ink - hover

Since it is on hover and not on the default link state, it should not affect the legibility any more than having a mouse directly over the link.

[trallard]

Thanks for taking some time to review this @drammock it is much appreciated.

I wonder if you could offer an opinion about whether we should merge (parts of?) the two source documents (styles/components/_navbar-links and styles/sections/_header) that were the cause of some frustration earlier when trying to change the navbar styles. WDYT?

I think this is doable, will have another look at these docs as I make some changes and if it makes sense will merge them as part of this PR

[trallard]

It does and this is "just" a personal preference so I can be persuaded either way. But since the thickness of the underline changes on hover, it makes a more evident non-colour distinction that a hover is taking place. See:

with skip ink - hover

without skip ink - hover

Since it is on hover and not on the default link state, it should not affect the legibility any more than having a mouse directly over the link.

[trallard]

@smeragoel - I had not thought about how to indicate the hover state on dropdowns within the navbar. Right now, as @drammock pointed out, there is no visible hover on the dropdown until this is expanded (see video)

Jun-27-2023.11-08-21.mp4

Do you have any recommendations on how to fix this (I had not considered this case as I left dropdown styling until later as we review the rest of the interactive elements)

[choldgraf]

I ran through the built docs and tried a few workflows. This seems like it is in good shape to me, and I'm fine merging it as-is because I think it's a clear improvement and "safe to try" provided we'll likely iterate on this as well.

Some things I noticed for (non-blocking) improvement if you wanna do so before merge or if you have other stuff to address from @drammock :

The underline spans gaps when there is a fontawesome glyph within the link (e.g. on the right sidebar). Any way to prevent that from happening without de-linking the glyph?

I notice this doesn't happen within admonitions, so maybe you can follow the same CSS patterns there:

It seems like we generally treat "UI text" as "not needing an underline", so I wonder if we should remove the underline for the breadcrumbs:

Not sure if it's related to this change or not, but I noticed a kind-of distracting "UI hover effect" that happens on mouse click-down events. It very temporarily highlights a section as active until the mouse is un-pressed, which feels like it may not be intended and is kind-of distracting:

[trallard]

I can look into those icons @choldgraf - I am not 100% happy with those either as now they look odd (different from each other).

Not sure if it's related to this change or not, but I noticed a kind-of distracting "UI hover effect" that happens on mouse click-down events. It very temporarily highlights a section as active until the mouse is un-pressed, which feels like it may not be intended and is kind-of distracting:

Definitely because of the PR - essentially, buttons and links should have the following states default, hover, focus, active and, in some cases current (like nav bars and TOCs ref: https://www.w3.org/WAI/tutorials/menus/styling/#currentnav)

An active state denotes the menu item that was clicked, tapped, or selected through the keyboard (a.k.a activated). Right now, that active state looks the same as the hover state.
The state's purpose is to indicate to the user that an interactive element has been activated (and is good to identify unintended activations). Many websites and apps remove this active styling but then it also removes valuable visible cues for users.
I can try and make this more subtle or less distracting but I would rather not remove it for the above reasons.

[drammock]

An active state denotes the menu item that was clicked, tapped, or selected through the keyboard (a.k.a activated). Right now, that active state looks the same as the hover state.

might this be a good situation to use our "accent" color? I know we need to do something else too in addition to color (maybe a thin all-around border?) Just throwing out ideas, feel free to ignore if you think of something better :)

[trallard]

Righto folks, I think I hunted and fixed most of the links - it ended up being more than I though. This PR should be okay to merge and might need some fixes later.
There are, however, some things to note;

The underline spans gaps when there is a fontawesome glyph within the link (e.g. on the right sidebar). Do you know of any way to prevent that from happening without de-linking the glyph?

I could not find a way to do this (I will need to open an issue and try and fix this later)

• I added a "temporary" focus indicator (many items were missing a focus indicator altogether). It is in our accent colour, but I am not sure about this, so I will wait for @smeragoel recommendations and fix this and the active states in another PR

• Also - visited links are now in our accent colour. I remember something about folks feeling unsure about too much pink. So shall we keep this in pink, or shall I use a different colour (perhaps a darker shade of the primary or secondary colour instead?)

We are also planning to do a keyboard navigation audit in the upcoming couple of weeks so that should be useful to identify more focus indicators missing or keyboard traps.

[drammock]

looks great, +1 for merge and addressing remaining bits in subsequent PRs. Two things that stand out to me that we might want to discuss more and possibly change later:

1. On e.g. the user guide page the in-page TOC tree looks quite messy to me:

   

   I've never much liked having the TOCtree as part of the main content, so one solution is to just remove it and let people navigate with the prev/next buttons or the left sidebar links. Another solution is some kind of formatting change: either special-casing these links to have no underline (or a more subtle one?) or possibly adding some visual structure (like bullets or tree-view lines?) to make it seem less chaotic. Or some other approach that I haven't thought of?

2. I'm not a huge fan of distinguishing "visited" links with a different color, and (as someone with typical color vision) I find the interleaving of teal-vs-pink distracting. For me, if we're going to have visited links in a different color, I prefer a color that is less distinct from the "normal" link state (teal). (that said, I think using pink for the "active" state when doing keyboard navigation is great --- that's a case where it should stand out prominently)

[trallard]

Thanks @drammock

1. Agree there is something that looks a bit odd from the TOCtree. It might be worth opening a separate issue to go over this and make some improvements.
2. Agree here too, the pink colour stands out a lot and I am almost sure some folks will complain about it. There is no specific requirement accessibility-wise to distinguish visited and unvisited links so I suppose we can remove it or make it more subtle. Will work on this

[trallard]

Addressed the last few comments so if y'all are happy with this it could be merged

[drammock]

thanks @trallard!!