Skip to content

toc.js: marking active page in toc when redirected to url without .html suffix #2570

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pistonight
Copy link
Contributor

Issue

When serving the book through services that redirects /foo/bar.html -> /foo/bar, the page in TOC isn't marked as active because the toc uses an exact match with the href on the a tag.

This is a common behavior for services such as GH pages, and tools like npx serve (which actually redirects to paths without .html and serves the html anyway. in this case the active page never gets marked)

Fix

Refactored the check for if a link is current page, and loosened the condition so the .html suffix is optional:

  • /foo and /foo.html will match /foo.html link
  • /foo/, /foo/index, /foo/index.html will match /foo/index.html link

If the active page is still not found, it will try to map /foo to /foo/index.html, which is the redirect behavior of some services/tools

Testing

cargo run -- build test_book
npx serve test_book/book

Verified each link in the TOC now gets highlighted when switching to the page

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Mar 5, 2025
@szabgab
Copy link
Contributor

szabgab commented Mar 6, 2025

I did not notice that Github pages redirects page.html to page, but I do see that if can serve the content of page.html if we access page. Do I have to set some configuration option for that redirection to work?

I personally prefer to direct people to the nice URLs (without the .html) so currently I post-process the generated html files and the toc.js removing all the .html extensions. In the future we might have a way to tell mdbook to use this linking method so I won't need to do the post processing, but it will mean that toc.js will also contain href entries without .html.

I tried this PR locally, both with the files generated by mdbook and the files after my post-processing. It seems that the highlighting works in all the cases.

One thing I find odd in the PR is that current_page is used in isCurrentPage without passing it as an argument. It works, but I am not sure I like it.
Related to this I think it would be cleaner if the code

        this.innerHTML = '{{#toc}}{{/toc}}';
        // Set the current, active page, and reveal it if it's hidden
        let current_page = document.location.href.toString().split("#")[0];
        if (current_page.endsWith("/")) {
            current_page += "index.html";
        }
        var links = Array.prototype.slice.call(this.querySelectorAll("a"));

came after all the function definitions and we would not have

code
function definitions
code

but

function definitions
code

@Pistonight
Copy link
Contributor Author

Pistonight commented Mar 6, 2025

I did not notice that Github pages redirects page.html to page, but I do see that if can serve the content of page.html if we access page. Do I have to set some configuration option for that redirection to work?

My bad for not making that clear, gh pages behaves like what you said (serves html without the extension and without redirect). npx serve redirects to paths without the extension and that's what I used for testing, but I originally noticed the issue with gh pages

I tried this PR locally, both with the files generated by mdbook and the files after my post-processing. It seems that the highlighting works in all the cases.

Yep that's intended, this code should require no change to continue working if the a tags are changed in the future to not include the html extension

One thing I find odd in the PR is that current_page is used in isCurrentPage without passing it as an argument. It works, but I am not sure I like it.

I will update that

@Pistonight Pistonight force-pushed the feat/looser_active_link_check branch from 81a07a5 to 568b064 Compare March 7, 2025 01:13
@Pistonight
Copy link
Contributor Author

Addressed the comments, and I also found a bug where in the test book, the prefix chapter gets incorrectly highlighted for the root path because it's the first chapter, while the root path opens Introduction instead. In this case the first chapter is not aliased to because an index.html path exists. I fixed it by doing the aliasing logic in an extra pass in the end

@Pistonight
Copy link
Contributor Author

@ehuss are you able to review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants