-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Only load one CSS theme by default #103971
Only load one CSS theme by default #103971
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you would call this function in the setTimeout
call too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: https://github.com/rust-lang/rust/compare/master...jsha:rust:theme-better-3?expand=1 |
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