djangodocs Sphinx extension incompatible with other themes

[Mike Edmunds]

The docs/_ext/djangodocs.py catchall Sphinx extension has three problems that make it difficult or impossible to use with other Sphinx themes. These need to be fixed before the docs/_theme/djangodocs Sphinx theme can be replaced with a third-party, well-maintained theme as proposed in #37138.

1. Outdated workaround to remove table borders crashes other themes

The DjangoHTMLTranslator.visit_table() and depart_table() overrides are working around <table border=1> created by the docutils html4 writer. But Sphinx has used the html5 writer (which doesn't emit the obsolete border attribute) since 2.0. The workaround depends on undocumented Sphinx internal attrs and causes an IndexError when used with (e.g.,) sphinx-pydata-theme.

It can and should be removed.

Edit: similar problem with DjangoHTMLTranslator.visit_desc_parameterlist(), which was meant to prevent <big>(</big> in parameter lists. Sphinx hasn't done that in ages, and the override regularly breaks with new Sphinx versions (and prevents use of Sphinx's multi_line_parameter_list option).

2. console-tabs implementation creates asymmetric DOM

The console-tabs renders structurally different html for the Unix/macOS tab:

<section class="c-content-unix" id="c-content-0-unix">
  <div class="highlight-console notranslate">
    <div class="highlight">...</div>
  </div>
</section>

… compared to the Windows tab:

<section class="c-content-win" id="c-content-0-win">
  <div class="highlight">...</div>
</section>

Notice the expected wrapper <div class="highlight-doscon notranslate">) is missing on the Windows tab. Although this doesn't matter for the djangodocs and djangoproject.com themes (or they have specific workarounds?), it affects spacing in themes like furo and pydata-sphinx-theme. (It would also break a Pygments style that had special rules for the doscon lexer, and it might result in translation tools treating the Windows command shell syntax as English text to be translated.)

One solution is to borrow Sphinx's own machinery to render the Windows literal block, rather than calling the Pygments highlighter directly as now. This would also also simplify the code a bit.

3. console-tabs extension is tightly coupled to djangodocs theme templates

The console-tabs extension relies on a console-tabs.css file that is conditionally included in docs/_theme/djangodocs/layout.html based on context provided by the extension. That doesn't work for third-party themes with their own layout templates. (Or requires maintaining a custom theme inheriting from the third party.)

We should extract console-tabs.css from the djangodocs theme and make it available in the djangodocs extension for any theme. Options:

1. Move console-tabs.css to the project Sphinx static dir (docs/_static) and add it to all pages in docs/conf.py, whether or not they contain console tabs. (Note that docs.djangoproject.com essentially already does this with its customized console-tabs.scss.)
2. Move console-tabs.css to docs/_static but inject it conditionally only in pages that need it in the djangodocs extension. (Extensions can call app.add_css_file() in page-specific hooks since Sphinx 3.5.)
3. Move console-tabs.css to docs/_ext/static and make the djangodocs extension responsible for copying it into the static output. (Requires Sphinx 9.1 or later for app.add_static_dir() support in extensions.)

