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

take into account the system theme #61236

Open
wants to merge 1 commit into
base: master
from

Conversation

@GuillaumeGomez
Copy link
Member

commented May 27, 2019

Fixes #61079.

The CSS can now take into account the system theme. I used it to generate some content on the document and from there, if no theme has already been selected, it'll look at the system level theme.

r? @QuietMisdreavus
cc @fenhl

@fenhl

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

How does this interact with having set the theme manually? I switch my system theme between light and dark mode based on time of day and would like rustdoc to adhere to that.

@fenhl

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Looking at the changes, it seems that I'd just have to clear local storage. I'm okay with that.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

You'd still have to reload the page, but that's a browser limitation (and I don't want to check multiple times after the page has been loaded if the theme hasn't change...).

@@ -54,6 +54,21 @@
box-sizing: border-box;
}

/* This part handles the "default" theme being used depending on the system one. */
html {
content: "";

This comment has been minimized.

Copy link
@bjorn3

bjorn3 May 28, 2019

Contributor

Maybe use a custom css property instead: https://developer.mozilla.org/en-US/docs/Web/CSS/--*

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 28, 2019

Author Member

You mean CSS variable? Not supported on IE so we can't use it.

This comment has been minimized.

Copy link
@bjorn3

bjorn3 May 28, 2019

Contributor

IE doesn't support prefers-color-scheme anyway. IE would not set the custom property to any value, but the javascript has a fallback for that already.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 29, 2019

Author Member

I'd prefer avoid potential failures if possible.

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 5, 2019

Member

Just to confirm, since string values for content is only ever allowed for ::before and ::after, this won't cause a situation where the entire page is replaced with whatever is set here? MDN seems to imply this, but i want to double-check that that's what's actually going on.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jun 6, 2019

Author Member

The content is a hack that isn't displayed on this tag. You can only "see" it with js (I think it's because it's not a ::before or ::after element).

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 6, 2019

Member

What are the chances of this behavior changing in the future? I don't want to be trapped in a situation where the CSS spec starts allowing element replacement with strings, and a whole bunch of docs are rendered unusable because we started depending on this workaround.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jun 6, 2019

Author Member

I'm not sure to follow you. There is nothing in the specs or anywhere changing this behavior for the moment and if there is any change, we'll have quite a lot of time before needing to update it. Also, what are you referring to when talking about "and a whole bunch of docs are rendered unusable because we started depending on this workaround."?

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 6, 2019

Member

If this behavior changes, i.e. some browser decides that setting content to a string on an element will cause the element to be replaced by that string, then any documentation already built by a rustdoc using this CSS will only display the string set here.

As long as the specs are on our side, and setting content to a string on an element does nothing, then i think i can be okay with this.

@QuietMisdreavus
Copy link
Member

left a comment

This PR seems fine, just one naming nit.

Would it be worth checking for whether it's set and adding a "system" entry to the drop-down that clears the local storage and loads the prefers-color-scheme value? I'm not sure how many people would be willing to dive into browser dev tools to clear the theme value.

@@ -88,7 +88,7 @@ function getCurrentValue(name) {
return null;
}

function switchTheme(styleElem, mainStyleElem, newTheme) {
function switchTheme(styleElem, mainStyleElem, newTheme, dontSaveNewValue) {

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 5, 2019

Member

I'm a bit wary of naming the parameter "don't save new value", just because it feels weird to name a boolean parameter after the opposite of what it does. Maybe something like initialLoad or skipStorage? Or even inverting the value that is sent on the initial call and calling it persist?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jun 6, 2019

Author Member

I like skipStorage! Let's go for it! :)

@@ -54,6 +54,21 @@
box-sizing: border-box;
}

/* This part handles the "default" theme being used depending on the system one. */
html {
content: "";

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 5, 2019

Member

Just to confirm, since string values for content is only ever allowed for ::before and ::after, this won't cause a situation where the entire page is replaced with whatever is set here? MDN seems to imply this, but i want to double-check that that's what's actually going on.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@QuietMisdreavus: I'm not sure it's worth it to add a system theme entry. It's just in case you didn't set your theme after all, and since it's locally stored, if you change of computer it'll just pick a theme depending on your system theme.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:system-theme branch from 3dcca5f to a050d2a Jun 6, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Updated!

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@QuietMisdreavus: I'm not sure it's worth it to add a system theme entry. It's just in case you didn't set your theme after all, and since it's locally stored, if you change of computer it'll just pick a theme depending on your system theme.

I'm not sure what you mean by "change of computer". My concern is that people who have already set a theme on doc.rust-lang.org won't have the option of using the system theme as a default without clearing their browser history or manually digging into developer tools to fiddle with specific Local Storage values. Since we've had themes available on that domain for a long time now, it's very likely that users will already have a theme setting saved. The same goes for docs.rs.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Why would you want to use it system theme if you already set it yourself? I'm not sure to follow on this one...

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Why would you want to use it system theme if you already set it yourself? I'm not sure to follow on this one...

What if you decide you wanted to change your system theme? What if you have your system theme change on different times of day (e.g. to track sunset and sunrise, or to differentiate between "work mode" and "personal mode", etc)? All of our current users have already set a theme choice, and may want to start taking advantage of this feature once it lands.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Sounds weird to change the website design when switching pages though. Hummm... I don't want to add the "system theme" entry but I don't really know what to do for this one...

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

ping from triage @QuietMisdreavus any updates on this?

}
}

switchTheme(currentTheme, mainTheme, getCurrentValue("rustdoc-theme") || "light");
function getSystemValue() {
return getComputedStyle(document.documentElement).getPropertyValue('content');

This comment has been minimized.

Copy link
@dima74

dima74 Jul 7, 2019

Contributor

FYI there exists window.matchMedia method, which can be used directly (without setting content prop in css)

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jul 7, 2019

Author Member

Nice! Didn't know about this one... Thanks!

@gagan0723

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

ping from triage, @QuietMisdreavus any updates on this?

@chocol4te

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Second ping from wg-triage, pinging random member of Rustdoc.

@ollie27

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

It's been around for a while and I resolved all issues (as long as the users don't change the theme by themselves, it won't be set in the local storage so changing page will still rely on the system theme).

I'll wait to see if @ollie27 has anything to say.

@chocol4te

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Ah sorry, my bad, I wasn't sure if the final concern was blocking or not.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

If you're talking about window.matchMedia, it can't be added because not supported in old browsers (but very interesting more new projects of mine!).

@dima74

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

@GuillaumeGomez may I ask, which old browsers did you mean? I don't know exactly policy about browsers (best what I found is this rfc), but looks like window.matchMedia has strong browser support:

image

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

We unofficially support rustdoc starting IE9. I think I'll drop it not too far in the future though.

@JohnTitor

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Ping from triage: @QuietMisdreavus @ollie27 waiting on your review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.