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

Fix searchindex.js loading when ajax fails (because e.g. CORS in embedded iframes) #15649

Merged
merged 2 commits into from Nov 10, 2019

Conversation

@jeroonk
Copy link
Contributor

jeroonk commented Nov 9, 2019

Summary:

Searching the documentation does not work in Jupyterlab's Matplotlib reference pane, because searchindex.js fails to load due to a AJAX/XHR CORS issue. The search page remains stuck on "Searching..".

This PR partially fixes the issue by restoring the backup-method of using a <script> tag that was missing from Matplotlib's version of the search.html template.

The search page now returns a list of relevant links. The preview snippets that load underneath each link still do not work because of the same CORS issue (and cannot be loaded in an alternate way).

Context of the fix:

The searchindex.js is being loaded using AJAX/XHR in search.html#L45 calling loadIndex in searchtools.js#L78 (A file provided by Sphinx. Matplotlib docs inherits its theme from numpy/numpydoc, which inherits from scipy/scipy-sphinx-theme, which inherits from the sphinx-doc/sphinx basic theme).

If the XHR fails, loadIndex resorts to loading the script through the <script id="searchindexloader"> tag. Matplotlib's version of the search.html template has somehow lost this tag.

Additionally, Sphinx has changed the way searchindex.js loads:

  • In the latest version of the search.html template, the AJAX method is dropped for just a simple defer-ed <script> tag. Alternative to this PR, matplotlib could also adopt this change instead?
  • But this was then reverted again because of third-party themes (such as Matplotlib's) still using the old method.

What about the original issue, that XHR fails?

The original AJAX/XHR request fails because the CORS (Cross-Origin-Resource-Sharing) preflight OPTIONS request returns a 405 Method Not Allowed response from matplotlib.org. Even though a subsequent GET would return the correct Access-Control-Allow-Origin: * header. The same happens for the snippets.

This issue has to be fixed in the web server configuration. For the OPTIONS request it should instead respond with a 200 OK and the headers Access-Control-Allow-Methods: POST, GET, OPTIONS and
Access-Control-Allow-Origin: *. I'll submit a request in discourse for this, but otherwise do not know where/whom to direct this issue to. (Should this require a separate issue on the main repo?)

See also: https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request or https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Preflighted_requests.

What about the older docs?

I am not sure how to go about potentially getting this fix back-ported to older versions of Matplotlib documentation (Is that a thing? Or are those frozen?). The Jupyterlab reference pane only points to the latest version on matplotlib.org, so it is not an immediate concern. It's probably better to just have the CORS issue on the server fixed.

@ImportanceOfBeingErnest

This comment has been minimized.

Copy link
Contributor

ImportanceOfBeingErnest commented Nov 10, 2019

Matplotlib's website is built upon a vendored version of the sphinx_rtd_theme, which was forked years ago. This fact already led to some problems with search in the past, see #12216.
If you know a future-proof solution, you are very welcome to add it here.

I wouldn't worry about older versions.

@jeroonk jeroonk force-pushed the jeroonk:master branch from 34e5e06 to adbc7b1 Nov 10, 2019
@jeroonk

This comment has been minimized.

Copy link
Contributor Author

jeroonk commented Nov 10, 2019

Forgot the type="text/javascript" on the previous commit, sorry about that.

A more future-proof version would probably be to follow sphinx-doc/sphinx@55c5168 and replace the whole:

<script type="text/javascript">
    jQuery(function() { Search.loadIndex("searchindex.js"); });
</script>
<script type="text/javascript" id="searchindexloader"></script>

with:

<script type="text/javascript" src="{{ pathto('searchindex.js', 1) }}" defer></script>

That way, loading searchindex.js will no longer be dependent on AJAX or searchtools.js (which is, even in sphinx_rtd_theme, inherited from the sphinx basic theme template), which may in future versions of sphinx again remove the loadIndex method (as was done in the mentioned commit, but then reverted again in sphinx-doc/sphinx@a6e3170).

@tacaswell tacaswell added this to the v3.1-doc milestone Nov 10, 2019
Hopefully a more future-proof fix, removing the dependency on ajax and
searchtools.js altogether. (After loading, searchindex.js itself does
depend on searchtools.js, but both are updated with the version of
sphinx that is used, unlike this template.)

Because the script is loaded with `defer` and also right at the end of
the body, the size of searchindex.js should not block loading the rest
of the page (one of the arguments for using ajax).
@jeroonk jeroonk force-pushed the jeroonk:master branch from 9c7bf5e to 4bed3f6 Nov 10, 2019
@jeroonk

This comment has been minimized.

Copy link
Contributor Author

jeroonk commented Nov 10, 2019

Referenced the correct pull request this time.

This solution removes the need for searchtools.js or ajax for loading searchindex.js altogether (after loading, searchindex.js itself does depend on searchtools.js, but both are updated with the version of sphinx that is used, unlike this template).

From the author: sphinx-doc/sphinx#3620 (comment), the argument for using ajax to load searchindex.js seems to have been that it is a large file (~1.5 MB currently) and might block loading the page. But since the template block had already been moved to the very bottom of the page, just before </body> and including the defer keyword, that will probably not be a problem.

Also in the pull request sphinx-doc/sphinx#6091 the conclusion seems to be that it works fine on all browsers.

@jeroonk

This comment has been minimized.

Copy link
Contributor Author

jeroonk commented Nov 10, 2019

Did a bit more digging and found that this issue became visible mainly as the result of Jupyterlab iframe containing a sandbox attribute with no allow-same-origin, triggering the CORS preflight OPTIONS request that the website is incorrectly configured for. I have reported it at jupyterlab#7506.

This same issue also affects documentation for Numpy, Scipy, Pandas, Sympy, IPython and even the main Python.org docs. None of them however return no results, which is why this fix should still be merged in Matplotlib. It also allows loading search results when opened as a local file:/// page.

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Nov 10, 2019

A note about our website hosting: It is primarily hosted through githubpages and proxied through cloudflare. It looks like CF passes through CORS headers (https://support.cloudflare.com/hc/en-us/articles/200308847-Using-cross-origin-resource-sharing-CORS-with-Cloudflare ) so to fix this on the server side we would have to figure out how to configure github pages.

@dorafc
dorafc approved these changes Nov 10, 2019
Copy link
Contributor

dorafc left a comment

Looks good. Thank you for your detailed notes and research.

@tacaswell tacaswell modified the milestones: v3.1-doc, v2.2-doc Nov 10, 2019
@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Nov 10, 2019

Thanks!

I set this to backport back to the 2.2.x docs so the docs of all of our currently supported versions will be fixed.

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Nov 10, 2019

@tacaswell tacaswell merged commit 3608411 into matplotlib:master Nov 10, 2019
18 checks passed
18 checks passed
LGTM analysis: Python No code changes detected
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
Travis CI - Pull Request Build Passed
Details
ci/circleci: docs-python36 Your tests passed on CircleCI!
Details
ci/circleci: docs-python37 Your tests passed on CircleCI!
Details
ci/circleci: docs-python38 Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 66e4fb2...4bed3f6
Details
codecov/project/library 74.3% (target 50%)
Details
codecov/project/tests 99.25% remains the same compared to 66e4fb2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
matplotlib.matplotlib Build #20191110.5 had test failures
Details
matplotlib.matplotlib (Job Linux_py36) Job Linux_py36 succeeded
Details
matplotlib.matplotlib (Job Linux_py37) Job Linux_py37 succeeded
Details
matplotlib.matplotlib (Job Windows_py36) Job Windows_py36 succeeded
Details
matplotlib.matplotlib (Job Windows_py37) Job Windows_py37 succeeded
Details
matplotlib.matplotlib (Job Windows_pyPre) Job Windows_pyPre succeeded
Details
matplotlib.matplotlib (Job macOS_py36) Job macOS_py36 succeeded
Details
matplotlib.matplotlib (Job macOS_py37) Job macOS_py37 succeeded
Details
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 10, 2019
…ils (because e.g. CORS in embedded iframes)
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 10, 2019
…ils (because e.g. CORS in embedded iframes)
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 10, 2019
…ils (because e.g. CORS in embedded iframes)
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 10, 2019
…ils (because e.g. CORS in embedded iframes)
@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Nov 10, 2019

Thanks @jeroonk Congratulations on your first PR into Matplotilb 🎉 Hopefully we will hear from you again!

@jeroonk

This comment has been minimized.

Copy link
Contributor Author

jeroonk commented Nov 10, 2019

Thanks for merging!

I also monkey-patched the built https://26552-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/search.html into the offending Jupyterlab iframe src. There are still XHR errors on the snippets, but at least the results list loads now.

I guess the server configuration can wait. The SymPy, Pandas, IPython and Python.org docs all have the same problem. jupyterlab#7506 should also mostly mitigate it.

QuLogic added a commit that referenced this pull request Nov 10, 2019
…649-on-v3.1.1-doc

Backport PR #15649 on branch v3.1.1-doc (Fix searchindex.js loading when ajax fails (because e.g. CORS in embedded iframes))
timhoffm added a commit that referenced this pull request Nov 11, 2019
…649-on-v3.2.x

Backport PR #15649 on branch v3.2.x (Fix searchindex.js loading when ajax fails (because e.g. CORS in embedded iframes))
dstansby added a commit that referenced this pull request Nov 11, 2019
…649-on-v2.2.x

Backport PR #15649 on branch v2.2.x (Fix searchindex.js loading when ajax fails (because e.g. CORS in embedded iframes))
tacaswell added a commit that referenced this pull request Nov 21, 2019
…649-on-v3.1.x

Backport PR #15649 on branch v3.1.x (Fix searchindex.js loading when ajax fails (because e.g. CORS in embedded iframes))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.