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

Add settings to rustdoc to use the system theme #77809

Merged
merged 4 commits into from
Oct 16, 2020
Merged

Add settings to rustdoc to use the system theme #77809

merged 4 commits into from
Oct 16, 2020

Conversation

nasso
Copy link
Contributor

@nasso nasso commented Oct 11, 2020

This PR adds new settings to rustdoc to use the operating system color scheme.

click

rustdoc actually had basic support for this, but the setting wasn't visible and couldn't be set back once the theme was explicitly set by the user. It also didn't update if the operating system theme preference changed while viewing a page.

I'm using this method to query and listen to changes to the (prefers-color-scheme: dark) media query. I kept the old method (based on getComputedStyle) as a fallback in case the user-agent doesn't support window.matchMedia (so like... pretty much nobody).

Since there's now more than one official ""dark"" theme in rustdoc (and also to support custom/third-party themes), the preferred dark and light themes can be configured in the settings page (the defaults are just "dark" and "light").

This is also my very first "proper" PR to Rust! Please let me know if I did anything wrong :).

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2020
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 11, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Small nit, but I think guillaume is the better reviewer for this. This looks really cool, thanks for working on it!

r? @GuillaumeGomez

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added the A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc label Oct 11, 2020
@jyn514 jyn514 assigned GuillaumeGomez and unassigned jyn514 Oct 11, 2020
@nasso nasso changed the title Add a setting to rustdoc to use the system theme Add settings to rustdoc to use the system theme Oct 11, 2020
@nasso
Copy link
Contributor Author

nasso commented Oct 11, 2020

I'm not sure I understand how the current implementation of the theme system works. It says here that it works by changing the href of the light.css <link> tag. But then, what's the point of including <link> tags for every other theme (especially if they stay disabled).

I'd like to know so I can properly implement a fix to support JavaScript disabled browsers to use the system theme :)

@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

@Cldfire might know ^

@GuillaumeGomez
Copy link
Member

The disabled attribute is used to prevent the style to be applied, however they still are loaded by the browser in case you want to switch theme. Which means that when you arrive on a page, you won't have the light theme then the selected theme displayed (it would look like a weird flash haha).

@Cldfire
Copy link
Contributor

Cldfire commented Oct 11, 2020

I tried getting rid of the disabled attribute in #75063, but unfortunately I wasn't able to figure out how to get rid of it in a way that's supported everywhere and doesn't result in content flashing on page load.

Usage of the disabled attribute was first introduced in #71237; prior to that PR all themes were enabled at the same time and therefore had to have exactly the same CSS rules (otherwise they'd conflict with one another).

@Nemo157
Copy link
Member

Nemo157 commented Oct 12, 2020

What is the forward/backwards compatibility of this? All docs hosted on docs.rs are using a shared localStorage but the javascript from when they were built, so will this behave well when you change the settings in docs with this new JS then open docs with the old JS?

@nasso
Copy link
Contributor Author

nasso commented Oct 12, 2020

I didn't change the way localStorage is used at all. I only added new settings, and the default theme behavior (system theme) will be kept. So it's 100% backwards and forward compatible!

It actually even improves backwards: when the system theme is set, it's saved to localStorage so that when the user visits a page using the old JS, it will stay on the theme of the user's system.

@Nemo157
Copy link
Member

Nemo157 commented Oct 12, 2020

(This also looks relatively easy for docs.rs to hijack to configure its own theme (when we get support), other than the lack of self-storage-events to make it easy for us to detect when to check the value).

@GuillaumeGomez
Copy link
Member

@Nemo157 Yes, I said a while ago that I needed to do it but still haven't found time to do so... I'll try to get back to it once this PR is merged.

@nasso Looks all good to me, thanks! Just remains to replace single quotes with double quotes and then it's good to go.

@nasso
Copy link
Contributor Author

nasso commented Oct 12, 2020

Actually I think I should make sure it works properly with JavaScript disabled before merging. Currently it doesn't work, because the logic is in storage.js. One way i see to fix it would be to add a default-theme.css stylesheet that would look like this:

@import "{static_root_path}light{suffix}.css" (prefers-color-scheme: light);
@import "{static_root_path}dark{suffix}.css" (prefers-color-scheme: dark);

