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
Incompatibilities with search and Sphinx 5.x #8871
Labels
Comments
stsewd
added a commit
that referenced
this issue
Mar 3, 2022
Ref #8871. To make this work we require: - sphinx-doc/sphinx#10153 - sphinx-doc/sphinx#10233 - A new release of https://github.com/readthedocs/readthedocs-sphinx-ext
stsewd
added a commit
that referenced
this issue
Mar 3, 2022
Ref #8871. To make this work we require: - sphinx-doc/sphinx#10153 - sphinx-doc/sphinx#10233 - A new release of https://github.com/readthedocs/readthedocs-sphinx-ext
stsewd
added a commit
that referenced
this issue
Mar 3, 2022
Ref #8871. To make this work we require: - sphinx-doc/sphinx#10153 - sphinx-doc/sphinx#10233 - A new release of https://github.com/readthedocs/readthedocs-sphinx-ext
25 tasks
FWIW, this also affects Sphinx 4 sites. |
@pradyunsg do you have a link to those projects? What are the problems present on 4.x? |
We have deployed the fixes, search is working as expected on sphinx's master docs https://www.sphinx-doc.org/en/master/search.html?q=test For search to work on our theme we only need to wait for sphinx-doc/sphinx#10153 to get merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
TL;DR: Sphinx is dropping jQuery sphinx-doc/sphinx#10028 and those changes break our custom server side search feature (SSS), and it also breaks the search page on our theme (with or without our SSS).
The first problems were reported at #8862.
Seach.init
is being called twice, duplicating some elements (will be fixed with Search: correctly remove Search.init on Sphinx >= 5.0 readthedocs-sphinx-ext#100)Our custom js that replaces Sphinx's default search with our SSS assumes that some attributes from the
Search
object are jQuery objects, but after the refactor done upstream, they will be HTML/Node objectWe need to check all usages of
Sphinx.*
and decide what to do.readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/search.js
Line 209 in e7102a4
All calls to
Sphinx.{element}.text()
should be easy to fix, we can have a utility function that checks if the element is an jQuery object, something likereadthedocs.org/readthedocs/core/static-src/core/js/doc-embed/search.js
Lines 209 to 211 in e7102a4
I think we can delete that call to
.fadeIn
, it's done at the end of the search operation, so at this stage the text fromSphinx.status
is already shown. Actually, I think this was meant to beSphinx.title.fadeIn
.readthedocs.org/readthedocs/core/static-src/core/js/doc-embed/search.js
Lines 191 to 192 in e7102a4
list_item
is a jQuery object (we are making heavy use of jQuery to create that element), but nowSearch.output
will be an HTML/Node object, we can do something like this:If some expected elements aren't on the search page, the search operation will fail.
https://github.com/sphinx-doc/sphinx/blob/3b01fbe2adf3077cad5c2cf345c6d000d429d7ac/sphinx/themes/basic/static/searchtools.js#L211-L213
Our theme completely overrides the search page, and it doesn't inherit from Sphinx's default search page
https://github.com/readthedocs/sphinx_rtd_theme/blob/d64dadf1ceec4f9ff6c1ca2d3ea4c3d0fdb0e8d2/sphinx_rtd_theme/search.html#L10
When retrieving a non-existent element, jQuery would still return an object (kind of empty one, so method calls won't raise a null exception), but now
getElementById
will returnnull
and raise an exception when trying to call a method on that value.We can fix this in our theme (by adding that element or by inheriting from sphinx's search page to avoid things like this in the future),
or we can also send a patch to sphinx to not assume that that element will always be present (it's used in 2 places only, so it shouldn't be a problem) Searchtools: don't assume that all themes define some elements sphinx-doc/sphinx#10153.
Another approach
Another way to solve most of these problems is to stop relying on sphinx's
Search
object and create and display the elements by ourselves, the mkdocs implementation doesn't rely on any mkdocs internals to display the search results.This way, if sphinx changes the internals of the
Search
object, we won't be affected.A downside would be that the structure/classes of the containers wouldn't be dictated by sphinx anymore,
so if our structure is different from the one expected for that version of sphinx, the styles may look off if the theme doesn't play nice with our structure.
Related issues
Those are the issues I have found so far, additionally Sphinx will soon be dropping jQuery too (and we make heavy use of jQuery in our injected footer and friends).
The text was updated successfully, but these errors were encountered: