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

Close #6241: html: Allow to add JS/CSS files to the specific page #8631

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

tk0miya
Copy link
Member

@tk0miya tk0miya commented Dec 31, 2020

Feature or Bugfix

  • Feature

Purpose

@tk0miya tk0miya added type:enhancement enhance or introduce a new feature builder:html extensions labels Dec 31, 2020
@tk0miya tk0miya added this to the 4.0.0 milestone Dec 31, 2020
@choldgraf
Copy link
Contributor

Does this happen purely "under the hood" (AKA without any extra user configuration) as long as add_js_file is used within the html context event rather than an earlier event? So that:

  • add_js_file and add_css_file before html-page-context will apply to all pages
  • the same methods called within html-page-context will only apply to the current page

Is that right?

If so, I think this is awesome :-)

Also - maybe this should be documented here: https://www.sphinx-doc.org/en/master/extdev/appapi.html#event-html-page-context ?

@tk0miya
Copy link
Member Author

tk0miya commented Dec 31, 2020

Yes, you're right.

Also - maybe this should be documented here: https://www.sphinx-doc.org/en/master/extdev/appapi.html#event-html-page-context ?

Thanks. I'll update it too.

@choldgraf
Copy link
Contributor

choldgraf commented Dec 31, 2020

Should probably also be mentioned here:

(or cross-referenced). Otherwise I suspect users will be confused when there's different behavior based on context that isn't documented

@tk0miya tk0miya force-pushed the 6241_assets_for_specific_page branch 2 times, most recently from e3e9bda to b197bf0 Compare December 31, 2020 11:50
@tk0miya
Copy link
Member Author

tk0miya commented Dec 31, 2020

Should probably also be mentioned here:

We're using autodoc to build out document. So the docstrings are automatically applied to the docs.

@akhmerov
Copy link

akhmerov commented Dec 31, 2020

I have two considerations that may be relevant here:

  • If I understand correctly, html-page-context is executed after the end user assets are added in conf.py. This means that the per-page additions to CSS will have a higher priority than those by the end user, and this shouldn't be the case.
  • I wonder how an end user would access this behavior? Imagine here a use case where someone writes a documentation of a library that produces rich HTML output. They are unlikely to also want to implement a sphinx extension. Would they be able to hook into the event from conf.py?

@choldgraf
Copy link
Contributor

choldgraf commented Dec 31, 2020

A quick thought from me:

I wonder how an end user would access this behavior

I think it would be possible with something like:

def add_per_page_css(<html-page-context-args>):
    if pagename in <[list-of-pagenames]>:
        app.add_css_file(<my-css-file>)

def setup(app):
    app.connect("html-page-context", add_per_page_css)

IMO this would be a useful snippet in the documentation somewhere. Maybe somewhere around https://www.sphinx-doc.org/en/master/development/theming.html?highlight=%22add_css_file%22 ?

@akhmerov
Copy link

I see, I didn't realize that one can add a setup function to conf.py. The configuration page indeed already mentions:

The configuration file itself can be an extension; for that, you only need to provide a setup() function in it.

So it doesn't need a separate action then.

@tk0miya
Copy link
Member Author

tk0miya commented Dec 31, 2020

If I understand correctly, html-page-context is executed after the end user assets are added in conf.py. This means that the per-page additions to CSS will have a higher priority than those by the end user, and this shouldn't be the case.

At present, the order of JS/CSS is not mentioned in its specification at all. So it is not defined and not controllable. Please file another issue to control the order if important. For now, I don't have a plan to implement it because I don't know how worth for Sphinx. Of course, I understand it'ss important when building interactive website. But Sphinx is mainly used for the static pages.

@tk0miya
Copy link
Member Author

tk0miya commented Dec 31, 2020

I wonder how an end user would access this behavior? Imagine here a use case where someone writes a documentation of a library that produces rich HTML output. They are unlikely to also want to implement a sphinx extension. Would they be able to hook into the event from conf.py?

I don't know how many users would like to add JS/CSS conditionally. Additionally, it would be tough to design and implement a new "conditional" configuration. So I'd not like to support conditional JS/CSS via configurations.

IMO, it's not difficult to create a custom event handler in conf.py. So it's easier and simpler to use conditional assets than configurations.

@akhmerov
Copy link

Of course, I understand it'ss important when building interactive website. But Sphinx is mainly used for the static pages.

With JS the order probably doesn't matter anyway because it's loaded asynchronously. The order of CSS is however important also in static pages and whenever an extension injects its CSS after user's custom.css, users report an issue (e.g. jupyter/jupyter-sphinx#149 and executablebooks/sphinx-tabs#15). Therefore this matter seems to be within scope of sphinx since it is used for static pages.

At present, the order of JS/CSS is not mentioned in its specification at all. So it is not defined and not controllable.

