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

sphinx_rtd_theme.css disappears if html_context['css_files'] is used #2116

Closed
mgeier opened this issue Apr 6, 2016 · 11 comments
Closed

sphinx_rtd_theme.css disappears if html_context['css_files'] is used #2116

mgeier opened this issue Apr 6, 2016 · 11 comments
Labels
Support Support question

Comments

@mgeier
Copy link
Contributor

mgeier commented Apr 6, 2016

I'm using this in my conf.py:

html_context = {
    'css_files': ['_static/custom.css'],
}

This looks harmless, but it somehow causes RTD not to include this into the HTML pages:

<link rel="stylesheet" href="https://media.readthedocs.org/css/sphinx_rtd_theme.css" type="text/css" />

Here's an example: http://nbsphinx.readthedocs.org/en/readthedocs-theme/
[Note for future readers: in the future the problem will be hopefully gone!]

For comparison, here's a version without css_files, where sphinx_rtd_theme.css is included correctly: http://nbsphinx.readthedocs.org/en/0.2.5/

IIRC, all this was still working fine one or two weeks ago ...

Here's a screenshot how a broken page looks like: jupyter/notebook#1305

@agjohnson
Copy link
Contributor

So, this is something that we could protect against in https://github.com/rtfd/readthedocs-sphinx-ext, but the fact that you are overriding the context passed into the template is the real culprit. If you are trying to add a css_file, this isn't the prescribed method. You can add to css_files by running your conf.py as an extension -- that is, having a setup method:

def setup(app):
    app.add_stylesheet('file.css')
    app.add_javascript('file.js')

@agjohnson agjohnson added the Support Support question label Apr 8, 2016
@mgeier
Copy link
Contributor Author

mgeier commented Apr 9, 2016

Thanks, that works!

But this cannot be the proper solution, that's clearly a work-around.

Why should I not be allowed to use html_context?
This is a user option, there's even a command line option for that!

I've read readthedocs/sphinx_rtd_theme#117 (comment), but that doesn't give any reasons ...

And why whould it work locally but not on RTD?

Can you please point me to the code that is supposed to add the CSS file on RTD?

I'm not saying that there's a bug in RTD, it might as well be a bug in Sphinx or somewhere else, but a bug it definitely is!

@agjohnson
Copy link
Contributor

I don't think this is a bug anywhere, as the outcome is exactly as I'd expect. You are explicitly redefining css_files on the html_context that gets passed into the template processor, I wouldn't expect our additions to css_files to be in the list with your override.

You can still use html_context, but you need to append to css_files, not redefine. The proper way to do this is to configure the builder, via a setup function -- this is a standard configuration pattern.

It's working locally for you because you aren't building docs using our extension and related build overrides.

@mgeier
Copy link
Contributor Author

mgeier commented Apr 12, 2016

I disagree.

Using a setup() function is not a standard configuration pattern, it's rather a very specialized means for advanced users (or at least it should be).

The standard configuration pattern is to assign values to the options mentioned in http://www.sphinx-doc.org/en/stable/config.html.

It's the extensions that should honor the user settings and append (or rather prepend) to and not redefine those.

I had a look at the RTD extension, and I found this (around line 75):

app.builder.css_files.insert(0, theme_css)

I don't know exactly what's happening, but this looks like the intention was to insert the CSS file in the beginning of the list (which seems to make sense), but somehow this has no effect if html_context['css_files'] is defined in conf.py.

Can anyone explain why this happens?

Interestingly, the same thing (which is IMHO the wrong thing) happens when using app.add_stylesheet().

@mgeier
Copy link
Contributor Author

mgeier commented Apr 12, 2016

I've created a Sphinx issue to find out about the intended behavior: sphinx-doc/sphinx#2442

@agjohnson
Copy link
Contributor

I've described why above: you are overriding the context variable passed into the template processor -- at the very last point before templating. Normally, app.builder.css_files ends up in this context variable, but you are instead overriding it explicitly.

Sphinx does not imply you should override html_context to add css files, the docs mention it is simply an option to pass in variables to the template processor.

The option to override is there nonetheless, and you are free to use it, but the outcome is exactly what you describe -- you are overriding other attempts to add to css_files.

@benjaoming
Copy link
Contributor

I agree with @mgeier : Locally, the RTD theme builds correctly:

on_rtd = os.environ.get('READTHEDOCS', None) == 'True'

if not on_rtd:  # only import and set the theme if we're building docs locally
    import sphinx_rtd_theme
    html_theme = 'sphinx_rtd_theme'
    html_theme_path = ['.', sphinx_rtd_theme.get_html_theme_path()]

html_context = {
    'css_files': [
        '_static/theme_overrides.css',  # override wide tables in RTD theme
    ],
}

This will create a nice, functional local build - and then break on the RTD server. I think my configuration is intuitive and to the point.

benjaoming added a commit to benjaoming/kolibri that referenced this issue Aug 21, 2017
…d theme_overrides.css without overriding the entire theme when on remote RTD server

See:
readthedocs/readthedocs.org#2116
readthedocs/sphinx_rtd_theme#432
@mgeier
Copy link
Contributor Author

mgeier commented Aug 23, 2017

@benjaoming Thanks for your support! Can you please also add your opinion to sphinx-doc/sphinx#2442? Because if I'm not mistaken, that should solve the issue here.

@benjaoming
Copy link
Contributor

@mgeier sounds like sphinx-doc/sphinx#2442 is the root cause?

@mgeier
Copy link
Contributor Author

mgeier commented Aug 23, 2017

@benjaoming Yes, I think so. But it looks like my issue didn't convince the Sphinx people to do something about it ...

jdknight added a commit to sphinx-contrib/confluencebuilder that referenced this issue Mar 30, 2018
Instead of performing theme corrections using the 'css_files'
options, use the application instance's `add_stylesheet` instead.
This prevents issues where overriding the theme affects ReadTheDoc's
theme [1].

[1]: readthedocs/readthedocs.org#2116

Signed-off-by: James Knight <[email protected]>
clenk added a commit to mitre/multiscanner that referenced this issue Apr 4, 2018
emdupre added a commit to ME-ICA/tedana that referenced this issue May 14, 2018
@stsewd
Copy link
Member

stsewd commented Jun 15, 2018

Seems like the issue was solved in the sphinx side sphinx-doc/sphinx#2442 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Support Support question
Projects
None yet
Development

No branches or pull requests

4 participants