Navigating back/forward in rustdocs causes theme change if page is restored from bfcache

[thanatos]

I normally use the "dark" theme for docs.rs ; however, lately, if I hit "back" to go backwards, the previous page is now in light mode, not dark mode. Refreshing the page fixes it, but is annoying, of course.

[jyn514]

Please file this under rust-lang/docs.rs.

[thanatos]

I did some digging: this happens locally as well as in docs.rs. Since rustdoc is in this repo, I think this is the right place for it. (These sorts of things get encountered on docs.rs first … as that gets updated far before my local toolchain.)

The bug is around here:

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

Lines 221 to 228 in aaa9bb9

	function switchToSavedTheme() {
	switchTheme(
	window.currentTheme,
	window.mainTheme,
	getSettingValue("theme") || "light",
	false
	);
	}

Essentially, there are two paths through this file:

1. On initial page load, we run the top-level JavaScript: in particular, we call updateSystemTheme() which detects the system theme (dark, on my system) and sets the page theme appropriately.
2. Possibly (it's up to the UA) on navigation, such as back/forward — if the page is restored from bfcache , the top level JS isn't run. (As the page is stlil, in essence, "alive".) Instead, a pageshow event fires, and that calls the highlighted method above, switchToSavedTheme. That method unilaterally sets a theme — but never obeys the system theme, and thus, we end up with buggy theme switch. (On my system, the getSettingValue("theme") will return null, and so it goes with the default of 'light', despite the prefers-dark-theme pref.)

What I'm not getting is why that event handler is even there. (Edit: ugh, see the next paragraph. The committer of the commit that introduced the bug wrote a good commit message!) bfcache persists the state of the page … including, AFAICT, the adjustments that switchTheme makes to set the theme. I.e., if I comment out the event handler, I get the correct behavior on pageshow events. (I.e., if it's a persisted === true pageshow event, why are we setting the theme? It's persisted.)

So what's the event handler event there for? The commit tells us:

If they have changed theme in the meantime, that means seeing an incorrect theme on the page they went forward or back to. The pageshow event fires on such navigations, so we can update the theme based on that event.

That's true: the theme from the settings might have changed while the page was cached. But we only want to update the theme if that's the case. (In my case, it isn't.) (And the earlier remark about "completely recompute the desired theme" holds, here.)