Skip to content

Highlight correct TOC entry on page load #2185

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
Merged
merged 9 commits into from
Jul 28, 2025
Merged

Highlight correct TOC entry on page load #2185

merged 9 commits into from
Jul 28, 2025

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 7, 2025 •
edited
Loading

Compare

Preview URL - Kitchen Sink - Admonitions - with #topic in the hash portion of the URL:
https://pydata-sphinx-theme--2185.org.readthedocs.build/en/2185/examples/kitchen-sink/admonitions.html#topic

The entry for topic in the right sidebar table of contents is highlighted, as shown in the following screenshot:

This pull request fixes the following bug, which is currently in the dev version of the theme / latest version of the docs.

Production URL - same page - #topic in hash:
https://pydata-sphinx-theme.readthedocs.io/en/latest/examples/kitchen-sink/admonitions.html#topic

The TOC entry for admonition (just below topic) is incorrectly highlighted, as shown in the following screenshot:

Caution

gabalafou
gabalafou commented Apr 7, 2025
View reviewed changes
Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

self review

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

observer = new IntersectionObserver(callback, options);
headingsToTocLinks.keys().forEach((heading) => {
Array.from(headingsToTocLinks.keys()).forEach((heading) => {
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 was actually causing a runtime error in Safari for me. Only the latest versions of Safari support forEach on the map iterator.

@gabalafou
Copy link
Collaborator Author

Not exactly sure why tests are failing

@gabalafou
Copy link
Collaborator Author

Fails for me locally too, but also fails locally on main but not the same errors as what I'm seeing on the GitHub runner :-/

@trallard
Copy link
Collaborator

trallard commented Apr 7, 2025

The tests are failing on CI for the dev version of Sphinx (v8.3.0+/c13af9e) as this changes the behaviour of linkcheck_allowed_redirects (ref:https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-linkcheck_allowed_redirects).

A patch will be needed for this as the tests will keep failing in dev and later on when the new Sphinx release is out

@drammock
Copy link
Member

drammock commented Apr 7, 2025

this changes the behaviour of linkcheck_allowed_redirects

xref to sphinx-doc/sphinx#13462

The problem with linkcheck appears to be an upstream bug

@gabalafou gabalafou added kind: bug Something isn't working impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship tag: javascript Pull requests that update Javascript code tag: UX Issues or improvements related to user experience labels Apr 7, 2025
@gabalafou
Copy link
Collaborator Author

Labeled this PR as block-release because it fixes a bug (in Safari, maybe other browsers too) that I was not aware of until I was working on this PR.

@gabalafou gabalafou mentioned this pull request Apr 25, 2025
Closed
2 tasks
@gabalafou gabalafou linked an issue Apr 25, 2025 that may be closed by this pull request
BUG - Follow-up issues with new scrollspy #2178
Closed
2 tasks
@gabalafou gabalafou mentioned this pull request Apr 25, 2025
Open
@stevepiercy stevepiercy mentioned this pull request May 1, 2025
Open
trallard
trallard previously approved these changes Jul 14, 2025
View reviewed changes
Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks @gabalafou this looks good to me and the bug seems now resolved in the docs preview. I will approve and let @drammock review just in case. If not I will merge in a couple of days.

@drammock
Copy link
Member

for me (Linux/Firefox) the dev docs page and the PR page look the same ("admonition" is scrollspy-highlighted on both pages) 😞 I can get it to highlight "topic" by scrolling to the top of the page, then back down a tiny bit. +1 to merge anyway, I guess, because it fixes a Safari bug, and because we're overdue for a release.

@gabalafou gabalafou force-pushed the highlight-correct-toc-entry-on-page-load branch from 440518a to ae49b46 Compare July 28, 2025 11:10
@gabalafou
Copy link
Collaborator Author

@drammock I couldn't reproduce on Ubuntu + Firefox. However, I did notice a possibly related bug, which I fixed in my most recent push. Once the preview docs build, could you check again in your browser?

gabalafou
gabalafou commented Jul 28, 2025
View reviewed changes
src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
// contains a link to itself).
const link = event.target.closest("a");
if (link && link.hash) {
syncTocHash(link.hash);
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 a previous version of this PR, I used a timeout of 0 ms to call the TOC sync function. In some browsers, this gave window.location time to equal the just-clicked URL. But in some other browsers (Ubuntu + Firefox), it was not enough time. I realized that there's no reason to wait. We already have the hash fragment, so just sync it immediately.

@drammock
Copy link
Member

@drammock I couldn't reproduce on Ubuntu + Firefox. However, I did notice a possibly related bug, which I fixed in my most recent push. Once the preview docs build, could you check again in your browser?

If I go directly to https://pydata-sphinx-theme--2185.org.readthedocs.build/en/2185/examples/kitchen-sink/admonitions.html#topic then it works. If I go to https://pydata-sphinx-theme--2185.org.readthedocs.build/en/2185/examples/kitchen-sink/admonitions.html and then click on topic in the secondary sidebar, it works. If I go to https://pydata-sphinx-theme--2185.org.readthedocs.build/en/2185/examples/kitchen-sink/admonitions.html and then manually add #topic to end of the URL, then I still get admonition being highlighted. But that is a weird/niche/unusual case that I'm happy to overlook.

@gabalafou
Copy link
Collaborator Author

@drammock you are a world class tester.

I just fixed that last issue.

Thank you!

@drammock drammock merged commit 151623d into pydata:main Jul 28, 2025
31 checks passed
@drammock
Copy link
Member

thanks @gabalafou 🎉

cmarqu added a commit to cocotb/cocotb that referenced this pull request Jul 28, 2025
@cmarqu
Use specific version of pydata-sphinx-theme
8254cc1 
See pydata/pydata-sphinx-theme#2185
@cmarqu cmarqu mentioned this pull request Jul 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock approved these changes

@trallard trallard trallard left review comments

Assignees
No one assigned
Labels
impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship kind: bug Something isn't working tag: javascript Pull requests that update Javascript code tag: UX Issues or improvements related to user experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG - Follow-up issues with new scrollspy
3 participants
@gabalafou @trallard @drammock