Skip to content

Move theme dropdown out of HTML #16851

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.

Sign up for GitHub

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

Open
GuillaumeGomez wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move theme dropdown out of HTML #16851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions util/gh-pages/index_template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<link id="githubLightHighlight" rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.6.0/styles/github.min.css" disabled="true" /> {# #}
<link id="githubDarkHighlight" rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.6.0/styles/github-dark.min.css" disabled="true" /> {# #}

<!-- The files are not copied over into the Clippy project since they use the MPL-2.0 License -->
<!-- The files are not copied over into the Clippy project since they use the MPL-2.0 License --> {# #}
<link rel="stylesheet" href="https://cdn.jsdelivr.net/gh/rust-lang/mdBook@0.4.46/src/theme/css/variables.css"/> {# #}
<link id="styleHighlight" rel="stylesheet" href="https://cdn.jsdelivr.net/gh/rust-lang/mdBook@0.4.46/src/theme/highlight.css"> {# #}
<link id="styleNight" rel="stylesheet" href="https://cdn.jsdelivr.net/gh/rust-lang/mdBook@0.4.46/src/theme/tomorrow-night.css" disabled="true"> {# #}
Expand All @@ -28,23 +28,6 @@
<script src="script.js" defer></script> {# #}
</head> {# #}
<body> {# #}
<div id="settings-dropdown" class="dropdown"> {# #}
<button class="settings-icon" tabindex="-1"></button> {# #}
<div class="settings-menu" tabindex="-1"> {# #}
<div class="setting-radio-name">Theme</div> {# #}
<select id="theme-choice"> {# #}
<option value="ayu">Ayu</option> {# #}
<option value="coal">Coal</option> {# #}
<option value="light">Light</option> {# #}
<option value="navy">Navy</option> {# #}
<option value="rust">Rust</option> {# #}
</select> {# #}
<label> {# #}
<input type="checkbox" id="disable-shortcuts"> {#+ #}
<span>Disable keyboard shortcuts</span> {# #}
</label> {# #}
</div> {# #}
</div> {# #}
<script src="theme.js"></script> {# #}

<div class="container"> {# #}
Expand Down
1 change: 0 additions & 1 deletion util/gh-pages/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,6 @@ summary {
}
}

html:not(.js) #settings-dropdown,
html:not(.js) #menu-filters {
display: none;
}
Expand Down
25 changes: 25 additions & 0 deletions util/gh-pages/theme.js
View file
Open in desktop
Copy link
Copy Markdown
Member

@Alexendoo Alexendoo Apr 23, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

theme.js is render blocking so the page load time would be the same or slower (since the JS version is larger)

But mainly, the size of the dropdown is negligible post compression. If it were a straight improvement I wouldn't say it's worth the awkwardness of HTML in JS strings

View changes since the review

Copy link
Copy Markdown
Member Author

@GuillaumeGomez GuillaumeGomez Apr 23, 2026

Choose a reason for hiding this comment

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

I can include all the changes at once if you prefer (ie, removing all the filters from HTML as well). Just thought it would make review simpler. As you prefer. :)

Copy link
Copy Markdown
Member

@Alexendoo Alexendoo Apr 27, 2026

Choose a reason for hiding this comment

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

The filter dropdowns themselves could go into script.js but not the buttons, but again I just don't think it's worth it. Currently all elements are present in the HTML template which is a lovely property to have, deferring a small amount of bytes isn't a good enough trade for that IMO

Copy link
Copy Markdown
Member Author

@GuillaumeGomez GuillaumeGomez Apr 27, 2026

Choose a reason for hiding this comment

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

Removing the dropdowns from the HTML would remove 4.7KB. Removing the theme dropdown from the HTML would remove 0.5KB.

So 5.3KB is a bit more than two lints "blocks". So not really a game-changer, but it allows to completely split the static content from the dynamic one, which I think is a very good thing. That would also allow to simplify the CSS since we won't need to have a CSS class to know if JS is enabled or not. A few small things that together seem worth it to me.

Copy link
Copy Markdown
Member

@Alexendoo Alexendoo Apr 27, 2026

Choose a reason for hiding this comment

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

The dropdown toggles and search bar still have to be in the HTML to avoid layout instability so there's no meaningful split between static/dynamic to be had, having some parts in one file and some in another is just annoying

The compressed size is what matters though, menu repetition compresses significantly better than ~unique prose. I measure it to be 402 byte difference in compressed files for the filter dropdowns (the ul.dropdown-menus)

Copy link
Copy Markdown
Member Author

@GuillaumeGomez GuillaumeGomez Apr 27, 2026

Choose a reason for hiding this comment

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

The dropdown toggles and search bar still have to be in the HTML to avoid layout instability so there's no meaningful split between static/dynamic to be had, having some parts in one file and some in another is just annoying

Just like in this PR, the HTML would be added before the page is even visible to the user since the loading of the script would block the rendering (a fraction of second without caching though).

The compressed size is what matters though, menu repetition compresses significantly better than ~unique prose. I measure it to be 402 byte difference in compressed files for the filter dropdowns (the ul.dropdown-menus)

True, but that's about transmitted data, not what you have in the end. For noscript users, they would still have the 5KB extra.

Copy link
Copy Markdown
Member

@Alexendoo Alexendoo Apr 27, 2026

Choose a reason for hiding this comment

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

Just like in this PR, the HTML would be added before the page is even visible to the user since the loading of the script would block the rendering

Then what would it achieve? That would be the same issue as #16851 (comment) - it's only if you moved content into script.js that the page could start to display earlier due to a smaller download that the sizes would become relevant

True, but that's about transmitted data, not what you have in the end. For noscript users, they would still have the 5KB extra.

That's the part that matters for load times outside of extreme examples

I don't consider the tiny waste for noscript users to be a problem that needs solving

Copy link
Copy Markdown
Member Author

@GuillaumeGomez GuillaumeGomez Apr 27, 2026

Choose a reason for hiding this comment

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

Then what would it achieve? That would be the same issue as #16851 (comment) - it's only if you moved content into script.js that the page could start to display earlier due to a smaller download that the sizes would become relevant

Page is smaller for noscript users.

I don't consider the tiny waste for noscript users to be a problem that needs solving

Arf, an opinion difference that locks this discussion I guess...

Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,31 @@ function setTheme(theme, store) {
}

(function() {
function generateSettingsButton() {
const dropdown = document.createElement("div");
dropdown.id = "settings-dropdown";
dropdown.classList.add("dropdown");
dropdown.innerHTML = `
<button class="settings-icon" tabindex="-1"></button>
<div class="settings-menu" tabindex="-1">
<div class="setting-radio-name">Theme</div>
<select id="theme-choice">
<option value="ayu">Ayu</option>
<option value="coal">Coal</option>
<option value="light">Light</option>
<option value="navy">Navy</option>
<option value="rust">Rust</option>
</select>
<label>
<input type="checkbox" id="disable-shortcuts"> {#+ #}
<span>Disable keyboard shortcuts</span>
</label>
</div>`;
document.body.insertBefore(dropdown, document.body.firstChild);
}

generateSettingsButton();

// This file is loaded first. If so, we add the `js` class on the `<html>`
// element.
document.documentElement.classList.add("js");
Expand Down