#5460 Make JS refactoring compatible with old templates

[TimKam]

Subject: Make JS refactoring compatible with old templates

Feature or Bugfix

• Bugfix

Purpose

• Fixes Search does not work for docs built with 1.8.0 #5460, prevents old templates from breaking search function

Detail

This is my attempt of fixing this issue.
Feedback on implementation details are appreciated.

• Inject documentation_options.js script as part of HTML build
• Set attributes of the script accordingly
• Remove script reference from basic template (would be duplication)
• Add fallback for URL_ROOT variable
• Tested with Read the Docs template and Sphinx template

Relates

• Search does not work for docs built with 1.8.0 #5460

[codecov]

Codecov Report

Merging #5500 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5500      +/-   ##
==========================================
+ Coverage   83.08%   83.08%   +<.01%     
==========================================
  Files         289      289              
  Lines       38795    38801       +6     
  Branches     5842     5843       +1     
==========================================
+ Hits        32231    32237       +6     
  Misses       5200     5200              
  Partials     1364     1364

Impacted Files	Coverage Δ
sphinx/builders/html.py	85.18% <100%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c83b8b...53f6967. Read the comment docs.

[tk0miya]

This would not work with old themes. Because they has not used js_tag() function. It was added in 1.8.

To support them, we need to modify #5207. They did not read documentation_options.js.

[TimKam]

Thanks for reviewing, @tk0miya.

The idea of the change is the following:

• The documentation_options script is added in any case to the HTML file, on config-inited.
• If a "new" template is used, the setup_js_tag_helper function adds the attributes to the documentation_options script tag that are needed to define the URL root.
• If an "old" template is used, the attributes are not added. However, the URL root is determined within the old templates, and caught by the following change in documentation_options.js_t:

URL_ROOT: document.getElementById("documentation_options") ?
        document.getElementById("documentation_options").getAttribute('data-url_root') :
        DOCUMENTATION_OPTIONS.URL_ROOT,

Or am I missing something?

[mitya57]

I just tested this commit with sphinx-rtd-theme 0.4.1 tag, and it seems to work.

Maybe you can change the base branch from master to 1.8 (and rebase accordingly)?

[TimKam]

Great, I'll wait for feedback from @tk0miya and rebase then.

[TimKam]

@tk0miya Sorry for pinging you again, but could you take a look at my justification above?

[tk0miya]

We have four generations for searching mechanism:

1. DOCUMENTATION_OPTIONS is defined in layout.html directly (I guess this might be not used)
2. DOCUMENTATION_OPTIONS is defined in documentation_options.js. But <script> tag does not have any ID
3. "2." and <script> tag has documentation_options ID
4. "3." and support js_tag() to generate <script> tag. In addition, some codes were moved to documentation_options.js from searchtools.js.

I think current documentation_options.js contains a stemmer, Scorer and word-splitter. But this does not write-back them to serachtools.js. So this would not work for non-English document.
(There are some fallback functions. So it would not be crashed.)

I think reverting #5207 would fix the situation.

[tk0miya]

As commented above, 3rd generation themes does not use js_tag() helper. So this would not be moved.

In addition, to support this tag by js_tag(), we have to give a special code for this like you modified. I feel it is too much.

[mitya57]

How about moving stemmer, scorer, word-splitter and SEARCH_LANGUAGE_STOP_WORDS to a separate JS file (e.g. language_data.js), which will then be in script_files? This way it will be included even for custom, old-style templates.

@tk0miya Will you accept a pull request implementing such a change?

[tk0miya]

+1 for @mitya57
From my understanding, they were moved for testing searchtools.js (see #5207).
So to create a new file might be a better solution for this problem.

[mitya57]

@tk0miya Submitted as #5590.

[tk0miya]

Merged #5590 instead. So I'm closing this now.