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

Jump to bottom

add_css_file adds file twice. #9267

Closed
cblegare opened this issue May 24, 2021 · 9 comments
Closed

add_css_file adds file twice. #9267

cblegare opened this issue May 24, 2021 · 9 comments
Labels
builder:html html theme type:bug
Milestone
4.2.0

Comments

@cblegare
Copy link

Describe the bug

While playing at building a theme, I noticed that when I use sphinx.application.Sphinx.add_css_file, the file gets added twice in the generated HTML.

To Reproduce
Steps to reproduce the behavior:

Create a new theme and setup it with the following setup function

def setup(app):
    app.require_sphinx("3")

    theme_path = Path(__file__).parent.resolve()
    app.add_html_theme("mytheme", str(theme_path))

   # This call behaves wierdly
    app.add_css_file("basic.css")

add also the theme conf file:

[theme]
inherit = basic
stylesheet = mytheme.css

[options]

The whether or not I'm using add_css_file("basic.css"), the generated HTML/_static folder contains

  • mytheme.css
  • basic.css

in either case.

With add_css_file("basic.css"), the HTML <head> looks like (comment added for emphasis on duplicated link tag)

<head>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <meta name="generator" content="Docutils 0.17.1: http://docutils.sourceforge.net/" />

    <title>A theme &#8212; mytheme 0.0.1 documentation</title>
    <link rel="stylesheet" type="text/css" href="_static/pygments.css" />
    <link rel="stylesheet" type="text/css" href="_static/mytheme.css" />
    <!-- HERE notice there are two -->
    <link rel="stylesheet" type="text/css" href="_static/basic.css" />
    <link rel="stylesheet" type="text/css" href="_static/basic.css" />
    <script data-url_root="./" id="documentation_options" src="_static/documentation_options.js"></script>
    <script src="_static/jquery.js"></script>
    <script src="_static/underscore.js"></script>
    <script src="_static/doctools.js"></script>
    <link rel="index" title="Index" href="genindex.html" />
    <link rel="search" title="Search" href="search.html" />
  </head>

Without add_css_file("basic.css"), the _stagic/basic.css link tag is added 0 times.

Expected behavior

I am not sure was would be the expected behavior. Should the parent theme CSS always be added implicitely? I am not sure. But having it linked twice seams weird to me.
@leotrs
Copy link

leotrs commented Aug 11, 2021 •
edited

I'm having the same problem with add_js_file, in particular with the html builder. I am developing a custom theme with a setup function that calls app.add_js_file. This method calls self.registry.add_js_file and then calls self.builder.add_js_file. However, the builder has another method called init_js_files which includes the following excerpt:

for filename, attrs in self.app.registry.js_files:
    self.add_js_file(filename, **attrs)

Since app.add_js_file already called put the js file in the registry AND called self.builder.add_js_file, every file is being added twice.

The workaround for now is to call app.registry.add_js_file from my setup() function and then let the builder process the files found there when it calls init_js_files. However, this bug should be addressed as the official documentation mentions app.add_js_file is the correct way of adding js files.

@tk0miya tk0miya added this to the 4.2.0 milestone Aug 11, 2021
@tk0miya
Copy link
Member

tk0miya commented Aug 11, 2021

I found the reason. CSS/JS files added at setup() of theme will be added twice unexpectedly. It must be a bug.

@mgeier
Copy link
Contributor

mgeier commented Aug 11, 2021

Regardless of the discussed error, I think it is not good to rely on add_css_file() in a theme. Ideally, a theme should be usable by just copying the theme directory somewhere and setting html_theme_path.

There might be a better way to do this (please let me know!), but I've done it like this: https://github.com/mgeier/insipid-sphinx-theme/blob/3336d6b50f5a7c7d18e6efc3a974c4b2e2cc3a60/src/insipid_sphinx_theme/insipid/layout.html#L50-L56

This way, it is not necessary to install the theme as an extension.

@leotrs
Copy link

leotrs commented Aug 11, 2021

@mgeier I am not installing my theme as an extension. The only change in my conf.py is to set the value of html_theme. The theme itself has its own setup function that the user need not interact with or even know about.

Furthermore, the official documentation recommends add_js_file for theme developers.

@tk0miya
Copy link
Member

tk0miya commented Aug 14, 2021

Yes, it is one of the officially recommended ways to install JS files. It helps to distribute the HTML theme as a python package.

@leotrs
Copy link

leotrs commented Aug 14, 2021

Did you read what I said upthread?

@tk0miya
Copy link
Member

tk0miya commented Aug 14, 2021

Hmm? What do you mean?

@leotrs
Copy link

leotrs commented Aug 14, 2021

The recommend method is broken.

@tk0miya
Copy link
Member

tk0miya commented Aug 14, 2021

Yes. The goal of this issue is to fix it.

@tk0miya tk0miya mentioned this issue Aug 14, 2021
Merged
tk0miya added a commit that referenced this issue Aug 16, 2021
@tk0miya
Merge pull request #9546 from tk0miya/9267_JS_loaded_twice
8fd4373 
Fix #9267: html theme: CSS and JS files added by theme were loaded twice
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
builder:html html theme type:bug
Projects
None yet
Milestone
4.2.0
Development

No branches or pull requests
4 participants
@mgeier @tk0miya @leotrs @cblegare