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

Drop html4 testing, remove support for html4writer #1364

Closed
benjaoming opened this issue Oct 18, 2022 · 3 comments · Fixed by #1500
Closed

Drop html4 testing, remove support for html4writer #1364

benjaoming opened this issue Oct 18, 2022 · 3 comments · Fixed by #1500
Labels
Design Design or UX/UI related Needed: design decision A core team decision is required
Milestone

Comments

@benjaoming
Copy link
Contributor

benjaoming commented Oct 18, 2022

While testing the theme for the 1.1 release, I noticed that we set an HTML5 doctype while using the HTML4 writer.

image

Since this is probably an old error and no one has reacted, I think that we have happily been supporting html5 only render mode for quite some time but using an html4 CSS version? Edit: It's intentional but also means that no one can actually have HTML 4 using the theme 🤷

@agjohnson I don't know where this leaves html4 support or how important it is.. well, it builds so thus far it's good. I don't think we should spend time trying to make it work correctly but just leave it as is and make sure it's removed for the 2.0 release.

Removing all the html4writer rules can also help clean up the SASS code a bit...

@benjaoming benjaoming added this to the 2.0 milestone Oct 18, 2022
@benjaoming benjaoming added Needed: patch A pull request is required Bug A bug Design Design or UX/UI related labels Oct 18, 2022
@agjohnson agjohnson removed the Bug A bug label Oct 18, 2022
@agjohnson
Copy link
Collaborator

So, a couple things:

The html4 writer still exists in modern Sphinx and docutils. Our support for html4 was never to provide a technically correct html4/xml document, we always wanted an html5 document output. This split in builder type was just to provide styling for docutils html4 and html5 tag structure. So, I wouldn't consider html4 doctype support a bug for that reason.

Html4 writer is already deprecated either way:

https://github.com/readthedocs/sphinx_rtd_theme/blob/master/sphinx_rtd_theme/__init__.py#L47-L51

The next step is figuring out which release we will drop html4 support in, however I think it's a couple out. In our order of priorities, it's probably a bigger win to start pruning our support for Sphinx/docutils versions first, then docutils writers, and then more minor deprecations. There might be an issue already for deprecating html4 writer, we've discussed this one in the past already.

@agjohnson agjohnson added Needed: design decision A core team decision is required and removed Needed: patch A pull request is required labels Oct 18, 2022
@agjohnson agjohnson modified the milestones: 2.0, 3.0 Oct 18, 2022
@benjaoming
Copy link
Contributor Author

@agjohnson firstly, thanks for the background in this! I had seen several conditions in SASS sources for generating different outputs for the html4writer, so I assumed it had to do with HTML4 compatibility, but it's actually because the DOM output of html4writer is different and needs different CSS rules 👍 So I think I can grasp it now: We are generating HTML5 outputs using the html4writer.

After adding this issue, I noticed that we already wrote into our roadmap that html4writer is dropped in 2.0: https://sphinx-rtd-theme.readthedocs.io/en/stable/development.html#release-2-0-0

So I think this issue actually is already in the roadmap? Seems good to get rid of html4writer? We are deprecating old Sphinx versions anyways and Sphinx 2.0 uses html5writer as default.

@agjohnson
Copy link
Collaborator

So I think this issue actually is already in the roadmap?

Yup! Though I think we can discuss tweaking the order of operations, as it probably makes sense to split these deprecations up, though we could decide to do everything at once too.

If we split it up, we'd want Sphinx deprecation to happen first, otherwise there will be a release that throws an error when using Sphinx 1.x + the (default) html4 writer. Removing Sphinx 1.x from our concerns should remove nearly all of the usage of the html4 writer (in theory).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Design or UX/UI related Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants