qBittorrent icon indicating copy to clipboard operation
qBittorrent copied to clipboard

WebUI: Add color scheme switcher

Open sledgehammer999 opened this issue 1 year ago • 19 comments

Closes #21600

Sometimes I am bored and want to try new things. This time I wanted to code the WebUI.

If user hasn't explicitly set validly his preference -> Use system/browser preference and react to changes to it If user has explicitly and validly set his preference -> Use the user's settings and don't react to system/browser preference change

Obligatory pic: Untitled

sledgehammer999 avatar Oct 14 '24 22:10 sledgehammer999

Honestly, this would look a lot better if you simply had a sun/moon toggle button. No need to take up all that space with text and a checkbox. I'd go with that instead and put the toggle at the bottom right corner. If you want to keep it using text, I'd put it under the view dropdown. Anything but the checkbox and text, which looks very out of place.

Pentaphon avatar Oct 15 '24 04:10 Pentaphon

I would vote for not inventing anything special here. Just make a regular selector in the settings. With clear and obvious options: System, Light and Dark. Same as in #21615.

HanabishiRecca avatar Oct 15 '24 18:10 HanabishiRecca

I would vote for not inventing anything special here. Just make a regular selector in the settings.

Do you mean in the Settings/Preferences dialog or in the menu (eg View)?

sledgehammer999 avatar Oct 15 '24 21:10 sledgehammer999

A regular option in the settings dialog. For example:

image

HanabishiRecca avatar Oct 15 '24 21:10 HanabishiRecca

Its a humble request. Please merge this for webui ASAP. I have chronic floaters in my eye (what are floaters?) and I cant see well in situations like inverted dark mode. Seemingly, the current webui torrent list and rss list look like inverted dark mode and I am so lost when looking at the screen because I cant read. Currently, I am using a android client to connect to the local docker. Due to my setup, I am not able to downgrade as well. Thank you @sledgehammer999 @HanabishiRecca

wolfdael avatar Oct 20 '24 17:10 wolfdael

Wdym by "inverted dark mode"? And if you have issues with dark modes, why do you have the dark mode enabled in your system/browser?

HanabishiRecca avatar Oct 20 '24 17:10 HanabishiRecca

I have addressed the comments.

sledgehammer999 avatar Oct 20 '24 21:10 sledgehammer999

I think I am done with the modifications now.

sledgehammer999 avatar Oct 21 '24 09:10 sledgehammer999

I think I am done with the modifications now.

There is still one issue present - white theme flash on page load when using a dark theme (and vice versa). I can guarantee users will complain about that right after release if not further changes are made.

skomerko avatar Oct 21 '24 09:10 skomerko

I think I am done with the modifications now.

There is still one issue present - white theme flash on page load when using a dark theme. I can guarantee users will complain about that right after release if not further changes are made.

I am not well versed in the webui world. Currently we set the theme inside the "load" signal handler. Aka it runs immediately after the page is rendered. Each time the user changes the setting, the page is reloaded and the signal handler is run again.

What's the preferred solution here for time run and subsequent user change?

sledgehammer999 avatar Oct 21 '24 09:10 sledgehammer999

Currently we set the theme inside the "load" signal handler. Aka it runs immediately after the page is rendered.

It's not.

The load event is fired when the whole page has loaded, including all dependent resources such as stylesheets, scripts, iframes, and images, except those that are loaded lazily.

Which could have a large delay, depending on network connection speed.

If you want to fire immediately after the page is rendered, use document's DOMContentLoaded event.

It is a common mistake to use load where DOMContentLoaded would be more appropriate.

HanabishiRecca avatar Oct 21 '24 10:10 HanabishiRecca

I think I am done with the modifications now.

There is still one issue present - white theme flash on page load when using a dark theme. I can guarantee users will complain about that right after release if not further changes are made.

I am not well versed in the webui world. Currently we set the theme inside the "load" signal handler. Aka it runs immediately after the page is rendered. Each time the user changes the setting, the page is reloaded and the signal handler is run again.

What's the preferred solution here for time run and subsequent user change?

I don't know if it advisable to do so but a solution is:

  1. Drop the defer script attribute from client.js in index.html
  2. Do the setup of updateColorScheme() outside the signal handler for load, in the global scope. It will run as soon as the script loads and before the page renders.
  3. I am not sure how to handle the user changing the setting afterwards. Will the page reload re-run the scripts?

sledgehammer999 avatar Oct 21 '24 10:10 sledgehammer999

No, that's a terrible advice. Just use DOMContentLoaded event instead. I.e. move your init code to somewhere under the https://github.com/qbittorrent/qBittorrent/blob/784a8c777dddf330958274f349ba66124062de03/src/webui/www/private/scripts/client.js#L155

But actually, as we use defer on scripts anyway, even DOMContentLoaded is not really needed. You can just do your stuff inside the main script body.

HanabishiRecca avatar Oct 21 '24 10:10 HanabishiRecca

No, that's a terrible advice. Just use DOMContentLoaded event instead. I.e. move your init code to somewhere under the

https://github.com/qbittorrent/qBittorrent/blob/784a8c777dddf330958274f349ba66124062de03/src/webui/www/private/scripts/client.js#L155

But actually, as we use defer on scripts anyway, even DOMContentLoaded is not really needed. You can just do your stuff inside the main script body.

I can't really check it right now but IIRC by the time DOMContentLoaded will fire, the browser may already have painted page's background. Adding a tiny render blocking script in the head will work for sure.

skomerko avatar Oct 21 '24 10:10 skomerko

Applying the following patch I notice 2 things:

  1. On the very first render I don't see a white flash (with browser set to dark mode)
  2. After a reload/refresh there's a flash of white background
diff --git a/src/webui/www/private/scripts/client.js b/src/webui/www/private/scripts/client.js
index d3a8b783b..684c2cf95 100644
--- a/src/webui/www/private/scripts/client.js
+++ b/src/webui/www/private/scripts/client.js
@@ -25,6 +25,19 @@
 
 "use strict";
 
+// Setup color scheme switching
+const updateColorScheme = () => {
+    const root = document.documentElement;
+    const colorScheme = LocalPreferences.get("color_scheme");
+    const validScheme = (colorScheme === "light") || (colorScheme === "dark");
+    const isDark = window.matchMedia("(prefers-color-scheme: dark)").matches;
+    root.classList.toggle("dark", ((!validScheme && isDark) || (colorScheme === "dark")));
+};
+
+const colorSchemeQuery = window.matchMedia("(prefers-color-scheme: dark)");
+colorSchemeQuery.addEventListener("change", updateColorScheme);
+updateColorScheme();
+
 window.qBittorrent ??= {};
 window.qBittorrent.Client ??= (() => {
     const exports = () => {
@@ -1681,19 +1694,6 @@ window.addEventListener("load", () => {
     window.qBittorrent.Cache.preferences.init();
     window.qBittorrent.Cache.qbtVersion.init();
 
-    // Setup color scheme switching
-    const updateColorScheme = () => {
-        const root = document.documentElement;
-        const colorScheme = LocalPreferences.get("color_scheme");
-        const validScheme = (colorScheme === "light") || (colorScheme === "dark");
-        const isDark = window.matchMedia("(prefers-color-scheme: dark)").matches;
-        root.classList.toggle("dark", ((!validScheme && isDark) || (colorScheme === "dark")));
-    };
-
-    const colorSchemeQuery = window.matchMedia("(prefers-color-scheme: dark)");
-    colorSchemeQuery.addEventListener("change", updateColorScheme);
-    updateColorScheme();
-
     // switch to previously used tab
     const previouslyUsedTab = LocalPreferences.get("selected_window_tab", "transfers");
     switch (previouslyUsedTab) {

sledgehammer999 avatar Oct 21 '24 10:10 sledgehammer999

That's a bit complicated topic yeah.

DOMContentLoaded does not wait for stylesheets to load, however deferred scripts do wait for stylesheets, and the DOMContentLoaded event is queued after deferred scripts.

HanabishiRecca avatar Oct 21 '24 10:10 HanabishiRecca

We could do it vice versa though: load the dark theme initially, then switch to the light. I.e. instead of .dark class there would be .light. Light mode users are not afraid of black screen flashes afaik.

HanabishiRecca avatar Oct 21 '24 11:10 HanabishiRecca

Another way: simply setting class="dark" inside the html also works.

--- a/src/webui/www/private/index.html
+++ b/src/webui/www/private/index.html
@@ -1,5 +1,5 @@
 <!DOCTYPE html>
-<html lang="${LANG}">
+<html lang="${LANG}" class="dark">
 
 <head>
     <meta charset="UTF-8">

HanabishiRecca avatar Oct 21 '24 11:10 HanabishiRecca

@Chocobo1 should I follow @HanabishiRecca recommendation (apply dark class) or do you have something else to propose?

sledgehammer999 avatar Oct 24 '24 11:10 sledgehammer999

@Chocobo1 should I follow @HanabishiRecca recommendation (apply dark class) or do you have something else to propose?

I think it is fine. I would also add a code comment about avoiding the bright flash.

Chocobo1 avatar Oct 31 '24 20:10 Chocobo1

I added the dark class by default to the html.

sledgehammer999 avatar Nov 01 '24 20:11 sledgehammer999

Hmm, seems like all iframe dialogs (download.html, upload.html etc.) ignore selected color scheme. Unfortunately I didn't notice that earlier.

HanabishiRecca avatar Nov 02 '24 21:11 HanabishiRecca

Addressed in a new PR: #21750

HanabishiRecca avatar Nov 02 '24 22:11 HanabishiRecca