Also, as part of any rework here, we should eliminate the vendored fa-brands font in the Django docs source (it's used only for console-tabs icons) and update the console-tabs rendering for improved accessibility (see #35874 stage 1)

djangoproject.com considerations

Items 1 and 2 should not affect docs.djangoproject.com.

docs.djangoproject.com will likely want to track any changes to console-tabs.css in its custom console-tabs.scss. That file is used for all recently-rendered versions on docs.djangoproject.com, and older versions are rendered with the older djangodocs Sphinx extension (unless this gets backported). If the rendered DOM for the tabs is changing significantly, we should make it possible for djangoproject.com styles to differentiate docs built with the older tabs from the new tabs (e.g., by changing class="console-tabs" to class="dj-console-tabs").

[Natalia Bidart]

Thank you Mike, this all makes sense and matches what I saw when working on a PoC for #37138. My thoughts on some the questions:

1. Agreed
2. For this I was using a custom CSS rule in console-tabs.css to cope with the different spacing, I guess that's one solution but not robust and does not scale well. I'm not familiar with "Sphinx's own machinery to render the Windows" but sounds like a sensible approach.
3. I think option 1 is the simplest and is acceptable.

[Mike Edmunds]

​https://github.com/django/django/pull/21613

[Mike Edmunds]

Also…

4. Inconsistent handling of versionchanged, versionadded, deprecated directives

The djangodocs extension implements custom .. versionadded:: and .. versionchanged:: directives and overrides Sphinx's visit_versionmodified() in html writers (which also applies to .. deprecated:: and .. versionremoved::). There seem to be three goals:

1. Show "Django N.M" rather than the generic "version N.M" in these messages.
2. Show "Added in Django Development version" rather than "Added in Django N.M" for the current in-development version (as set by conf.py django_next_version).
3. Wrap the changeset message in a <span class="title"> so it can be bolded. (This seems to be an obsolete workaround for a very early version of Sphinx's html generation.)

There are multiple problems with the current implementation:

• The DOM generated by versionadded and versionchanged is significantly different from that for deprecated (which goes through Sphinx's native domains.changeset.VersionModified directive, and outputs a <span class="versionmodified"> for all changeset directives). This results in inconsistent styling with third-party themes.
• Item b is applied only in html-based output formats. Other formats will use Sphinx's standard changeset messages.
• None of this applies to deprecated, so you will see "Deprecated since version 6.2" rather than "Deprecated since Django development version" (in the ​current 6.2 dev build).
• Sphinx 9.0 renamed all these directives to version-changed, version-deprecated, etc. The original names are still supported as aliases. Django's custom handling would not apply to any uses of the new, hyphenated changeset directive names. (And we don't have a linter to prohibit the new names.)

Also, the capitalization seems wrong—it should be "Added in Django development version" not "Added in Django Development version." And there's no handling for versionremoved (though Django's docs don't use that). And it doesn't support localized message strings (but djangoproject.com handles localization through a different mechanism, so this doesn't really matter).

I think a better approach here would be to subclass Sphinx's native VersionModified to replace the django_next_version string while retaining all standard behavior; update Sphinx's changeset label strings to use "Django" rather than "version" in all output formats; and let Sphinx generate consistent HTML for all changeset directives.

Compatibility note: nothing I can find actually relies on the <span class="title">. djangoproject.com has ​rules targeting other changeset classes, and even the local theme's djangodocs.css targets the span.versionmodified present in Sphinx's normal changeset directive output. Changing to Sphinx's native behavior should not break any styles.

[Mike Edmunds]

Replying to Mike Edmunds:

1. Show "Django N.M" rather than the generic "version N.M" in these messages.
2. Show "Added in Django Development version" rather than "Added in Django N.M" for the current in-development version (as set by conf.py django_next_version).

It's worth asking how important these behaviors are. I suspect if someone proposed adding this functionality today, it would be rejected as too complicated for too little value.

An alternative would be to remove these customizations and use Sphinx's built-in behavior:

• "Added in version 6.0" (rather than "Added in Django 6.0")
• "Changed in version 6.2" (rather than "Changed in Django development version" during 6.2 development)
• "Deprecated since version 6.2" (no change)

Of the two, (a) is considerably more complicated/fragile than (b) to maintain. (Though (b) reads awkwardly without (a): "Changed in version development version" or "Changed in version Django development version.")

[Natalia Bidart]

Thank you Mike for the PR and the analysis. I have started a deep review and I have a strong preference to use Sphinx's wording for the version annotations. I have created ​a forum post about it.

One note/question: did you consider ticket #20104 when changing/redesigning how versionchanged should be changed?

[Mike Edmunds]

Replying to Natalia Bidart:

One note/question: did you consider ticket #20104 when changing/redesigning how versionchanged should be changed?

No, I was not aware of that… and had a moment of panic that I've been using those directives wrong in my own projects.

But I think maybe that's outdated: I use change directives both with and without the blank line in other projects, and the argument/content is rendered the same either way—as part of the changenote. I'll try to figure out if Sphinx removed that restriction at some point (and if so why there's still an ​admonition about it in their docs).

[Natalia Bidart]

Replying to Mike Edmunds:

[...] had a moment of panic that I've been using those directives wrong in my own projects.

Sorry about this!

But I think that's outdated: Sphinx commit ​7630467 in Jan 2013 (Sphinx 1.2?) fixed the body-content paragraphs getting dropped. (fwiw, I use change directives both with and without the blank line in other projects, and the argument/content is rendered the same either way—as part of the changenote.)

So I robot'd this quest of debugging and apparently the issue being fixed in #20104 was a bug in Django's own directive, not in Sphinx. This is VersionDirective as it stood immediately before ​the fix:

if len(self.arguments) == 2:
    inodes, messages = self.state.inline_text(self.arguments[1], self.lineno+1)
    node.extend(inodes)
    if self.content:
        self.state.nested_parse(self.content, self.content_offset, node)
    ret = ret + messages

Note that if self.content: is nested inside if len(self.arguments) == 2:, so the blank line form silently discarded the description. That matches the data loss reported in #20104 exactly. And the Sphinx commit you found confirms it from the other side: upstream had the two conditions as siblings, we did not. We fixed a bug in our copy of the directive by banning the form that triggered it, instead of un-nesting two lines to match Sphinx.

So there is nothing here worth preserving, and I would rather not carry the restriction forward: both forms render correctly on stock Sphinx, the footgun is gone, and reinstating the check would mean keeping exactly the kind of special treatment this PR is removing. It was never applied evenly anyway, since it only ever covered versionadded and versionchanged and never deprecated. Could you please add Refs #20104 to the commit message, and include in this branch a fix for docs/internals/contributing/writing-documentation.txt, which currently mandates the blank line, so that it reads as our convention rather than as a requirement Sphinx enforces?

[Mike Edmunds]

Replying to Natalia Bidart:

[…] Could you please add Refs #20104 to the commit message, and include in this branch a fix for docs/internals/contributing/writing-documentation.txt, which currently mandates the blank line, so that it reads as our convention rather than as a requirement Sphinx enforces?

👍 will do as part of removing the custom changeset directives.