Keep all theme-updating logic together #107177

thanatos · 2023-01-22T00:03:30Z

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,

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

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.

rustbot · 2023-01-22T00:03:38Z

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-01-22T00:03:41Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

GuillaumeGomez · 2023-01-22T00:09:41Z

Thanks for working on this! The changes look good to me but I need to run tests locally too, to be sure I didn't miss something. In the meantime, cc @notriddle

notriddle · 2023-01-22T01:06:18Z

I've run tests locally: I've managed to reproduce the bug in Safari and Chromium. This fixes it in both, apparently.

I haven't managed to reproduce the problem in Firefox. I think rustdoc trips over some compat heuristic and isn't getting bfcached, so the whole point is moot.

notriddle · 2023-01-22T01:13:04Z

src/librustdoc/html/static/js/storage.js

+        isDarkMode = () => mql.matches;
+
+        if (mql.addEventListener) {
+            mql.addEventListener('change', updateTheme);


For the eslint to pass, you'll need to use double-quotes

Suggested change

mql.addEventListener('change', updateTheme);

mql.addEventListener("change", updateTheme);

There's a few other spots that need it, too.

Thanks! I'll probably not get back to this until the weekend, but when that rolls around, I'll fix this up.

@notriddle Thanks again, I think I've fixed this & the other errors causing es-lint failures.

Cool. Can you squash the fixup commits with the one before it?

https://rustc-dev-guide.rust-lang.org/git.html?highlight=Git#advanced-rebasing gives some information on how to do that.

thanatos · 2023-01-24T04:41:54Z

I've run tests locally: I've managed to reproduce the bug in Safari and Chromium. This fixes it in both, apparently.

I haven't managed to reproduce the problem in Firefox. I think rustdoc trips over some compat heuristic and isn't getting bfcached, so the whole point is moot.

Interesting. Firefox is my current browser, so that's where I see it. My rough understanding though is that entering/leaving bfcache isn't guaranteed to happen, so I suppose it's not out of spec to not see it, too, but it seems to happen pretty reliably for me, so that is still a bit surprising.

(Though, if FF does have a heuristic that it uses … and it happens to be something like "tab pressure", I'm also one of those people with hundreds of tabs open at a time. Although most of them are "snoozed 💤" due to an extension…)

notriddle · 2023-01-24T04:54:58Z

Huh. Weird.

In any case, we've covered all the major engines between us two, so r=me with the eslint stuff fixed.

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 rust-lang#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".)

notriddle · 2023-01-30T14:11:42Z

@bors r+ rollup

bors · 2023-01-30T14:11:44Z

📌 Commit 727a1fd has been approved by notriddle

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#107125 (Add and use expect methods to hir.) - rust-lang#107172 (Reimplement NormalizeArrayLen based on SsaLocals) - rust-lang#107177 (Keep all theme-updating logic together) - rust-lang#107424 (Make Vec::clone_from and slice::clone_into share the same code) - rust-lang#107455 (use a more descriptive name) - rust-lang#107465 (`has_allow_dead_code_or_lang_attr` micro refactor) - rust-lang#107469 (Change turbofish context link to an archive link) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…ddle Clean up eslint annotations and remove unused JS function Thanks to rust-lang#107177 I realized that some eslint annotations might be unneeded so I cleaned them up. I also removed the unused function while we wait for rust-lang#107177 to be updated. r? `@notriddle`

…ddle Clean up eslint annotations and remove unused JS function Thanks to rust-lang#107177 I realized that some eslint annotations might be unneeded so I cleaned them up. I also removed the unused function while we wait for rust-lang#107177 to be updated. r? ``@notriddle``

thanatos added 2 commits January 21, 2023 17:33

This function appears to be unused

ee9e8cd

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.

Fix typo in comment

fa214c3

rustbot assigned GuillaumeGomez Jan 22, 2023

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 Jan 22, 2023

thanatos force-pushed the fix-doc-errant-light-theme branch from a8c80f3 to 7a6d7ff Compare January 22, 2023 00:05

thanatos changed the title ~~Keep all theme-updating logic together]~~ Keep all theme-updating logic together Jan 22, 2023

notriddle reviewed Jan 22, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

GuillaumeGomez mentioned this pull request Jan 26, 2023

Clean up eslint annotations and remove unused JS function #107331

Merged

thanatos force-pushed the fix-doc-errant-light-theme branch from dd5f952 to 727a1fd Compare January 30, 2023 07:37

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 Jan 30, 2023

matthiaskrgr mentioned this pull request Jan 30, 2023

Rollup of 7 pull requests #107472

Merged

bors merged commit d1320a5 into rust-lang:master Jan 30, 2023

rustbot added this to the 1.69.0 milestone Jan 30, 2023

thanatos deleted the fix-doc-errant-light-theme branch January 31, 2023 04:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep all theme-updating logic together #107177

Keep all theme-updating logic together #107177

thanatos commented Jan 22, 2023

rustbot commented Jan 22, 2023

rustbot commented Jan 22, 2023

GuillaumeGomez commented Jan 22, 2023

notriddle commented Jan 22, 2023

notriddle Jan 22, 2023

thanatos Jan 24, 2023

thanatos Jan 29, 2023

notriddle Jan 30, 2023 •

edited

Loading

This comment has been minimized.

thanatos commented Jan 24, 2023 •

edited

Loading

notriddle commented Jan 24, 2023

notriddle commented Jan 30, 2023

bors commented Jan 30, 2023

	mql.addEventListener('change', updateTheme);
	mql.addEventListener("change", updateTheme);

Keep all theme-updating logic together #107177

Keep all theme-updating logic together #107177

Conversation

thanatos commented Jan 22, 2023

rustbot commented Jan 22, 2023

rustbot commented Jan 22, 2023

GuillaumeGomez commented Jan 22, 2023

notriddle commented Jan 22, 2023

notriddle Jan 22, 2023

Choose a reason for hiding this comment

thanatos Jan 24, 2023

Choose a reason for hiding this comment

thanatos Jan 29, 2023

Choose a reason for hiding this comment

notriddle Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

thanatos commented Jan 24, 2023 • edited Loading

notriddle commented Jan 24, 2023

notriddle commented Jan 30, 2023

bors commented Jan 30, 2023

notriddle Jan 30, 2023 •

edited

Loading

thanatos commented Jan 24, 2023 •

edited

Loading