executablebooks / sphinx-tabs

Tabbed views for Sphinx
https://sphinx-tabs.readthedocs.io
MIT License
263 stars 67 forks source link

sphinx-tabs inadvertently removes css and js #183

Closed peytondmurray closed 8 months ago

peytondmurray commented 8 months ago

Describe the bug

context When I use the sphinx-tabs extension and I include css or js in specific pages, this function removes it even if I intend for it to be included.

expectation I expect sphinx-tabs to leave my included css/js alone. Rather than including sphinx-tabs assets globally and then removing them upon html-page-context if the page doesn't have any tabs directives, this package should only insert those assets if the page does have a tabs directive, not the other way around. This can also be done at html-page-context time.

bug Instead, sphinx-tabs automatically removes all css/js unless it is included on every single page.

problem This is a problem for people who want to include assets on specific pages to keep page load times down, but who also want to use tabs. Both sphinx docs and pydata-sphinx-theme docs suggest inserting css/js assets upon html-page-context, so the current behavior is extremely confusing because your assets get removed in the background, leaving you wondering how this is happening.

Reproduce the bug

Here's a small working example: https://github.com/peytondmurray/minimalsphinx/tree/per-page-css

You'll see if you run make html that the apidocs.html will only include the css if sphinx-tabs is disabled.

List your environment

No response

welcome[bot] commented 8 months ago

Thanks for opening your first issue here! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out EBP's Code of Conduct. Also, please try to follow the issue template as it helps other community members to contribute more effectively.
If your issue is a feature request, others may react to it, to raise its prominence (see Feature Voting).
Welcome to the EBP community! :tada:

foster999 commented 8 months ago

This is potentially the same as #180, or caused by the fix of that issue. What version of the package are you using?

foster999 commented 8 months ago

Thanks for including the example - this pins an earlier version, so please try it with the latest

peytondmurray commented 8 months ago

@foster999 Thanks, you're absolutely right that upgrading fixed the issue. I looked at the differences between 3.4.1 and 3.4.4 and I'm stumped as to why this now works. Here's the difference:

image

What exactly does moving the slicing operation do here? v3.4.1 has

context["css_files"] = context["css_files"][:]

which in my mind means 'set the context["css_files"] list equal to the slice containing the entire list of context["css_files"]'. In v3.4.4 the relevant lines read 'set the slice containing the entire list of context["css_files"] equal to the list of context["css_files"].' Naively I would expect the operation in either case to do nothing, basically, but I must be wrong :shrug:. I guess I'm not the only one struggling to understand this, did you ever figure it out?

foster999 commented 8 months ago

Afraid I'm still baffled by the slicing too! My feeling was that it's something to do with updating all references to the object (and nested objects inside it) vs just updating the single reference. Kinda like shallow vs deep copying. But didn't understand how that lead to the issue.

I was surprised the issue hadn't come up sooner, though it seems to be themes rather than other extensions that were affected. So maybe something to do with the order that sphinx evaluates things 🤷

peytondmurray commented 8 months ago

Thanks again for the help!