Skip to content
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

singlehtml: Use same-document hyperlinks for internal project references #12551

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Jul 12, 2024

Feature or Bugfix

  • Bugfix

Purpose

  • With pull request 🐛 Fix singlehtml target uris to be same-document references #11970, most hyperlinks from singlehtml are same-document references, containing a relevant HTML fragment identifier in the query-string, but without a filename part (e.g. #document-directives instead of the previous index.html#document-directives). That's a nice feature, because it allows some browsers to avoid a complete page reload when following the link. However: some URI fixup logic elsewhere in the singlehtml builder has still been constructing filename-inclusive hyperlinks. This pull request fixes that.

Detail

Relates

Edit: add missing backtick
Edit: add explanatory note about special pages

@jayaddison
Copy link
Contributor Author

cc @eanorige @danwos - it seems that #11970 fixed many-but-not-all same-document hyperlinks in singlehtml output to remove the filename part - this changeset proposes to expand that.

I'll try to add some test coverage here too.

@jayaddison jayaddison requested a review from picnixz July 17, 2024 17:25
tests/test_builders/test_build_html_tocdepth.py Outdated Show resolved Hide resolved
tests/test_builders/test_build_html_tocdepth.py Outdated Show resolved Hide resolved
sphinx/builders/singlehtml.py Show resolved Hide resolved
jayaddison and others added 4 commits July 18, 2024 09:42
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
…perlinks in `tocdepth` testroot content area are intradocument.
@jayaddison
Copy link
Contributor Author

Two more commits planned (one to fix my codestyle errors, and then one to confirm that external links are unmodified during these singlehtml builds).

@jayaddison
Copy link
Contributor Author

Ok; no changes planned, and this should be ready for further review (apologies for my over-eager review request earlier @picnixz. I should have added more of these tests before that).

@jayaddison
Copy link
Contributor Author

Hmm.. working on this might have helped me figure out a way that we could remove the fix_refuris method entirely - it seems like it is a workaround for the temporary creation of hyperlinks that contain two # symbols during singlehtml builds.

@jayaddison
Copy link
Contributor Author

Hmm.. working on this might have helped me figure out a way that we could remove the fix_refuris method entirely - it seems like it is a workaround for the temporary creation of hyperlinks that contain two # symbols during singlehtml builds.

Ok, yep - that cleanup seems straightforward I think. I can push that here, or as a separate branch/PR.

@jayaddison jayaddison changed the title [singlehtml] Use same-document hyperlinks in nested/fixed reference URIs. [singlehtml] Use same-document hyperlinks for (almost) all internal references. Jul 18, 2024
@jayaddison jayaddison changed the title [singlehtml] Use same-document hyperlinks for (almost) all internal references. [singlehtml] Use same-document hyperlinks for internal project references. Jul 22, 2024
@jayaddison
Copy link
Contributor Author

I think this is ready -- but to mention again: I think it may be possible to remove fix_refuris, and I have prepared a branch that does that too (but don't want to bloat the changeset by including that to begin with).

@picnixz picnixz self-requested a review July 31, 2024 09:56
CHANGES.rst Outdated Show resolved Hide resolved
tests/test_builders/xpath_util.py Outdated Show resolved Hide resolved
tests/test_builders/xpath_util.py Outdated Show resolved Hide resolved
@picnixz
Copy link
Member

picnixz commented Aug 4, 2024

Sorry for the late reply. I started a review, then went (physically) somewhere else, and forgot about it...

@jayaddison
Copy link
Contributor Author

No problem - thanks for the review! I wasn't sure how to phrase the changelog credit either; I do think it is basically a continuation/completion of the previous change.

While going through some other changesets I've learned about StandardDomain._virtual_doc_names -- it contains a list of some of the generated/virtual docnames (genindex, etc), and I'm wondering whether a small refactor to use that might make sense.

@picnixz
Copy link
Member

picnixz commented Aug 5, 2024

I am travelling this week so I won't be able to review except on a phone (which is a bit hard...). I'll reply next week..By the way, thank you very much Jay for the work, you've been helping a lot while I left quite a lot of work on autodoc since I'm working on CPython.. I'll try to get some fixes on autodoc next week or the week after (I now have time to formalize autodoc v2).

@jayaddison jayaddison changed the title [singlehtml] Use same-document hyperlinks for internal project references singlehtml: Use same-document hyperlinks for internal project references Aug 5, 2024
@eanorige
Copy link
Contributor

eanorige commented Aug 6, 2024

I just noticed this today. This PR looks good to me, although I'm not able to understand why some CI docutils tests are failing. Thanks for adding tests to ensure that all same-document links are removed.

@AA-Turner
Copy link
Member

Conflicts due to the tests/ formatting

@AA-Turner AA-Turner merged commit 0bfaadf into sphinx-doc:master Aug 11, 2024
20 checks passed
@jayaddison jayaddison deleted the pr-11970-followup/singlehtml-toctree-refuris-samedocument branch August 11, 2024 22:30
jayaddison added a commit to jayaddison/sphinx_rtd_theme that referenced this pull request Aug 12, 2024
This is intended primarily to resolve a problem with `singlehtml`
documentation projects when viewed on narrow (typically mobile)
displays.

Under those circumstances, the `.current` CSS selector does not
match any sidebar menu elements, because there is no logical notion
of a 'current' page in `singlehtml` -- every item is on the same
page.

Therefore this change proposes an alternative way to identify sidebar
links that are 'current' -- it checks for anchors that have an `href`
beginning with the query-string fragment identifier (`#`).

This is intended to be compatible with both `singlehtml` and `html`
project builds.  It depends upon sphinx-doc/sphinx#12551
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024
@AA-Turner AA-Turner modified the milestones: 8.x, 8.1.0 Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants