From ee9e8cd0ec8c204d6c51acbc3663c40d089a3e24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roy=20Wellington=20=E2=85=A3?= Date: Sat, 21 Jan 2023 17:33:03 -0500 Subject: [PATCH 1/3] This function appears to be unused The comment says that it is called from main.js, but there don't seem to be any references to it in main.js. A quick ripgrep says there are no references in all of librustdoc. --- src/librustdoc/html/static/js/storage.js | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/librustdoc/html/static/js/storage.js b/src/librustdoc/html/static/js/storage.js index db2db83ca6310..61856303483e7 100644 --- a/src/librustdoc/html/static/js/storage.js +++ b/src/librustdoc/html/static/js/storage.js @@ -153,22 +153,6 @@ function switchTheme(styleElem, mainStyleElem, newThemeName, saveTheme) { } } -// This function is called from "main.js". -// eslint-disable-next-line no-unused-vars -function useSystemTheme(value) { - if (value === undefined) { - value = true; - } - - updateLocalStorage("use-system-theme", value); - - // update the toggle if we're on the settings page - const toggle = document.getElementById("use-system-theme"); - if (toggle && toggle instanceof HTMLInputElement) { - toggle.checked = value; - } -} - const updateSystemTheme = (function() { if (!window.matchMedia) { // fallback to the CSS computed value From fa214c3b11f6444c268b4ca7d93219df3dc020a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roy=20Wellington=20=E2=85=A3?= Date: Sat, 21 Jan 2023 17:34:30 -0500 Subject: [PATCH 2/3] Fix typo in comment --- src/librustdoc/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index 56b40d8c66baf..2c514a0c8267b 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -509,7 +509,7 @@ impl Options { // these values up both in `dataset` and in the storage API, so it needs to be able // to convert the names back and forth. Despite doing this kebab-case to // StudlyCaps transformation automatically, the JS DOM API does not provide a - // mechanism for doing the just transformation on a string. So we want to avoid + // mechanism for doing just the transformation on a string. So we want to avoid // the StudlyCaps representation in the `dataset` property. // // We solve this by replacing all the `-`s with `_`s. We do that here, when we From 727a1fd1945685f21d4d2b1d86dbf6429372f3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roy=20Wellington=20=E2=85=A3?= Date: Sat, 21 Jan 2023 18:21:43 -0500 Subject: [PATCH 3/3] Keep all theme-updating logic together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this PR, if the page is restored from the browser bfcache¹, we call `switchToSavedTheme`. But `switchToSavedTheme` never looks at the `use-system-theme` preference. Further, if it can't find a saved theme, it will fall back to the default of "light". For a user with cookies disabled² whose preferred color scheme is dark, this means the theme will wobble back and forth between dark and light. The sequence that occurs is, 1. The page is loaded. During a page load, we consult `use-system-theme`: as cookies are disabled, this preference is unset. The default is true. Because the default is true, we look at the preferred color scheme: for our example user, that's "dark". **The page theme is set to dark.** We'll attempt to store these preferences in localStorage, but fail due to cookies being disabled. 2. The user navigates through the docs. Subsequent page loads happen, and the same process in step 1 recurs. Previous pages are (potentially) put into the bfcache. 3. The user navigates backwards/forwards, causing a page in bfcache to be pulled out of cache. The `pageShow` event handler is triggered. However, this calls `switchToSavedTheme`: this doesn't consider the system theme, as noted above. Instead, it only looks for a saved theme. However, with cookies disabled, there is none. It defaults to light. **The page theme is set to light!** The user wonders why the dark theme is lost. There are effectively two functions trying to determine and apply the correct theme: `updateSystemTheme` and `switchToSavedTheme`. Thus, we merge them into just one: `updateTheme`. This function contains all the logic for determining the correct theme, and is called in all circumstances where we need to set the theme: * The initial page load * If the browser preferred color scheme (i.e., light/dark mode) is changed * If the page is restored from bfcache * If the user updates the theme preferences (i.e., in `settings.js`) Fixes #94250. ¹bfcache: https://web.dev/bfcache/ The bfcache is used to sleep a page, if the user navigates away from it, and to restore it from cache if the user returns to it. ²Note that the browser preference that enables/disables cookies really controls many forms of storage. The same preference thus also affects localStorage. (This is so a normal browser user doesn't need to understand the distinction between "cookies" and "localStorage".) --- src/librustdoc/html/static/js/settings.js | 4 +- src/librustdoc/html/static/js/storage.js | 96 ++++++++++++----------- 2 files changed, 54 insertions(+), 46 deletions(-) diff --git a/src/librustdoc/html/static/js/settings.js b/src/librustdoc/html/static/js/settings.js index 84df1b7d3911a..d51cce37d6462 100644 --- a/src/librustdoc/html/static/js/settings.js +++ b/src/librustdoc/html/static/js/settings.js @@ -1,5 +1,5 @@ // Local js definitions: -/* global getSettingValue, getVirtualKey, updateLocalStorage, updateSystemTheme */ +/* global getSettingValue, getVirtualKey, updateLocalStorage, updateTheme */ /* global addClass, removeClass, onEach, onEachLazy, blurHandler, elemIsInParent */ /* global MAIN_ID, getVar, getSettingsButton */ @@ -19,7 +19,7 @@ case "theme": case "preferred-dark-theme": case "preferred-light-theme": - updateSystemTheme(); + updateTheme(); updateLightAndDark(); break; case "line-numbers": diff --git a/src/librustdoc/html/static/js/storage.js b/src/librustdoc/html/static/js/storage.js index 61856303483e7..8836d1b2e464b 100644 --- a/src/librustdoc/html/static/js/storage.js +++ b/src/librustdoc/html/static/js/storage.js @@ -153,63 +153,74 @@ function switchTheme(styleElem, mainStyleElem, newThemeName, saveTheme) { } } -const updateSystemTheme = (function() { - if (!window.matchMedia) { - // fallback to the CSS computed value - return () => { - const cssTheme = getComputedStyle(document.documentElement) - .getPropertyValue("content"); - - switchTheme( - window.currentTheme, - window.mainTheme, - JSON.parse(cssTheme) || "light", - true - ); +const updateTheme = (function() { + /** + * Update the current theme to match whatever the current combination of + * * the preference for using the system theme + * (if this is the case, the value of preferred-light-theme, if the + * system theme is light, otherwise if dark, the value of + * preferred-dark-theme.) + * * the preferred theme + * … dictates that it should be. + */ + function updateTheme() { + const use = (theme, saveTheme) => { + switchTheme(window.currentTheme, window.mainTheme, theme, saveTheme); }; - } - // only listen to (prefers-color-scheme: dark) because light is the default - const mql = window.matchMedia("(prefers-color-scheme: dark)"); - - function handlePreferenceChange(mql) { - const use = theme => { - switchTheme(window.currentTheme, window.mainTheme, theme, true); - }; // maybe the user has disabled the setting in the meantime! if (getSettingValue("use-system-theme") !== "false") { const lightTheme = getSettingValue("preferred-light-theme") || "light"; const darkTheme = getSettingValue("preferred-dark-theme") || "dark"; - if (mql.matches) { - use(darkTheme); + if (isDarkMode()) { + use(darkTheme, true); } else { // prefers a light theme, or has no preference - use(lightTheme); + use(lightTheme, true); } // note: we save the theme so that it doesn't suddenly change when // the user disables "use-system-theme" and reloads the page or // navigates to another page } else { - use(getSettingValue("theme")); + use(getSettingValue("theme"), false); } } - mql.addListener(handlePreferenceChange); + // This is always updated below to a function () => bool. + let isDarkMode; - return () => { - handlePreferenceChange(mql); - }; -})(); + // Determine the function for isDarkMode, and if we have + // `window.matchMedia`, set up an event listener on the preferred color + // scheme. + // + // Otherwise, fall back to the prefers-color-scheme value CSS captured in + // the "content" property. + if (window.matchMedia) { + // only listen to (prefers-color-scheme: dark) because light is the default + const mql = window.matchMedia("(prefers-color-scheme: dark)"); -function switchToSavedTheme() { - switchTheme( - window.currentTheme, - window.mainTheme, - getSettingValue("theme") || "light", - false - ); -} + isDarkMode = () => mql.matches; + + if (mql.addEventListener) { + mql.addEventListener("change", updateTheme); + } else { + // This is deprecated, see: + // https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList/addListener + mql.addListener(updateTheme); + } + } else { + // fallback to the CSS computed value + const cssContent = getComputedStyle(document.documentElement) + .getPropertyValue("content"); + // (Note: the double-quotes come from that this is a CSS value, which + // might be a length, string, etc.) + const cssColorScheme = cssContent || "\"light\""; + isDarkMode = () => (cssColorScheme === "\"dark\""); + } + + return updateTheme; +})(); if (getSettingValue("use-system-theme") !== "false" && window.matchMedia) { // update the preferred dark theme if the user is already using a dark theme @@ -219,13 +230,10 @@ if (getSettingValue("use-system-theme") !== "false" && window.matchMedia) { && darkThemes.indexOf(localStoredTheme) >= 0) { updateLocalStorage("preferred-dark-theme", localStoredTheme); } - - // call the function to initialize the theme at least once! - updateSystemTheme(); -} else { - switchToSavedTheme(); } +updateTheme(); + if (getSettingValue("source-sidebar-show") === "true") { // At this point in page load, `document.body` is not available yet. // Set a class on the `` element instead. @@ -243,6 +251,6 @@ if (getSettingValue("source-sidebar-show") === "true") { // specifically when talking to a remote website with no caching. window.addEventListener("pageshow", ev => { if (ev.persisted) { - setTimeout(switchToSavedTheme, 0); + setTimeout(updateTheme, 0); } });