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

Search: default Sphinx's search doesn't work when addons is enabled #213

Closed
humitos opened this issue Dec 11, 2023 · 4 comments · Fixed by readthedocs/readthedocs-sphinx-ext#131
Assignees
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@humitos
Copy link
Member

humitos commented Dec 11, 2023

The default Sphinx's search works fine on this project https://jdaviz.readthedocs.io/en/latest/search.html?q=imviz (the same happens with https://docs.astropy.org/en/latest/search.html?q=test). However, if the addons are enabled on that project, it fails. The JS console doesn't report anything, so I'm not sure yet what's the exact problem.

See astropy/astropy#15459 for more information.

@humitos
Copy link
Member Author

humitos commented Dec 12, 2023

Using #217 approach, I tested this with the official Sphinx's documentation and I was able to reproduce the issue. I noticed some JS conflict, but I'm not 100% where it is.

When loading search.html?q=testing:

  • without addons, the testing query is automatically written in the search input
  • with addons enabled, the search input is empty

@humitos
Copy link
Member Author

humitos commented Dec 12, 2023

Actually, not being able to populate the input field is the main reason why it doesn't work.

The Sphinx code gets the q= attribute from the URL and writes it down in all the inputs. After that, it performs the search calling Search.performSearch(query): https://github.com/sphinx-doc/sphinx/blob/35965903177c6ed9a6afb62ccd33243a746a3fc0/sphinx/themes/basic/static/searchtools.js#L174

@humitos
Copy link
Member Author

humitos commented Dec 12, 2023

I found the issue! 🎉

The problem is our readthedocs-sphinx-ext that's installed automatically when building on Read the Docs 😞 . The good news is that we are slowly deprecating it and it will eventually be completely removed 🥇

The explanation of the problem is that we are removing the _ready(Search.init) call from the searchtools.js at build time because the old implementation of the readthedocs-doc-embed.js needs to perform some hacky stuffs to inject our own search backend there.

If you open https://jdaviz.readthedocs.io/en/latest/_static/searchtools.js and go to the bottom of that file, you will see:

/* Search initialization removed for Read the Docs */

instead of _ready(Sphinx.init) --which is the original value of that file (see https://github.com/sphinx-doc/sphinx/blob/master/sphinx/themes/basic/static/searchtools.js#L574)

We can't kill this extension right now, unfortunately. So, we should find a way to support both scenarios: addons enabled/disabled. Besides, I don't want to loose the ability to enable/disable addons without rebuilding your docs. With these things in mind, my proposal to fix this problem is to adapt the javascript replaced to support both cases:

  1. if the readthedocs-addons.js is detected in the page, it calls to _ready(Search.init) (new behavior)
  2. if the readthedocs-doc-embed.js is detected in the page, it does nothing (as currently)

I will open a PR with this changes, but it should be simple enough.

This will allow us to keep both scenarios working and break nothing once we remove completely our Sphinx extension.

humitos added a commit to readthedocs/readthedocs-sphinx-ext that referenced this issue Dec 12, 2023
The extension removes the call to `Search.init` because it needs to perform some
injection before calling this method. The old implementation of the
`readthedocs-doc-embed.js` calls this method manually after modifying some
stuff.

However, with the introduction of addons, we don't want to manipulate the
defaults and the Sphinx default search should work out-of-the-box.

To keep the default behavior when addons is enabled, we check for addons
javascript and if it's present we call `_ready(Search.init);` as the default
Sphinx code does.

See readthedocs/addons#213 for more information.
humitos added a commit to readthedocs/readthedocs-sphinx-ext that referenced this issue Dec 12, 2023
The extension removes the call to `Search.init` because it needs to perform some
injection before calling this method. The old implementation of the
`readthedocs-doc-embed.js` calls this method manually after modifying some
stuff.

However, with the introduction of addons, we don't want to manipulate the
defaults and the Sphinx default search should work out-of-the-box.

To keep the default behavior when addons is enabled, we check for addons
javascript and if it's present we call `_ready(Search.init);` as the default
Sphinx code does.

See readthedocs/addons#213 for more information.
@humitos humitos moved this from Planned to Needs review in 📍Roadmap Dec 12, 2023
humitos added a commit to readthedocs/readthedocs-sphinx-ext that referenced this issue Dec 13, 2023
* Keep default `Search.init` behavior if addons injected

The extension removes the call to `Search.init` because it needs to perform some
injection before calling this method. The old implementation of the
`readthedocs-doc-embed.js` calls this method manually after modifying some
stuff.

However, with the introduction of addons, we don't want to manipulate the
defaults and the Sphinx default search should work out-of-the-box.

To keep the default behavior when addons is enabled, we check for addons
javascript and if it's present we call `_ready(Search.init);` as the default
Sphinx code does.

See readthedocs/addons#213 for more information.

* Update test case

* Lint

* Update integration test

* Wait for the DOM to be ready before checking for the `script`
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Dec 13, 2023
@humitos
Copy link
Member Author

humitos commented Dec 13, 2023

This is already working fine at https://docs.astropy.org/en/latest/search.html?q=coordinates

Screenshot_2023-12-13_10-12-02

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
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant