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

🐛 Fix singlehtml target uris to be same-document references #11970

Merged
merged 6 commits into from
Apr 7, 2024

Conversation

eanorige
Copy link
Contributor

@eanorige eanorige commented Feb 8, 2024

Subject: Fix singlehtml target uris to be same-document references

Feature or Bugfix

  • Feature (Output quality improvement)

Purpose

Before this change, target_uris would be generated as
index.html#foo. This makes it difficult to rename the
generated document and can require reloading the document
when following a link, which can cause problems with JupyterLab
hosted documentation.

After this change, they are just #foo, which avoids the above
problems.

Detail

I looked into the history of this code and there doesn't seem to be any reason to put the name of the generated document into target_uris. Support for same document references (with only a fragment identifier) should be universal; documented in https://www.ietf.org/rfc/rfc2396.txt section 4.2.

Relates

n/a

Before this change, target_uris would be generated as
`index.html#foo`.  This makes it difficult to rename the
generated document and can require reloading the document
when following a link, which can cause problems with JupyterLab
hosted documentation.

After this change, they are just `#foo`, which avoids the above
problems.
@picnixz
Copy link
Member

picnixz commented Feb 9, 2024

This RFC is actually outdated. Could you quickly check that this is still compliant with RFC 3986?

Also, I don't know if extensions may rely on the existence of that page name ... I don't think so but if you can quickly do a github search for that one, it'd be great (and maybe add a test to check that the URIs are correctly generated).

Sorry for being nitpicky, but by experience, those changes that seem "fine" sometimes raise unexpected issues...

@picnixz picnixz added the awaiting:response Waiting for a response from the author of this issue label Feb 14, 2024
@mr-c
Copy link

mr-c commented Mar 5, 2024

Could you quickly check that this is still compliant with RFC 3986?

My reading of RFC 3986 affirms that #foo (a fragment-only reference) is still compliant, as it would be resolved against the base URI, as explained in Section 5.1

@picnixz
Copy link
Member

picnixz commented Mar 7, 2024

According to two external reviews, it appears that it will be compliant wih the current standard. Thank you everyone for your time.

@eanorige Could you add a CHANGES entry please? (no need for a test I think).

@picnixz picnixz removed awaiting:response Waiting for a response from the author of this issue priority:low labels Apr 5, 2024
@picnixz
Copy link
Member

picnixz commented Apr 5, 2024

I've added the CHANGES entry, crediting you with your github nickname. Thank you for your contribution! I'll merge once the tests passed.

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

wait!

@picnixz
Copy link
Member

picnixz commented Apr 5, 2024

wait!

Then I'll wait!

@chrisjsewell
Copy link
Member

chrisjsewell commented Apr 5, 2024

I just want to double-check this, since its a pretty significant change;
in particular I know there are tools that build PDFs from these documents, and I want to check this won't break them 😅

@picnixz
Copy link
Member

picnixz commented Apr 5, 2024

in particular I know there are tools that build PDFs from these documents, and I want to check this won't break them 🤔

Oh ! I wasn't aware of this (I would have thought they would be using LaTeX). Good catch then (though I would say, if we are RFC compliant, should we actually worry about it; I'd expect the tools to be compliant as well but maybe it's a bit too much, or maybe it's because singlehtml may be understood from a browser point of view or a standalone HTML, no browser needed, point of view).

@chrisjsewell
Copy link
Member

I would have thought they would be using LaTeX

Basically its a "light-weight" alternative to running the whole horrible latex toolchain lol (naturally with some tradeoffs)

@chrisjsewell
Copy link
Member

cc @danwos and @ubmarco, maybe you want to have a quick check of this, in particular if it would affect: https://github.com/useblocks/sphinx-simplepdf/blob/3a37cce56e5c4c019959621853c7b92e10f8a4ad/sphinx_simplepdf/builders/simplepdf.py#L26

@chrisjsewell
Copy link
Member

FYI, I think it makes sense in general, I just want to check whether it would affect anyone "relying" on the current HTML output.
Was trying to track down where the current behaviour was implemented, its at least in b08a7c4, ...

@picnixz
Copy link
Member

picnixz commented Apr 5, 2024

I'll let you merge this one then, unless there is an issue, in which case I'd like you to @ me.

@chrisjsewell
Copy link
Member

chrisjsewell commented Apr 5, 2024

Was trying to track down where the current behaviour was implemented

yeh goes all the way back to its creation: 744a519

so thats good, that it was not added later to fix a particular bug or anything 👍

@chrisjsewell chrisjsewell self-requested a review April 5, 2024 10:15
@danwos
Copy link
Contributor

danwos commented Apr 5, 2024

Thanks for taking sphinx-extensions into account :)

I have made a quick test with Sphinx-SimplePDF and this PR.
I used the official Sphinx documentation as test object and it looks good:
image

Sphinx_with_fix.pdf

The internal links are still working.

So, the change looks fine to me 👍 Thanks for it!

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

ok all good then thanks!

@chrisjsewell chrisjsewell changed the title Fix singlehtml target uris to be same-document references 🐛 Fix singlehtml target uris to be same-document references Apr 7, 2024
@chrisjsewell chrisjsewell merged commit 413e740 into sphinx-doc:master Apr 7, 2024
22 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2024
@AA-Turner AA-Turner added this to the 7.3.0 milestone Jul 13, 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.

7 participants