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

many older doc versions have broken JS #8323

Closed
leos opened this issue Jul 7, 2021 · 32 comments
Closed

many older doc versions have broken JS #8323

leos opened this issue Jul 7, 2021 · 32 comments
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@leos
Copy link

leos commented Jul 7, 2021

Details

Expected Result

Clicking on the version selector down in the bottom left should work. It should pop up a selector that lets me pick a version of the docs.

Actual Result

Clicking on it doesn't do anything and there's an error in the console:

(index):383 Uncaught ReferenceError: SphinxRtdTheme is not defined
    at HTMLDocument.<anonymous> ((index):383)
    at f (jquery-2.0.3.min.js:23)
    at Object.fireWith [as resolveWith] (jquery-2.0.3.min.js:23)
    at Function.ready (jquery-2.0.3.min.js:22)
    at HTMLDocument.dt (jquery-2.0.3.min.js:22)
(anonymous) @ (index):383
f @ jquery-2.0.3.min.js:23
fireWith @ jquery-2.0.3.min.js:23
ready @ jquery-2.0.3.min.js:22
dt @ jquery-2.0.3.min.js:22
readthedocs-doc-embed.js:1
Applying theme sidebar fix...

This appears to happen on old versions of docs for multiple projects that I tried. Happy to provide a few more if it's helpful. On https://django-reversion.readthedocs.io/en/latest/ the popup works correctly, but switching to one of the older versions breaks it.

@humitos
Copy link
Member

humitos commented Jul 7, 2021

Thanks for the report!

We have this chunk of JS embeded in the HTML:

  <script type="text/javascript">
      jQuery(function () {
          SphinxRtdTheme.StickyNav.enable();
      });
  </script>

SphinxRtdTheme is defined in https://media.readthedocs.org/javascript/readthedocs-doc-embed.js --which is loaded by the page, but it seems it is loaded after this code is executed.

@humitos humitos added Accepted Accepted issue on our roadmap Bug A bug labels Jul 7, 2021
@stsewd
Copy link
Member

stsewd commented Aug 4, 2021

This is coming from the theme https://github.com/readthedocs/sphinx_rtd_theme/blob/8a026194285971d25fa80d47bc1a703b4d67f60d/sphinx_rtd_theme/layout.html#L228-L232

but I don't see anything being loaded async or in the wrong order

  <script type="text/javascript" src="https://media.readthedocs.org/javascript/readthedocs-doc-embed.js"></script>
  <script type="text/javascript">
      jQuery(function () {
          SphinxRtdTheme.StickyNav.enable();
      });
  </script>

The only thing that could have made this happen would be if window is undefined.... which would be weird, I think?

https://github.com/readthedocs/sphinx_rtd_theme/blob/8a026194285971d25fa80d47bc1a703b4d67f60d/src/theme.js#L227-L235

@stsewd
Copy link
Member

stsewd commented Aug 4, 2021

The only thing that could have made this happen would be if window is undefined.... which would be weird, I think?

Looks like it's defined, so more like a race condition with the other part of the script being loaded before... but I don't understand how that would be possible with the current order

@astrojuanlu
Copy link
Contributor

Found in other projects as well, for example https://neuropsydia.readthedocs.io/

@benjaoming
Copy link
Contributor

Another example: https://django-tenant-schemas.readthedocs.io/

It's weird. When the jQuery block is executed, SphinxRtdTheme is not defined and it fails. After loading the document, SphinxRtdTheme is defined. But calling SphinxRtdTheme.StickyNav.enable() doesn't make the Flyout menu work.

@kuzmoyev
Copy link
Contributor

kuzmoyev commented Feb 14, 2023

Similar issue with the search page (e.x. gcsa with

Uncaught ReferenceError: jQuery is not defined
    at search.html?q=SendUpdatesMode&check_keywords=yes&area=default:291:7
search.html?q=SendUpdatesMode&check_keywords=yes&area=default:299 Uncaught ReferenceError: jQuery is not defined
    at search.html?q=SendUpdatesMode&check_keywords=yes&area=default:299:5

)

@benjaoming
Copy link
Contributor

@kuzmoyev thanks for adding feedback here - this project is broken because it uses Sphinx 6.x and hasn't upgraded to sphinx-rtd-theme 1.2.0. If you can contribute a fix to them, that'd be great 👍

@kuzmoyev
Copy link
Contributor

Thanks for the info. I'm the owner of the project. I don't have a cap on the sphinx and sphinx-rtd-theme versions, but for some reason, the last build was with sphinx-rtd-theme=0.5.1. Hitting rebuild resolved the issue

@benjaoming
Copy link
Contributor

@kuzmoyev haha, that makes it very easy for you to contribute to the project 💯 thanks for confirming that it works, I guess the former build from last week happened exactly before sphinx-rtd-theme 1.2.0 was released.

@spacemanspiff2007
Copy link

I'm experiencing the same issue for sml2mqtt when building with sphinx_rtd_theme 1.2.0 and sphinx 6.1.3