While the order of CSS is not documented (I'll open an issue about that), it is in practice controllable. The two issues I linked above show that documentation authors do in fact expect being able to override the extension or theme CSS. This PR would remove such ability.

@tk0miya
Copy link
Member Author

tk0miya commented Dec 31, 2020

Absolutely. I perfectly understand why the order of CSS is important. We have to add a priority feature to CSS API first.

@tk0miya
Copy link
Member Author

tk0miya commented Jan 1, 2021

Now I posted #8639 to control the order of JS/CSS. It helps that the JS/CSS files registered at the html-page-context event is loaded before user's JS/CSS by default. And extensions can register their JS/CSS files before or after other assets intentionally.

@akhmerov
Copy link

akhmerov commented Jan 2, 2021

Thanks, with #8639 in place, I think everything is perfect.

A question about milestones: this one seems to go into 4.0.0 while #8639 was tagged 3.5.0: is this intentional?

@tk0miya
Copy link
Member Author

tk0miya commented Jan 2, 2021

Yes. I thought this would be a breaking change. So it would be better to merge this into 4.0. But I'm debating now because it's a very minor change.

@tk0miya tk0miya force-pushed the 6241_assets_for_specific_page branch 2 times, most recently from 88190bd to 4d54a41 Compare January 2, 2021 17:42
@choldgraf
Copy link
Contributor

choldgraf commented Jan 2, 2021

I think it’d be great if this were in 3.5. I feel like it is a quite “under the hood” change - the “breaking” effect would only affect people who were

  • using these methods in the html-page-context event
  • and expecting it to apply to all pages instead of one page.

I suspect that this is a pretty small group of people...perhaps we could just log an info statement saying that the behavior has changed or something?

@tk0miya tk0miya changed the base branch from master to 3.x January 4, 2021 13:30
@tk0miya tk0miya modified the milestones: 4.0.0, 3.5.0 Jan 4, 2021
@tk0miya tk0miya force-pushed the 6241_assets_for_specific_page branch 2 times, most recently from edb5a55 to 4d14317 Compare January 4, 2021 15:05
@tk0miya
Copy link
Member Author

tk0miya commented Jan 4, 2021

Thank you for comment. Finally, I determined to merge this into the 3.x branch.

…c page

Allow to add JS/CSS files to the specific page when an extension calls
`app.add_js_file()` or `app.add_css_file()` on `html-page-context`
event.
@tk0miya tk0miya force-pushed the 6241_assets_for_specific_page branch from 4d14317 to af4e615 Compare January 6, 2021 17:12
@tk0miya tk0miya merged commit ca9342c into sphinx-doc:3.x Jan 7, 2021
@tk0miya tk0miya deleted the 6241_assets_for_specific_page branch January 7, 2021 16:11
@humitos
Copy link
Contributor

humitos commented Apr 16, 2021

Is there a way to configure this to behave as it was? I mean, including all the JS and CSS files in all the pages even if they "are not used" in that page?

I'm asking because in my extension sphinx-hoverxref we show a small tooltip with the content of another page from the same documentation. That content could have a math expression that requires MathJax to be render.

Example:

This is because the page usage.html does not include the mathjax.js with the changes in this PR.

Edit: Note that using Sphinx<3.5 my problem disappears.

humitos added a commit to readthedocs/sphinx-hoverxref that referenced this pull request Apr 16, 2021
Sphinx 3.5.x includes a feature to only include the JS and CSS on the pages that
they are used. This conflicts when we render content that uses MathJax, since
the page that shows the tooltip does not has MathJax loaded, but the content
rendered inside the tooltip requires it to work.

See sphinx-doc/sphinx#8631
humitos added a commit to readthedocs/sphinx-hoverxref that referenced this pull request Apr 16, 2021
Sphinx 3.5.x includes a feature to only include the JS and CSS on the pages that
they are used. This conflicts when we render content that uses MathJax, since
the page that shows the tooltip does not has MathJax loaded, but the content
rendered inside the tooltip requires it to work.

See sphinx-doc/sphinx#8631
@humitos
Copy link
Contributor

humitos commented Apr 16, 2021

I think that I will have the same problem with other 3rd party extensions start using this feature as well. For example in https://sphinx-hoverxref.readthedocs.io/en/latest/usage.html#tooltip-with-sphinx-tabs if you hover "tooltip with Sphinx Tabs" you will see a bigger tooltip that renders tabs ("from PyPI" and "from GitHub") that won't work when sphinx-tabs extension uses this feature to only include their JS files on the "pages that are used".

@tk0miya
Copy link
Member Author

tk0miya commented Apr 16, 2021

@humitos Could you send a feature request, please? Then I'll consider it later.

@humitos
Copy link
Contributor

humitos commented Apr 19, 2021

Great, thanks! I've created the issue at #9115

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builder:html extensions type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants