Why are some theme-specific HTML elements (with CSS classes) inserted?

[mgeier]

Details

The file https://github.com/readthedocs/readthedocs.org/blob/master/readthedocs/core/static/core/js/readthedocs-doc-embed.js contains some JavaScript code that inserts a bunch of HTML elements with CSS classes, for example:

<div class='wy-table-responsive'></div>

It looks like this is used in the sphinx_rtd_theme, but there is a separate JS file over there: https://github.com/readthedocs/sphinx_rtd_theme/blob/71d6d182cfeb7872c2399eac4a47d2f3c6b2c7d1/src/theme.js#L102-L103

Why is this theme-specific manipulation done, even if another theme is used?

Expected Result

No HTML/CSS manipulations unless they are related to the RTD-specific site components.

Especially no theme-specific manipulations.

Actual Result

Site elements that are not provided by RTD (e.g. tables in the main content) are manipulated.

[stsewd]

I think this comes from here

https://github.com/readthedocs/readthedocs.org/blob/135025f2a40ad7dfbaabda1261ee048a84e44a72/readthedocs/core/static-src/core/js/doc-embed/sphinx.js#L39-L52

That calls .enable from

https://github.com/readthedocs/sphinx_rtd_theme/blob/71d6d182cfeb7872c2399eac4a47d2f3c6b2c7d1/src/theme.js#L18

[mgeier]

It looks like the JS file is included in the generated HTML file like this:

<script async="async" src="https://assets.readthedocs.org/static/javascript/readthedocs-doc-embed.js"></script>

What's the process for removing the offending code from there?

[ericholscher]

If we are editing tables in other themes, that is definitely a bug. That code should be scoped more cleanly than that, but there might be something you're doing similar to the theme that is caught somewhat. We'll have to look into it to understand more what is happening.

The file that is being rendered is here: https://github.com/readthedocs/readthedocs.org/blob/d6f7347f82d7b3e210cd7e7e84d860282dd029f8/readthedocs/core/static-src/core/js/readthedocs-doc-embed.js as a place to start.

[ericholscher]

@mgeier Can you please link to the docs where this is happening?

[stsewd]

@ericholscher from my comment https://github.com/readthedocs/readthedocs.org/issues/8497#issuecomment-920437964, that code is exported from our theme, so not sure if we can scope it without breaking our theme. I think we shouldn't export that code from our theme at all, but looks like that was done for compatibility with other themes? We could add a feature flag that skips that insertion for new projects or something like that.

[agjohnson]

The underlying issue is that we are calling enable() on the theme exported code (for badge only flyout on non-sphinx-rtd-theme themes), but there is sphinx-rtd-theme specific code in the theme's init() function.

There are several calls that happen in the theme init() (called by the theme's enable() function) related to table display, and those should only happen when the the theme used by sphinx is a derivative of sphinx_rtd_theme. I believe these calls are:

https://github.com/readthedocs/sphinx_rtd_theme/blob/71d6d182cfeb7872c2399eac4a47d2f3c6b2c7d1/src/theme.js#L101-L123

We don't have a way to turn this off currently. A feature flag doesn't quite solve this, there are some functions in there specifically for the badge only version of the flyout that are valid, but the table selector calls are definitely not required for all themes.

The best options are probably to resolve this in sphinx_rtd_theme 1.0.1/1.1.0, alter how we are calling the theme code from RTD embed.js, or both in some fashion most likely.

[mgeier]

@ericholscher

@mgeier Can you please link to the docs where this is happening?

I've noticed the unexpected behavior in https://github.com/mgeier/insipid-sphinx-theme/pull/61, a concrete example page would be https://insipid-sphinx-theme.readthedocs.io/en/0.2.8/showcase/tables.html, where all tables are wrapped with <div class="wy-table-responsive">, even though no RTD-derived theme is used.

The HTML file contains (as I mentioned above):

<script async="async" src="https://assets.readthedocs.org/static/javascript/readthedocs-doc-embed.js"></script>

This JS file seems to contain the offending code:

i("table.docutils:not(.field-list,.footnote,.citation)").wrap("<div class='wy-table-responsive'></div>")

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[humitos]

The best options are probably to resolve this in sphinx_rtd_theme 1.0.1/1.1.0, alter how we are calling the theme code from RTD embed.js, or both in some fashion most likely.

@agjohnson was this solved in this version?

@nienn should this issue be closed and tracked directly in https://github.com/readthedocs/sphinx_rtd_theme/issues/1239 instead?

[stsewd]

should this issue be closed and tracked directly in https://github.com/readthedocs/sphinx_rtd_theme/issues/1239 instead?

This problem needs to be fixed in both sides. We are calling code from the theme from our side and the theme has rtd specific code.

[mgeier]

Until recently, this was just an invisible quirk, but now it has a visible effect on one of my projects, see e.g. https://python-sounddevice.readthedocs.io/en/0.4.5/api/convenience-functions.html The vertical padding of the "Overview" block at the top of the page changes when the RTD stuff is inserted.

Is there a plan to avoid this hostile injection? Is there any progress on https://github.com/readthedocs/sphinx_rtd_theme/issues/1239 which might help? Is there anything I can do to help?

[agjohnson]

The plan I mentioned above is still valid, we just haven't had the bandwidth to take on this work in our roadmap. This code pattern is definitely not ideal, technically speaking, but the overall impact from this is quite limited.

The fix above describes splitting up the logic in the theme code into two separate chunks. One that executes for all projects, and one that only executes if the project is using sphinx_rtd_theme or a derivative theme of sphinx_rtd_theme. The code change is not large, but it is a bit of work to test and develop the change, as this requires working builds on a working RTD development instance.

We'd be happy to see this change, but warning: it will be a bit of work to test the changes.

Otherwise, I think we can consider it for the next theme release. The 1.1 release was meant to be a minor release, but had some extra bug fixes that pushed out release back a bit. The following release will likely be another bug fix release

[mgeier]

Are there any news on this?

@agjohnson, you were talking about release 1.1 ... now we seem to be at 1.2.0, can this now be fixed more easily?

[agjohnson]

@mgeier So, we have still not prioritized this particular issue internally, but we are also actively working on changing our approach to injecting all of our features into documentation project (including all of this JS). So, what we're working on now would resolve the issue here, but in a roundabout sort of way.

We're focusing on replacing basically all of the documentation JS, but this is a long term project. We don't yet have a flyout reimplementation available yet in this code, but when there is this new library would likely be usable in order to avoid this behavior.

[humitos]

Just want to make a note here that we are re-writing these javascript (mainly, readthedocs/core/static/core/js/readthedocs-doc-embed.js) completely in https://github.com/readthedocs/addons and this problem should be fixed once we roll it out more broadly. We are working on its initial version and we plan to start rolling it out for projects using the beta build.commands config key first.

[humitos]

I have some updates here. We have published the addons in beta (read more at https://blog.readthedocs.com/addons-flyout-menu-beta/). You can opt-in by using build.commands config key or by using enabling the beta addons in your project using the beta dashboard https://beta.readthedocs.io/ and going to your project's admin panel.

Once you enable the new beta addons, this issue should be solved in your project since none of these HTML/CSS code is injected anymore. The addons work by using isolated WebComponents with their own shadow DOM, which should not affect the original DOM.

@mgeier can you please enable the new beta addons in your project and confirm this is solved with that?

[humitos]

@mgeier Hi 👋🏼 . I would like to have your feedback here after enabling the addons since I think this problem is solved after enabling them. Have you had a change to jump into this?

[mgeier]

Sorry for my lack of response. I was quite busy lately ...

I would like to have your feedback here after enabling the addons

I'm hesitant to do that, because I don't know what will happen with the flyout ... will enabling the addons break my custom positioning?

the beta dashboard https://beta.readthedocs.io/

This seems to work instead: https://beta.readthedocs.org/

[mgeier]

OK, so I created a dummy copy for experimentation: https://just-testing-the-insipid-sphinx-theme.readthedocs.io/

And indeed, this destroys my custom positioning of the flyout (which I kinda expected). I will not enable this in my "real" projects for now.

But more relevant to the issue at hand, I can confirm that this does indeed not insert the <div> element around tables I was bickering about.

[humitos]

Perfect! Thanks for your feedback here.

We are moving towards gradually migrating projects into the new beta addons, since the "old way" is getting deprecated and we plan to not inject any kind of magic into the build process anymore to avoid unexpected behaviors.

Ideally, the custom flyout positioning should be covered by the new beta addons as well. I've been working on this in the last months but I didn't get too much feedback about it from theme authors yet, so I didn't move forward with that yet. I commented about this in another issue as well: https://github.com/readthedocs/readthedocs.org/issues/9134#issuecomment-1891916958

I'm happy to talk to you more if you think the work I'm doing could be useful for your theme 👍🏼

[humitos]

But more relevant to the issue at hand, I can confirm that this does indeed not insert the <div> element around tables I was bickering about.

Excellent! I'm closing this issue then. Thanks for your time!