@benjaoming
Copy link
Contributor

benjaoming commented Mar 16, 2023

Hi @spacemanspiff2007 - thanks for reporting. Looking at your build configuration, I didn't find anything problematic.

In the HTML output[1], it appears that jquery.js has been included as the last file instead of the first. This is opposite behavior from before. I'll have to look into why that has happened.

[1] view-source:https://sml2mqtt.readthedocs.io/en/rework/index.html

edit: next time, I'll take a screenshot :) Now jQuery is gone from the output html. That's a much better explanation of what is going on. Will investigate, as the problem doesn't seem to exist in other builds.

@spacemanspiff2007
Copy link

The last build from ~14 days ago worked fine so it seems there was something introduced in that time frame.

@mgeier
Copy link
Contributor

mgeier commented Mar 16, 2023

This is also happening on https://nbsphinx.readthedocs.io/en/0.3.3/ and older.
Newer versions work fine.

@benjaoming
Copy link
Contributor

@mgeier it's the same symptom as #8323 (comment) describes. I can't see anything in the served JS that has changed, and the HTML is untouched (static). I think the issue might be browser-related. It's pretty hard to fix in this case. If anyone has access to an older browser, could be worth seeing how it works there..

@mgeier
Copy link
Contributor

mgeier commented Apr 4, 2023

I think the issue might be browser-related.

Is there a browser where it is known to work?

In all browsers I tried it consistently didn't work.

If anyone has access to an older browser, could be worth seeing how it works there..

How old are you talking?

I tried https://nbsphinx.readthedocs.io/en/0.3.3/ on browserstack.com, and it still doesn't work on those old versions:

Windows/Firefox 32
Windows/Chrome 37
Windows/Opera 23

For comparison, https://nbsphinx.readthedocs.io/en/0.3.4/ works fine on all of them.

@mgeier
Copy link
Contributor

mgeier commented Apr 20, 2023

@benjaoming Do you need any further information?

It seems to happen on all versions of all browsers ...

I can't see anything in the served JS that has changed, and the HTML is untouched (static).

I guess most of the pages' HTML is static, but the HTML injected by RTD isn't, right?

So it might be either the JS or the injected HTML?

@benjaoming
Copy link
Contributor

For comparison, https://nbsphinx.readthedocs.io/en/0.3.4/ works fine on all of them.

That's an interesting comparison!

The most important difference is that 0.3.4 has:

<script type="text/javascript" src="https://nbsphinx.readthedocs.io/en/0.3.4/_static/js/theme.js"></script>

...which 0.3.3 (broken version) doesn't. It doesn't load any theme.js file.

It seems like both readthedocs-doc-embed.js and theme.js are defining window.SphinxRtdTheme so I wonder if it's because the version in readthedocs-doc-embed.js doesn't work.

We should figure out if this is the common denominator for all the botched versions.

Other notes

Details

Stuff that's the same:

  • jQuery version
  • readthedocs-doc-embed.js has same external URL
  • the .rst-versions DOM element has an identical structure AFAICT

0.3.4 (working fine) has this call at the end of its <body>:

SphinxRtdTheme.Navigation.enable(true);

0.3.3 has this:

SphinxRtdTheme.Navigation.enableSticky();

I don't know the difference but calling SphinxRtdTheme.Navigation.enable(true) on 0.3.3 doesn't work, neither.

@saleh-unikie

This comment was marked as off-topic.

@humitos

This comment was marked as off-topic.

@humitos
Copy link
Member

humitos commented Aug 26, 2023

