fix: don't highlight the first chapter when another one is the index - #3189
Open
VXNCXNX wants to merge 1 commit into
Open
fix: don't highlight the first chapter when another one is the index#3189VXNCXNX wants to merge 1 commit into
VXNCXNX wants to merge 1 commit into
Conversation
the sidebar treats the first chapter as an alias for index.html, but a README chapter placed later in SUMMARY.md generates index.html itself, so both entries were marked active. Skip the alias when a chapter already links to index.html. Fixes rust-lang#2883.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2883.
The problem
The sidebar treats the first chapter as an alias for
index.html:That holds when the first chapter is the index page. But a
README.mdchapter placed later inSUMMARY.mdgeneratesindex.htmlitself, so loading the index highlights both it and the first chapter:which matches the screenshot in the issue.
The fix
Skip the alias when some chapter already links to
index.html:In that case the real index chapter matches
current_pagedirectly on the line above, so the first-chapter fallback isn't needed and is actively wrong.getAttributerather thanlink.hrefis deliberate:link.hrefis reassigned inside the loop (link.href = path_to_root + href), and the browser-resolved absolute form would never equal a bare string on afile://book. Reading the raw attribute before the loop is the consistent choice.Why the exact string comparison holds up
This was the part I most expected to be brittle, so I checked rather than assumed. The href comes from
Chapter::pathverbatim (helpers/toc.rs:119-125), and the index preprocessor rewrites that path withset_file_name("index.md")(builtin_preprocessors/index.rs:35-40), so the filename is always literallyindex. Across 14 books I built to try to break it —./README.mdin SUMMARY,site-urlset,ReadMe.markdown, aREADME.md#topfragment, a prefix chapter, the index preprocessor disabled — every one produced exactlyindex.html.site-urlonly affects the 404 page'sbase_url;--dest-dirandno-section-labeldon't touch hrefs.The subdirectory case is correctly not matched:
src/sub/README.mdgives hrefsub/index.html, and the rootindex.htmlreally is generated from the first chapter there, so the alias should still apply.I also looked for a false positive — a book with both
src/index.mdandsrc/README.md. That configuration is already broken and warned about (it emits duplicateindex.htmlhrefs and highlighted three entries before this change); with the patch it highlights two rather than three. Not made worse.Tests
Added a case to
tests/gui/sidebar-active.gomlwith a new fixture book whose index chapter is not first.I ran the GUI tests for real rather than reasoning about them — no browser was present, so I installed puppeteer's chrome:
and confirmed the new case is load-bearing by stashing only the
toc.js.hbschange:No CHANGELOG entry, since CONTRIBUTING says those are generated at release time with
cargo xtask changelog— let me know if you'd like one anyway.One pre-existing thing I noticed
A SUMMARY link written as
sub/../README.mdproduces hrefsub/../index.html, which this comparison misses. That book already has a brokenpath_to_rootand highlights nothing before or after the change, so it looks like a separate pre-existing bug rather than something this should handle.