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

Highlight all search terms on the search results page #10930

Merged
merged 8 commits into from Aug 12, 2023

Conversation

mitya57
Copy link
Contributor

@mitya57 mitya57 commented Oct 23, 2022

Subject: Highlight all search terms on search results page

Feature or Bugfix

  • Bugfix

Purpose

SphinxHighlight.highlightSearchWords() is called on DOMContentLoaded event, and it will not highlight page fragments loaded asynchronously after that event. Also, when doing two searches in a row, that function uses search term from the previous search query because it was saved in localstorage.

So we should highlight all such fragments manually.

(At first I tried to rehighlight the whole page by calling SphinxHighlight.highlightSearchWords() after each fragment is loaded. It did not work because the first invocation deleted sphinx_highlight_terms from localstorage, so all subsequent invocations did not work.)

Relates

@mitya57
Copy link
Contributor Author

mitya57 commented Mar 19, 2023

I have pushed a new version of this PR with two improvements:

  • Now it highlights all occurrences of search term in a fragment, not just the first one.
  • highlightSearchWords() is no longer called on the search page at all. First, it's no longer needed after my changes. Second, it was wrong because it uses search terms from the previous search query, not from the current one.

Now our WebKit-based tests in Debian finally pass in a reproducible way (previously they have been failing from time to time because of race condition between highlightSearchWords() and my new code).

@AA-Turner Sorry for direct ping, but I don't want this PR to be buried under a bunch of more recent ones, so whenever you have time, please take a look.

@AA-Turner AA-Turner added this to the some future version milestone Apr 29, 2023
SphinxHighlight.highlightSearchWords() is called on DOMContentLoaded
event, and it will not highlight page fragments loaded asynchronously.

So we should highlight all such fragments manually.

Fixes sphinx-doc#10864.
@AA-Turner AA-Turner changed the title Highlight all search terms on search results page Highlight all search terms on the search results page Aug 12, 2023
@AA-Turner AA-Turner enabled auto-merge (squash) August 12, 2023 06:50
@AA-Turner
Copy link
Member

Thanks @mitya57!

A

@AA-Turner AA-Turner merged commit 449020e into sphinx-doc:master Aug 12, 2023
26 of 27 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

highlightSearchWords() is not called after fragments are loaded via AJAX
2 participants