I think the new implementation of the addons.js could:

  • remove all the old broken integration (https://media.readthedocs.org/javascript/readthedocs-doc-embed.js, https://media.readthedocs.org/javascript/readthedocs-analytics.js, https://media.readthedocs.org/css/readthedocs-doc-embed.css, _static/readthedocs-data.js) using the same technique we are currently using
  • inject the new addons integration

This way, we will keep old version working fine without rebuilding or touching the HTML at all. cc @ericholscher what do you think about this idea for once we have deployed the new addons more broadly?

@humitos
Copy link
Member

humitos commented Sep 7, 2023

This way, we will keep old version working fine without rebuilding or touching the HTML at all. cc @ericholscher what do you think about this idea for once we have deployed the new addons more broadly?

I've already implemented this in https://github.com/readthedocs/readthedocs-ops/pull/1402 and I'm currently testing it on our test-builds project: https://test-builds.readthedocs.io/en/latest/. That URL uses the new flyout (bottom right) and removes the old flyout (integrated in the navbar at bottom left). So far, it's working great 🚀

@humitos
Copy link
Member

humitos commented Sep 7, 2023

I also opened an issue at readthedocs/ext-theme#212 to discuss the possibility to let people opt-in into the new addons (new flyout, new search as you type, and others) and also fix their old using this process. I'd appreciate feedback on that to know if people is interested.

@mgeier
Copy link
Contributor

mgeier commented Sep 10, 2023

I don't understand: will the "new flyout" replace the old integrated flyout even on old, already built page versions?

How can an author "opt in" to something without re-building all their old stuff?

I thought the idea was that old builds will just remain as they were.

AFAIU, this issue is about fixing the JS in already existing builds, not about new builds.

our test-builds project: https://test-builds.readthedocs.io/en/latest/. That URL uses the new flyout (bottom right) and removes the old flyout (integrated in the navbar at bottom left). So far, it's working great 🚀

It is working, but the content is not scrollable (see #7375). When I open the flyout on my small laptop screen, the top of it is out of view, so I cannot close it again.

@humitos
Copy link
Member

humitos commented Sep 10, 2023

I don't understand: will the "new flyout" replace the old integrated flyout even on old, already built page versions?

Yes, but not by default. At least right now. We are still in beta and testing it only on projects using "build.commands" only for now.

How can an author "opt in" to something without re-building all their old stuff?

We think we will add an option on the project's setting for this. We are working on it to expand the usage of the new addons (including the flyout), but we haven't finished this work yet.

Would you be interesting on give it a try on your projects when it's ready? Or even before that? (we have a way to enable it by request)

It is working, but the content is not scrollable (see #7375). When I open the flyout on my small laptop screen, the top of it is out of view, so I cannot close it again.

Yes. This is a bug in the current flyout addons we have already reported and we need to fix (readthedocs/addons#121)

@humitos
Copy link
Member

humitos commented Sep 14, 2023

@mgeier

It is working, but the content is not scrollable (see #7375). When I open the flyout on my small laptop screen, the top of it is out of view, so I cannot close it again.

This issue is solved in the new flyout and it will release/deployed soon. Besides, we added an event listener that when clicking outside the flyout it also closes it. See readthedocs/addons#129

@humitos
Copy link
Member

humitos commented Nov 7, 2023

I think we can close this issue since the problem is solved when enabling the new beta addons on the project. You can enable them by:

  1. Opening https://beta.readthedocs.org
  2. Going to the project's admin page
  3. Clicking on Addons (Beta)
  4. Checking the check-box
  5. Save

Screenshot:

Screenshot_2023-11-07_11-49-56

Feel free to enable the beta addons and let me know if that solves the problem on your projects. I'm happy to re-open the issue if this is not solved after enabling the beta addons. Eventually, the beta addons will be enabled by default in all the projects.

@humitos humitos closed this as completed Nov 7, 2023
@mgeier
Copy link
Contributor

mgeier commented Jan 11, 2024

I wouldn't consider this solved, since this needs intervention by project maintainers.

Many old pages are unmaintained (but still useful to readers), and those are still broken.

@humitos
Copy link
Member

humitos commented Jan 15, 2024

I understand. I'm not sure what we could do to fix issues on unmaintained projects. There is a solution already, but it requires maintainers to enable new beta addons or to re-build their project with newer dependencies.

@mgeier
Copy link
Contributor

mgeier commented Feb 3, 2024

I'm not sure what we could do to fix issues on unmaintained projects.

I don't know either.
But it did work in the past, so there must be something you can do from your side without intervention for individual project maintainers.

Breaking those old pages damages the trust in the service RTD provides, at least it does for me.
(of course I'm not entitled to anything, since I'm using the free tier)

@humitos
Copy link
Member

humitos commented Feb 5, 2024

Currently, we are in a transition. Read the Docs Addons is in beta at the moment and we can't enforce projects to use it, and that's why we are giving users the ability to opt-in to give it a try. The beta Read the Docs Addons will eventually replace the old integration when it gets more mature.

Enforcing using the Read the Docs Addons while they are in beta on all old projects is a big risk and we want to avoid it for now until we are sure it's going to work for most of the projects.

@mgeier
Copy link
Contributor

mgeier commented Feb 7, 2024

The beta Read the Docs Addons will eventually replace the old integration when it gets more mature.

What does "replace the old integration" mean exactly?
Will it "throw away the old integration and replace it with the generic bottom right badge"?
Or will it "restore the original functionality" somehow?
Or something else?

@humitos
Copy link
Member

humitos commented Feb 13, 2024

@mgeier

What does "replace the old integration" mean exactly?

With the introduction of the Read the Docs Addons, we already implemented the "replace the old integration" code. Basically, it removes the old HTML tags for the flyout and adds the new js implementation (see #8323 (comment))

Will it "throw away the old integration and replace it with the generic bottom right badge"?

Yes and no. Ideally, it will throw away the old integration (using the method described in the comment linked before) and use the new addons. Those themes that integrates the new addons will show the flyout whenever they want (example on Read the Docs Sphinx theme, and those that don't will show the flyout at the bottom right.

Or will it "restore the original functionality" somehow?

The "original functionality" will be eventually completely removed.


Let me know if this clarifies the work and our path. I would also appreciate any feedback you may have for the proposed integration with the themes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug A bug
Projects
Status: Done
Archived in project
Development

No branches or pull requests

9 participants