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

Only load one CSS theme by default #103971

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #82614.

You can test it here.

cc @notriddle
r? @jsha

@rustbot rustbot added 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. labels Nov 4, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@notriddle
Copy link
Contributor

No, this causes a flash of unstyled content when switching themes.

Screencast.from.11-04-2022.09.29.14.AM.webm

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Maybe we should load the other themes as well once the page is loaded?

@notriddle
Copy link
Contributor

If I was you, I'd have settings.js create alternate stylesheet links when it runs for every present-but-inactive theme, so that all three stylesheets get loaded when the popover or settings.html is opened, but only the currently-in-use theme gets loaded during normal use.

@GuillaumeGomez
Copy link
Member Author

Seems like you have a plan in mind. Wanna give it a try?

@jsha
Copy link
Contributor

jsha commented Nov 4, 2022

Thanks for fixing this! As an FYI, can you hold off on merging until after #101702 since it's going to conflict?

@GuillaumeGomez
Copy link
Member Author

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\" \
Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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!

@notriddle
Copy link
Contributor

Preview of the version with preloading added: http://notriddle.com/notriddle-rustdoc-demos/load-only-one-theme/test_dingus/index.html

@GuillaumeGomez
Copy link
Member Author

I'll send a fix for the CI error (which comes from my commit).

@rust-log-analyzer

This comment has been minimized.

@jsha
Copy link
Contributor

jsha commented Nov 4, 2022

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

@GuillaumeGomez
Copy link
Member Author

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.

@jsha
Copy link
Contributor

jsha commented Nov 4, 2022

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).

@GuillaumeGomez
Copy link
Member Author

We just said the same thing. 😉 I'll give a try to what suggested (or just push on my branch, as you prefer).

@notriddle
Copy link
Contributor

My patch didn’t fix the initial page load. It fixes theme switching in the Settings area.

@GuillaumeGomez
Copy link
Member Author

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. ^^'

@bors
Copy link
Contributor

bors commented Nov 5, 2022

☔ The latest upstream changes (presumably #104017) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Closing in favour of #106915.

@GuillaumeGomez GuillaumeGomez deleted the load-only-one-theme branch January 30, 2023 16:19
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 9, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: CSS for all three themes is fetched
6 participants