The problem with this is that the styles might override each other in bad ways (since there's already the javascript controlled <link>). My idea was to make that <link> default to default-theme.css instead of light.css, and let JavaScript override it if necessary. I don't know if this will cause content flashing though, I'll have to test it first if you don't see any other issue it might cause.

@GuillaumeGomez
Copy link
Member

Actually I think I should make sure it works properly with JavaScript disabled before merging.

By default it's the light theme that gets displayed, so if you get the same result, it's working the same as currently! But if you want to extend it (which would be nice!), please do so but in another PR so we can get this one merged.

@nasso
Copy link
Contributor Author

nasso commented Oct 12, 2020

Actually the old behavior was using the system theme by default! You can try it by opening the standard library documentation in a private window (so that localStorage is empty). According to your system's theme, it will either use the "dark" or the "light" theme. Only thing is that you have to reload the page to update the theme when you change it in your operating system settings.

@GuillaumeGomez
Copy link
Member

Hmmmm... Makes sense in a way I guess?

@nasso
Copy link
Contributor Author

nasso commented Oct 12, 2020

Yeah, because the old method used getSystemTheme as the default value if rustdoc-theme wasn't set in localStorage. The "light" theme was only the second fallback. See here in storage.js.

So should I add support for JavaScript disabled browsers in another PR or this one to keep backwards compatibility with the old behavior?

@jyn514
Copy link
Member

jyn514 commented Oct 12, 2020

According to your system's theme, it will either use the "dark" or the "light" theme.

Isn't that the intended behavior? What more is there to change?

@jyn514
Copy link
Member

jyn514 commented Oct 13, 2020

Also, if rustdoc-theme is set to a dark theme but the system theme is a light theme, can you set use-system-theme to false?

@jyn514
Copy link
Member

jyn514 commented Oct 13, 2020

@bors r-

I'd like to address the issues @nasso brought up (thanks for thinking of them!)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 13, 2020
@nasso
Copy link
Contributor Author

nasso commented Oct 13, 2020

I can implement that logic! I suppose that for now I should hard-code what theme is "dark" and what theme is "light"?
At least until #74937 is fixed.

@nasso
Copy link
Contributor Author

nasso commented Oct 13, 2020

Also, if rustdoc-theme is set to a dark theme but the system theme is a light theme, can you set use-system-theme to false?

This can cause issues:

  1. Someone has their operating system switch automatically between dark mode and light mode according to the time of the day.
  2. Rustdoc is all using default settings (assuming an empty localStorage). That means it follows the system theme, and writes it to localStorage when switching to it.
  3. At night, Rustdoc is using the default dark theme. This is just the default behavior. Now localStorage["rustdoc-theme"] === "dark".
  4. The next day, localStorage["rustdoc-theme"] === "dark", but the OS is now in light mode (it's daytime).
  5. Rustdoc sets localStorage["rustdoc-use-system-theme"] to false, thinking it was a user of the older Rustdoc version!

One potential fix would be to not save the theme to localStorage when automatically switched to. The downside is that older versions of Rustdoc won't pick up the theme because it's not stored there anymore.

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

Someone has their operating system switch automatically between dark mode and light mode according to the time of the day.

Seems a bit of an edge case ... but since it leads to rustdoc overriding the user's selection, better to avoid it. Rather than having complicated logic, let's just only implement my first suggestion: If use-system-theme and preferred-dark-theme aren't set, but rustdoc-theme is set to a dark theme, update preferred-dark-theme to that theme.

@pickfire
Copy link
Contributor

Someone has their operating system switch automatically between dark mode and light mode according to the time of the day.

Is it an edge case? My phone does this too.

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

I suppose that for now I should hard-code what theme is "dark" and what theme is "light"?

You can write the canonical list :) There's only three themes, it seems reasonable to have a match on them somewhere.

If the user doesn't have a preferred dark theme but is already using a
dark theme, set the preferred dark theme to be that theme
@nasso
Copy link
Contributor Author

nasso commented Oct 15, 2020

If use-system-theme and preferred-dark-theme aren't set, but rustdoc-theme is set to a dark theme, update preferred-dark-theme to that theme.

That should be implemented now! I think it's ready now.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

Looks great! Thanks so much for working on this :)

@bors r=jyn514,guillaumegomez

@bors
Copy link
Contributor

bors commented Oct 15, 2020

📌 Commit 59f9cf2 has been approved by jyn514,guillaumegomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2020
@pickfire
Copy link
Contributor

pickfire commented Oct 15, 2020

I wonder if we can have a css fallback rather than relying on javascript? Some users may have disabled javascript.

@nasso
Copy link
Contributor Author

nasso commented Oct 15, 2020

I wonder if we can have a css fallback rather than relying on javascript? Some users may have disabled javascript.

I thought about this already and I think I may implement a CSS fallback in a future PR. I already have a few ideas about how it could work but I'd prefer to discuss about it first :)

@bors
Copy link
Contributor

bors commented Oct 16, 2020

⌛ Testing commit 59f9cf2 with merge 6999ff3...

@bors
Copy link
Contributor

bors commented Oct 16, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514,guillaumegomez
Pushing 6999ff3 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants