Skip to content

[rustdoc] Only generate search DOM elements if the search is actually needed - #160639

Open
GuillaumeGomez wants to merge 3 commits into
rust-lang:mainfrom
GuillaumeGomez:search-dom
Open

[rustdoc] Only generate search DOM elements if the search is actually needed#160639
GuillaumeGomez wants to merge 3 commits into
rust-lang:mainfrom
GuillaumeGomez:search-dom

Conversation

@GuillaumeGomez

Copy link
Copy Markdown
Member

I realized that we were generating the search DOM elements (everything contained into #search) all the times, even when there is no search query parameters in the URL. That seems unnecessary so I reworked the JS a bit to remove that.

r? @lolbinarycat

@rustbot

rustbot commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @lolbinarycat

@rustbot rustbot added A-rustdoc-js Area: Rustdoc's JS front-end S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Aug 6, 2026
@@ -16,6 +17,7 @@ define-function: ("collapsed-from-search", [], block {
// Then we collapse the section again...
set-property: ("#implementations-list .implementors-toggle", {"open": "false"})
// Then we run the search.
call-function: ("open-search", {})

@GuillaumeGomez GuillaumeGomez Aug 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that was my original "quest": looking at fixing the new flakyness which happened in #159593 (comment). ^^'

View changes since the review

Comment thread tests/rustdoc-gui/sidebar-source-code-display.goml
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Added the missing Typescript type definition update and CI is now happy.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Added comments as suggested for clarification.

@lolbinarycat lolbinarycat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks solid, only a few nits/questions.

View changes since this review

Comment on lines +250 to +251
// If you're browsing the nightly docs, the page might need to be refreshed for
// the search to work because the hash of the JS scripts might have changed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: technically this doesn't just apply to nightly, it's just way more common there

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied comment from here. But yeah agreed.

Comment on lines +261 to +269
if (!window.StringdexOnload) {
window.StringdexOnload = [];
}
window.StringdexOnload.push(() => {
loadScript(
getVar("static-root-path") + getVar("search-js"),
sendSearchForm,
);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the structure of this doesn't make a ton of sense to me in combination, shouldn't we just be initializing it to an array with a single element? otherwise we could hypothetically end up with an array of duplicate initializers?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moved existing code, didn't modify anything. I think it's because you can have multiple search indexes loaded in parallel.

Comment thread src/librustdoc/html/static/js/main.js Outdated
Comment on lines +503 to +504
// We load the search input.
window.searchState.inputElement();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best i can tell, inputElement() doesn't directly load the search, it sets up a focus handler that loads the search.. why not just load the search and get something more readable and less indirect?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll improve the comment as it's quite unclear.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Improved the problematic comment. As I mentioned, the rest is just code I moved around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-rustdoc-js Area: Rustdoc's JS front-end S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants