feat(links): underline content links on hover and focus only, and mark the ones that open a preview - #239
feat(links): underline content links on hover and focus only, and mark the ones that open a preview#239DrDrij wants to merge 3 commits into
Conversation
|
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
There was a problem hiding this comment.
🔵 Needs a closer look
The change is intricate cross-browser CSS with an explicit merge-order dependency on #238 and no computed-style test coverage for the new behavior, so it needs human review before approval.
Pull request overview
This PR changes how QuantEcon content links are decorated so they match the Sphinx lecture builds: .link/.hover-link no longer carry a resting underline and instead underline (solid, in the link's own colour) only on hover and keyboard focus. To keep un-underlined preview links distinguishable, it adds a stacked-squares glyph after every a.hover-link that opens a hover preview (excluding footnote sup, citations, and image-wrapping links), built as a pre-coloured background-image data URI painted into padding-inline-start with a word-joiner content to prevent line stranding. It fits into the theme's existing @myst-theme/styles override strategy in styles/quantecon.css, with an RTL mirror in styles/rtl.css. The author explicitly frames this as depending on #238 merging first and rebasing on top.
Changes:
- Remove resting underline on content links; underline solid on
:hoverand (new):focus-visible. - Add a
Square2Stackglyph after hover-preview links via three pre-coloured data URIs, with:has()andciteopt-outs and an RTLbackground-positionmirror. - Document both changes under CHANGELOG
[Unreleased].
File summaries
| File | Description |
|---|---|
| styles/quantecon.css | Rewrites CONTENT LINKS (no resting underline, hover+focus underline) and adds the HOVER-PREVIEW AFFORDANCE glyph block. |
| styles/rtl.css | Mirrors the glyph's physical background-position for dir="rtl". |
| CHANGELOG.md | Adds Added/Changed entries describing the glyph and the hover-only underline. |
Notes for the author (details in inline comments):
- The specificity/justification comment for
.link:hoveris inaccurate:.article :where(a):hoveris (0,2,0), not (0,1,0), so the underline already appears on hover without this rule; the declaration is actually needed for the:focus-visiblestate. - Both new CHANGELOG entries omit the issue/PR link that CONTRIBUTING.md and every existing entry require.
- The new decoration states and glyph logic (
:has()guard,citeopt-out, focus underline) have no computed-style test and slip under the visual pixel budget.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…k the ones that open a preview `.link` and `.hover-link` drop the resting underline and draw a solid one on hover and on keyboard focus, matching the Sphinx lecture builds. Focus is new: before this, content links underlined on hover alone. Links that open a hover preview carry a heroicons Square2Stack glyph after the label, at `.link-icon`'s size, gap and opacity, so a previewable link is still distinguishable once the underline is gone. Footnote markers and citations take no glyph; they lose the resting underline with everything else. The glyph is an `::after` whose content is U+2060 WORD JOINER with the image painted into `padding-inline-start`, because an inline-block box is an atomic inline that strands on wrap (31 of 61 label lengths in a 300px column). It is a pre-coloured `background-image` rather than a `currentColor` mask because WebKit will not render a mask on a non-replaced inline box. This is the option #176 did not take; that decision is being revisited separately.
#238 landed the superseded decision on these rules alongside its other work. On top of it, this PR's CSS stands as written; what #238 wrote about the resting underline elsewhere comes back out: - the `link-underline-solid` test is replaced by `link-states-and-preview-glyph`, which asserts the resting, keyboard focus and hover states, the word-joiner glyph on the cross-reference and its absence on the plain link and the footnote marker, and dark mode; - the CONTRIBUTING divergence bullet is removed, since matching the lecture builds is not a divergence; - the Links section of docs/typography.md describes hover-only underlining, which link kinds carry the glyph, and the 1.4.1 residue on plain internal links, with the arithmetic that rules out a darker blue; - #238's [Unreleased] entry on the resting underline is dropped so the release notes do not say both things, and the two new entries carry their issue and PR links. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
b0f50a9 to
cf22c57
Compare
mmcky
left a comment
mmcky
left a comment
There was a problem hiding this comment.
Rebased onto main now that #238 has landed, as the "Merge order" section planned. I pushed the rebase to this branch as two commits so the reversal is reviewable on its own: the first is your commit with the two conflicts resolved, the second takes the link surface back from #238. What the second commit absorbs, checked against the table in the description:
| What #238 landed | What the rebase does to it |
|---|---|
text-decoration-line: underline / text-decoration-style: solid on .link, .hover-link and the rewritten CONTENT LINKS comment |
reverted to the rules in this PR; the #237 sentence #238 added to the :visited paragraph is kept |
the link-underline-solid test |
replaced by link-states-and-preview-glyph, which asserts rest = none/solid, keyboard focus and hover = underline at #004979, the word-joiner ::after on the cross-reference and its absence on the plain link and the footnote marker, and dark mode (white text, white glyph) — it passes locally on desktop-chrome |
| the CONTRIBUTING divergence bullet | removed |
## Links in docs/typography.md |
rewritten: hover/focus-only underlining, the glyph and which kinds carry it, the 1.4.1 residue |
its [Unreleased] entry "Content links keep their resting underline" |
removed — the table in the description does not list it, but it would have shipped in the same release notes as this PR's entry saying the opposite |
The two new changelog entries now end with the ([#176](…)) ([#239](…)). link group CONTRIBUTING asks for; Copilot raises that on nearly every PR here. The pre-coloured data URIs and styles/rtl.css are untouched.
Two things are not mine to settle and are flagged for @mmcky rather than fixed. First, the 1.4.1 residue on plain internal .links. The description is candid that this reverses the 2026-09-10 call on #176, and the glyph does cover the previewable links, but myst-to-react gives an internal link hover-link only when the target page carries a description or thumbnail (links/index.js, skipPreview), and the lecture pages mostly carry neither, so most lecture-to-lecture links become colour-only at rest at 2.02:1. One correction to the remedy the description offers: darkening --qe-link-color cannot reach 3:1 against the #44403c prose, because darkening moves toward the prose colour (#004979 is 1.09:1 against it). Reaching 3:1 needs a relative luminance of at least 0.257, and any colour that light is at most 3.42:1 on white, below AA for text. So no light-mode blue closes it, and the only remedies are a non-colour resting cue on .link (the underline, or the weight-600 treatment dark mode already uses). I put that arithmetic in the docs rather than the "darken the token" suggestion. Second, density and consistency on real content, which the description already asks for: every {eq}, {ref} and {numref} gets a glyph, citations do not although they open a popover from the same bracketed-marker shape, and glyph presence on a list of lecture links follows the targets' frontmatter, which is the "reader cannot account for it" pattern #238 cited when it dropped the dotted underlines. Worth a look at a real lecture build before this merges.
The rest is inline. One item has no line to anchor to: the description promises an UPSTREAM-CANDIDATES.yml entry for the .link-icon stranding bug "if this lands", and the diff still has none. It is an upstream defect rather than a QuantEcon style choice, so it qualifies; please add it in this PR.
Baselines still need a refresh on both platforms after your fixes land (the -darwin set is stale from #238 regardless), and Copilot has not reviewed yet.
🤖 Rebase and review by Claude Code, posted via mmcky.
| .hover-link:hover { | ||
| .hover-link:hover, | ||
| .link:focus-visible, | ||
| .hover-link:focus-visible { |
There was a problem hiding this comment.
This never matches on a footnote marker. The focusable element there is the inner HashLink anchor, not the <sup class="hover-link">, and that anchor carries a no-underline utility with only hover:underline (myst-to-react/dist/footnotes.js and hashLink.js). So a keyboard reader tabbing to [1] gets no underline and no colour change, which is exactly the colour-only state the comment above says this rule exists to avoid. Pointer hover still works because sup:hover propagates. Adding .hover-link:focus-within to this selector list (or .hover-link:has(:focus-visible), which the sheet already requires support for) covers it; the glyph rule keys on a.hover-link and is unaffected.
| background-size: 0.85em 0.85em; | ||
| } | ||
|
|
||
| /* (0,2,1), which outranks the (0,1,2) base rule above. */ |
There was a problem hiding this comment.
The arithmetic here and in the two comments below is off, and one conclusion depends on it. :not(:has(img, figure, picture, video)) contributes (0,0,1), the specificity of its most specific argument, so the base rule is (0,1,3), this one is (0,2,2), and .dark a.hover-link::after is (0,2,2). The ordering between those three holds either way. What does not hold is the citation opt-out: cite a.hover-link::after is also (0,1,3), so it ties the base rule and wins on source order only, where this comment says it outranks. Anyone who later groups the opt-outs above the base rule silently gives every <cite><a class="hover-link"> the glyph back. Correcting the numbers and saying "ties, wins on order" at the cite rule is enough; the replacement test does not cover citations (the fixture has none), which is another reason the comment needs to be right.
| ties `.hover-link, .hover-text { text-decoration-style: dotted }`, | ||
| which is why `solid` is restated below rather than assumed. | ||
| - `.link:hover` (0,2,0) ties upstream's compiled `hover:` utility. It has | ||
| to carry `text-decoration-line` itself: the plugin's hover underline |
There was a problem hiding this comment.
Small attribution slip: .article :where(a):hover is this file's own rule in the base-layer typography block (around line 199), not the plugin's. Worth fixing since the bullet is explaining why text-decoration-line has to be restated here.
| /* Upstream's geometry: a 0.85em glyph behind a 0.125rem gap. `em` for the | ||
| glyph so it tracks the content size, `rem` for the gap because that is | ||
| what `.link-icon`'s `ml-0.5` resolves to. */ | ||
| padding-inline-start: calc(0.85em + 0.125rem); |
There was a problem hiding this comment.
The description lists print and forced-colors as out of scope; the print half is one line and worth taking now. background-image is dropped under the default print-color-adjust: economy but this padding is not, so a printed or PDF-exported lecture gets a 0.85em blank tail after every cross-reference, wiki and GitHub link, with no underline left to mark them either. @media print { a.hover-link::after { content: none; } } removes both the gap and the missing glyph. Forced-colors can stay deferred, but note it in the comment: the baked strokes ignore the system LinkText colour that .link-icon next to it follows.
| `myst build --html`: mystmd's static rewriter handles `.html`, `.js` and | ||
| `.json` and never rewrites `url()` inside a stylesheet. */ | ||
| @layer components { | ||
| a.hover-link:not(:has(img, figure, picture, video))::after { |
There was a problem hiding this comment.
Two things the description's link-kind table understates, both worth checking on a real lecture build rather than the fixture. .hover-link — lecture is not one kind: InternalLink adds the class only when the target page has a description or thumbnail (links/index.js, skipPreview), so in a list of twelve lecture links the glyph appears on the three whose targets happen to carry a thumbnail and not on the other nine, and adding a thumbnail to a lecture puts a glyph on links in pages nobody edited. That is the same per-target coin flip #238 cited when it replaced the dotted underline. And crossReference.js hangs the class on every {eq}, {numref} and {ref}, so an equation-heavy lecture renders a glyph after every (3), while the [Sar87] citations in the same paragraph, which also open a popover from a bracketed marker, are opted out below. Not asking for a specific change here; asking that the call on both be made with a real page in front of you.
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - Links that open a hover preview carry a small stacked-squares glyph after the label. `.hover-link` |
There was a problem hiding this comment.
I added the ([#176](…)) ([#239](…)). link group here and on the Changed entry, per CONTRIBUTING. Consider also dropping the last two sentences (the word joiner and the data URI): they describe how the stylesheet is built rather than what a reader of the release notes sees, and both live in the CSS comment already. The Changed entry's "narrowing the divergence recorded under 3.0.0" is history rather than the change; "where upstream underlines always" already says it.
| }); | ||
|
|
||
| test("link-underline-solid", async ({ page }, testInfo) => { | ||
| test("link-states-and-preview-glyph", async ({ page }, testInfo) => { |
There was a problem hiding this comment.
This replaces link-underline-solid and pins what the fixture can show: rest, keyboard focus (moved off and back with Shift+Tab / Tab so :focus-visible matches deterministically), hover, the glyph on the cross-reference and its absence on the plain link and the footnote sup, and dark mode. What it cannot cover is what the fixture lacks: a citation (so the cite opt-out is untested), a wiki or GitHub link, an internal link whose target has a description, a hover-link wrapping an image (the :has() guard), and any hover-link in fixture-rtl (so the styles/rtl.css mirror is exercised by nothing). The description says an eleven-kind fixture exists; committing it under tests/visual/fixture and extending this test over it would close all five in one go, and the pixel suite would then also see the change, which today it does not at maxDiffPixelRatio: 0.01.
| @@ -32,9 +32,23 @@ inside an exercise renders at the same 16px as the exercise. | |||
|
|
|||
| ## Links | |||
There was a problem hiding this comment.
Rewritten on the rebase to describe what the sheet now does, with the contrast residue and the token-recolouring caveat stated in prose as the description asked. Please read it as yours: if the decision on the residue changes, this section and the CONTRIBUTING list are the two places that must move with it.
…erline `.article :where(a):hover` is (0,2,0), not (0,1,0), but that is not why `.link:hover` has to restate `text-decoration-line`: the base-layer rule loses to the components-layer `none` regardless of specificity. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@DrDrij I (and Fable) did a quick review. Let me know if this is useful. I'll check in tomorrow. |
Closes #176. Content links match the lecture builds:
.linkand.hover-linkno longer underline at rest, and draw a solid underline on hover and — new here — on keyboard focus. Links that open a hover preview gain a glyph marking them as such, and that glyph is what makes the un-underlined state readable: without it, a link with a preview card behind it looks exactly like one without.This reverses the provisional call recorded on #176 on 2026-09-10. The reason it is safe to reverse is the glyph: the earlier decision was taken with only two options on the table — keep the underline, or lose the cue — and a marked hover-preview link was not one of them. The accessibility argument that drove that call still stands where the glyph does not reach, and is set out below rather than buried in the stylesheet.
#238 merges first, unchanged. It ships the superseded decision on these same rules, alongside unrelated work — 16px code and callouts, AA footer contrast — that is already reviewed and green. Rather than disturb it, this PR rebases on top once it lands and takes the link surface back. See "Merge order" below for exactly what that absorbs.
What changes
Two declarations on each of the two existing rules in the CONTENT LINKS block, plus
:focus-visible:solidis restated because upstream's dotted rule for.hover-linkis also (0,1,0) and would otherwise pick the style of the underline the hover rule draws. The.darkrules are untouched: nothing under.darkdeclarestext-decoration-line, so the pair above carries through both themes.Focus is new. Before this, content links underlined on hover only, so a keyboard reader had no cue at all.
Then a new HOVER-PREVIEW AFFORDANCE block puts a heroicons
Square2Stackglyph after everya.hover-link, at.link-icon's size, gap and opacity —0.85embehind0.125rem,opacity: .8..link.link, no preview.hover-link— lecture, wiki, xrefsup.hover-link)citedescendant).hover-linkwrapping an image:has()guardFootnote markers and citations are worth calling out: they are excluded from the glyph but not from the de-underlining, so they now render as bare markers. Measured on a live build, both come back
text-decoration-line: none.Why the glyph is built the way it is
Two findings forced an unusual construction, and both are reproducible.
It is a word joiner, not an inline-block. An inline-block icon is an atomic inline, which UAX #14 treats as a break opportunity, so it strands itself on a line of its own whenever the label's last word lands near the column edge — 31 of 61 label lengths in a 300px column, in Chromium and WebKit alike.
content: '\2060'with the glyph painted intopadding-inline-startforbids the break on both sides. Swept across 141 viewport widths from 340–900px with the label wrapping at every one: 0 strandings. Forcing the::afterback to an inline-block makes the same check report 5 of 141, so it can fail.Upstream's
.link-iconhas this bug. Itswhitespace-nowrapwrapper does not prevent the break — worth anUPSTREAM-CANDIDATES.ymlentry if this lands.It is a
background-image, not amask-image. AcurrentColorfill behind a mask would track the link colour for free, but WebKit will not render a mask on a non-replaced inline box: it paints the whole padding box and knocks the glyph out of it, a solid rectangle where the icon should be. Chromium is correct, and WebKit renders the same mask correctly on aninline-block— the one display this cannot use.mask-mode: alpha,-webkit-mask-source-type,mask-clipand a white-stroked source all render identically wrong. Hence three pre-coloured copies, one per state. The cost is that the glyph does not track--qe-link-*; retheming through the tokens recolours the text and leaves the icon stale.Accessibility — why this is the decision, and what it does not cover
CONTRIBUTING's rule is to match the lecture builds except where matching them fails WCAG, and this is a case that arguably does.
#0072bcagainst the#44403cprose is 2.02:1, short of the 3:1 that technique G183 wants where colour alone distinguishes a link. Hover and focus supply the other half of G183, but a mouse or touch reader still has colour alone at rest on any link that gets no glyph — which is every plain internal.link.Dark mode is better placed:
--qe-link-coloris#fffand.dark .article :where(a)already setsfont-weight: 600, so weight carries the distinction without colour.The glyph is the answer for every link that has a hover preview — a non-colour cue, present at rest, on the links a reader most needs to distinguish. External links already carry upstream's arrow. What is left uncovered is the plain internal
.linkwith no preview behind it: on those, at rest, colour is the only cue, and 2.02:1 is short of 3:1.That residue is the honest cost of matching the lecture builds, and it is the same cost the lecture builds have carried in production for years. It is recorded here rather than in a comment so it is not rediscovered as a surprise. Two things would close it if we want it closed later: a
:visited/resting treatment for plain internal links, or darkening--qe-link-coloruntil it clears 3:1 against#44403c— which is a two-theme decision and belongs with #237, not here.CONTRIBUTING's "Matching the lecture builds, and when not to" list needs no entry for this — the point of the change is that it matches — so the bullet #238 adds saying the opposite comes back out here. The 1.4.1 residue above goes into
docs/typography.mdinstead, so the next person choosing a link colour sees it without having to read the stylesheet.Verification
npm run compileclean ·npm run test:unit59/59 · visual suite (desktop-chrome + mobile-chrome, darwin) 45 passed, 0 failed, 15 skipped.Behaviour was measured on a live
myst startbuild against a purpose-made fixture carrying all eleven link kinds, in Chromium and WebKit, light and dark: every content linknone/solidat rest;underline/solidat#004979on hover and on keyboard focus; the glyph on the six previewable links and on none of the other five; white text and white glyph in dark mode.npm run prod:buildwas run and the compiled bundle inspected — all three data URIs survivetailwindcss -mwith their%3C/%23escapes intact, andrelative-css-asset-urls.mjsleavesdata:URLs alone.Visual baselines — none refreshed, and that is the problem
The suite passes against the committed baselines. That is not evidence: the assertions carry
maxDiffPixelRatio: 0.01, and a change confined to a few underlines and one glyph is under that budget on a full-page screenshot.Coverage is genuinely thin. The whole fixture contains two
hover-linkoccurrences — one footnotesup(excluded) and one cross-reference (included) — so the glyph has a single instance under test. Nothing covers a citation, a wiki/github/rrid/ror link, an internal link with a hover card, or ahover-linkwrapping an image, so theciteopt-out and the:has()guard are untested. The RTL fixture's only link is external, so the newstyles/rtl.cssrule is exercised by nothing; it was checked by hand instead.Scope, and what this does not do
.hover-linkis emitted by eight myst-to-react modules, and{eq}/{numref}/{ref}all qualify, so a paragraph with five equation references gets five glyphs. This wants a look at a real lecture build, not the fixture.@media printrule. Abackground-imageis dropped in print but the padding is not, so printed lectures get a blank gap after every preview link.forced-colorsrule. The colours are baked into the SVG, so the glyph cannot follow a forced palette.:visitedis still unstyled, as Which dark palette this theme ships: the pre-0.18.0 greys it has, the book theme's navy-charcoal, or its own #237 leaves it.styles/quantecon.css,styles/rtl.css,CHANGELOG.md. Nothing indocs/orREADME.mdmentions link decoration today, so no docs are owed against currentmain. feat(styles): 16px code and callouts, solid resting underline, AA footer contrast #238 introducesdocs/typography.md, and since it lands first, rewriting its## Linkssection is this PR's job on rebase.Merge order — #238 first, then this
This PR is written against
mainas it stands today. #238 has to merge before it, and this branch then rebases on top. Taking it in that order leaves #238 entirely alone: it is already reviewed and CI-green, and its other work — 16px code and callouts, AA footer contrast — is independent of anything here.The cost of not touching #238 is that it ships the superseded decision, so the rebase is not a fast-forward. This PR absorbs the reversal, and a reviewer should expect the diff to grow by four things that are not in it today:
text-decoration-line: underline/text-decoration-style: solidon.link, .hover-link, with a rewritten CONTENT LINKS commentmainships today, then takes the rules belowlink-underline-solidtest## Linkssection of the newdocs/typography.mdNone of it is difficult — the link work in #238 is confined to one of its five
styles/quantecon.csshunks — but it is real diff that belongs to this PR rather than to #238, and it is the reason this cannot be reviewed as a pure addition.Two further conflicts are positional rather than semantic:
styles/quantecon.css, where #238's callout block and this PR's HOVER-PREVIEW block insert at the same seam, andCHANGELOG.md, where both add under[Unreleased].One thing #238 leaves behind that this PR should pick up: its
-darwinbaselines were never refreshed — only-linuxwas — so they are already stale for its 16px code change. Whichever PR lands second should recapture both sets.Refs #187, #237. Supersedes the deferral recorded in #167.