Only load one CSS theme by default #103971
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @Folyd, @jsha |
|
No, this causes a flash of unstyled content when switching themes. Screencast.from.11-04-2022.09.29.14.AM.webm |
This comment has been minimized.
This comment has been minimized.
|
Maybe we should load the other themes as well once the page is loaded? |
|
If I was you, I'd have |
|
Seems like you have a plan in mind. Wanna give it a try? |
|
Thanks for fixing this! As an FYI, can you hold off on merging until after #101702 since it's going to conflict? |
|
Sure, just r+ once it's ready to go. ;) |
| if let Ok(theme) = file.basename() { | ||
| write!( | ||
| buf, | ||
| "<link rel=\"preload\" href=\"{root_path}{theme}{suffix}.css\" \ |
There was a problem hiding this comment.
Instead of generating all this HTML and duplicating the logic, why not creating a JS function to load them and instead call this function here like:
<script>window.preloadAllCss();</script>? That will make the maintenance easier too to only have it in one place.
There was a problem hiding this comment.
And you would call this function in the setTimeout call too.
There was a problem hiding this comment.
Because if I did that, the system wouldn't be able to start loading the extra themes until after the JavaScript was finished loading. I want the dependency tree to be shallow. That's also why I decided against implementing this stuff in settings.js.
There was a problem hiding this comment.
Makes sense. We'll need to be careful if we ever update this but I think it's fine otherwise. Thanks for the extra explanation!
|
Preview of the version with preloading added: http://notriddle.com/notriddle-rustdoc-demos/load-only-one-theme/test_dingus/index.html |
|
I'll send a fix for the CI error (which comes from my commit). |
This comment has been minimized.
This comment has been minimized.
|
By the way, I haven't tested it, but I suspect this approach will cause a FOUC (Flash of Unstyled Content) on page navigations when a dark theme is selected. The reason is that when storage.js modifies the href, that is a non-render-blocking operation. Normally non-render-blocking is good, but for something as big as the page background we actually do want to block render. The only way I know of to insert a stylesheet in a render-blocking way is document.write. I put together a working demo last year but never got it polished up to the point I could submit it: |
|
It's indeed a bug @notriddle's found on my original commit and it's supposed to be fixed by their commit. However it's true that I still have a white flash when the page is loading. Waiting after the page is loaded is too late I think. |
|
I think @notriddle's change doesn't fix the FOUC because we still need a document.write during page load (see the diff I linked above). |
|
We just said the same thing. 😉 I'll give a try to what suggested (or just push on my branch, as you prefer). |
|
My patch didn’t fix the initial page load. It fixes theme switching in the Settings area. |
|
Oh sorry, I don't know why but I somehow understood that you were working on the background flash... Finishing my other PR then time to sleep. ^^' |
|
☔ The latest upstream changes (presumably #104017) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing in favour of #106915. |
…theme, r=GuillaumeGomez,jsha Only load one CSS theme by default This is a tweaked version of rust-lang#103971 that uses `document.write` to create the stylesheet link at startup, avoiding a FOUC during page navigation. It also rebases the PR, making it work with the new hashed filenames. Fixes rust-lang#82614 Preview: http://notriddle.com/notriddle-rustdoc-demos/load-only-one-theme-v2/std/index.html
Fixes #82614.
You can test it here.
cc @notriddle
r? @jsha