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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 21, 2019
Merged

take into account the system theme #61236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c076d30
take into account the system theme
GuillaumeGomez May 27, 2019
1bd9424
Replaced skipStorage with saveTheme variable
GuillaumeGomez Aug 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustdoc/html/render.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,7 +914,7 @@ themePicker.onblur = handleThemeButtonsBlur;
var but = document.createElement('button');
but.innerHTML = item;
but.onclick = function(el) {{
switchTheme(currentTheme, mainTheme, item);
switchTheme(currentTheme, mainTheme, item, true);
}};
but.onblur = handleThemeButtonsBlur;
themes.appendChild(but);
Expand Down
15 changes: 15 additions & 0 deletions src/librustdoc/html/static/rustdoc.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,21 @@
box-sizing: border-box;
}

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

Choose a reason for hiding this comment

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

Maybe use a custom css property instead: https://developer.mozilla.org/en-US/docs/Web/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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer avoid potential failures if possible.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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."?

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
@media (prefers-color-scheme: light) {
html {
content: "light";
}
}
@media (prefers-color-scheme: dark) {
html {
content: "dark";
}
}

/* General structure and fonts */

body {
Expand Down
16 changes: 13 additions & 3 deletions src/librustdoc/html/static/storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function getCurrentValue(name) {
return null;
}

function switchTheme(styleElem, mainStyleElem, newTheme) {
function switchTheme(styleElem, mainStyleElem, newTheme, saveTheme) {
var fullBasicCss = "rustdoc" + resourcesSuffix + ".css";
var fullNewTheme = newTheme + resourcesSuffix + ".css";
var newHref = mainStyleElem.href.replace(fullBasicCss, fullNewTheme);
Expand All @@ -109,8 +109,18 @@ function switchTheme(styleElem, mainStyleElem, newTheme) {
});
if (found === true) {
styleElem.href = newHref;
updateLocalStorage("rustdoc-theme", newTheme);
// If this new value comes from a system setting or from the previously saved theme, no
// need to save it.
if (saveTheme === true) {
updateLocalStorage("rustdoc-theme", newTheme);
}
}
}

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

switchTheme(currentTheme, mainTheme,
getCurrentValue("rustdoc-theme") || getSystemValue() || "light",
false);