r634b05f3a5070fef98 breaks SQLAlchemy's search page, emits dozens of unnecessary HTTP requests

[zzzeek]

Describe the bug

upgrading to the 4.1 series, SQLAlchemy's search page now has a bunch of tags that look like <p class="context"> throughout them with ellipses inside with no functionality and it looks like these all emit indivudal HTTP requests that seem to do nothing, slowing down our search page.

the change is exactly that made in 634b05f. If i edit this directly in searchtools.js, the old behavior is restored. the change appears to now ignore an option that was in place before which explicitly states that the contextual files for this feature are not available. I have also tried setting html_copy_source to True, hoping to at least see the contextual stuff that I see in other doc pages, but it doesn't work for my site anyway.

i think it's a bad idea to emit an unecessary ajax / network request that has no chance of being useful. Can the old behavior please be restored? this change forces unnecessary network overhead for a feature that's not in use and also breaks our layout (which I can work around wtih CSS, but the 95 extra network calls makes this a non -starter).

How to Reproduce

$ git clone https://github.com/sqlalchemy/sqlalchemy
$ cd sqlalchemy/docs/build
$ pip install -r requirements.txt
$ make html
$ cd output/html
$ python -m http.server  # view docs on webserver
$ # navigate to http://localhost:8000

Expected behavior

No response

Your project

https://docs.sqlalchemy.org/

Screenshots

good:

bad:

OS

linux

Python version

3.9

Sphinx version

4.1.1

Sphinx extensions

No response

Extra tools

No response

Additional context

No response

[zzzeek]

edited above, note that this bug causes the search page to emit dozens of unnecessary network requests as well.

[tk0miya]

Sorry for the inconvenience.

There are a related change and a bug:

• searchtools.js starts to show the summary text (around the keyword) even if html_copy_source = False (refs: HTML: Fix search unnecessarily requiring source files #9129). Since Sphinx-2.0, searchtools.js does not require source files to create the summary text. So v4.1.0 starts to show them without source files.
• searchtools.js inserts only abbreviation marks ("...") as a summary text if failed to fetch it. On creating a summary text, the script searches a HTML element having role="main" attribute. But SQL Alchemy docs does not have it.

I think the former one is an intended change. But it has also brought incompatible change. That's not intended. So it should be reverted during 4.1.x at least. I'm still debating this change will be applied again since 4.2 or 5.0.

Another one is a bug. Therefore, it should be fixed.

cc: @Blendify

[zzzeek]

thanks for the reply!! I think regardless of how you want to approach this, a configuration option to turn off the ajax-request-per-result thing altogether should be available, at the very least to support sites that don't have this text. (why doesn't my site have the text, is it because my custom theme isn't doing something it's supposed to?)

[tk0miya]

I think regardless of how you want to approach this, a configuration option to turn off the ajax-request-per-result thing altogether should be available, at the very least to support sites that don't have this text.

Perfectly agreed. I'll add a configuration to disable showing summary texts on re-merge #9129.

(why doesn't my site have the text, is it because my custom theme isn't doing something it's supposed to?)

Before the upgrading Sphinx (< 4.1), it was disabled via html_copy_source = False). After the upgrading to 4.1.0, it failed to fetch summaries because the theme you're using does not have a container tag having role="main" attribute:

sphinx/sphinx/themes/basic/layout.html

Lines 178 to 181 in 07598f0

	<div class="body" role="main">
	{% block body %} {% endblock %}
	<div class="clearer"></div>
	</div>

[Blendify]

I would rather us add a new option rather than revert that commit, even if this means breaking compatibility. The reason I made the commit in the first place is to avoid having to copy and upload all the source files on my orgs build system, saving drive IOPS and network bandwidth.

[zzzeek]

thanks for looking into this! I will try to see if i can update my templates to work with this feature at some point.

[tk0miya]

I determined to revert the change from 4.1.x series and add a new option in 4.2.0.

[Blendify]

Any update here? This is preventing my project from updating to a newer version of sphinx

[tk0miya]

I reverted the commit that causes the original issue at #9494 and released as v4.1.2. So I think there are no troubles with the latest release. So there is no blocker to upgrade dependency on the sqlalchemy project.

The remaining task is reimplementing #9494 by another implementation (with an option switch for it).

[tk0miya]

Finally, I added html_show_search_summary configuration. It will be released in 4.5.0.
Sorry for the inconvenience long.