Skip to content

Conversation

@evnchn
Copy link
Collaborator

@evnchn evnchn commented Nov 21, 2025

Motivation

There are 2 problems this PR want to address:

  1. Tell browser when dark mode is enabled #5471, where select browsers and Dark Reader extension do not cope well with NiceGUI's dark mode because we did not use <meta> tags to inform them
  2. ui.dark_mode is hard-coded to use Quasar.Dark.set which gets in the way of using other UI frameworks than Quasar (Let configure UI framework other than Quasar #4859)

Real motivation: to experiment, since I have doubts over "demo is good enough instead of API addition" #5471 (comment)

Implementation

  • Quasar.Dark.set will never be called directly: It will be wrapped to take true/false/undefined, then set as darkSetter
  • darkSetter can be another UI library's dark mode function, if you need that.
  • setDark calls the darkSetter while also handles the <meta> tags for you.
  • To prevent FOUC, the <meta> tags are also populated in HTML.

Progress

  • I chose a meaningful title that completes the sentence: "If applied, this PR will..."
  • The implementation is complete.
  • Pytests are not necessary? (not sure...)
  • Documentation is not necessary? (not sure...)
  • Maybe let's use more unique id names to avoid collision with user's code?

@evnchn evnchn added the feature Type/scope: New or intentionally changed behavior label Nov 21, 2025
@falkoschindler falkoschindler added the review Status: PR is open and needs review label Nov 24, 2025
@falkoschindler falkoschindler self-requested a review November 24, 2025 10:39
@falkoschindler falkoschindler added this to the 3.5 milestone Nov 24, 2025
@davetapley
Copy link

@evnchn well this was fun to test...

TLDR a8fd25b subtly changed the behavior to make my original repro from #5471 not work, so this PR is still probably correct, but in a way I can't verify (without cherry-picking fix back).


I presume the Dark Reader extension has some built in semantics to guess if the page is 'dark', and the changes in a8fd25b make the page settle fast enough for it to automatically detect when dark = True, and so the extension doesn't try to inject its own CSS.

To further complicate things only a hard browser reload would causes the issue (regardless of a8fd25b).

Here's hard / soft refresh before / after a8fd25b:

Screen.Recording.2025-12-17.at.12.35.22.PM.mp4

So, with that said, LGTM 😁

@evnchn
Copy link
Collaborator Author

evnchn commented Dec 18, 2025

a8fd25b

Consider to change your calls to add_css to add_head_html and include the style tag by yourself. This way the CSS is in the head right away and is "slightly earlier"

@davetapley
Copy link

@evnchn to be sure: I'm not doing any custom styling. But that is a good tip.

@evnchn
Copy link
Collaborator Author

evnchn commented Dec 23, 2025

not doing any custom styling
a8fd25b broke the repro

@davetapley Sorry but I can't see how the repro can be broken while you are not using custom styling (so no add_css let alone add_scss/add_sass)

If you really have me guess, then I have to guess that Dark Reader is reacting to the following body snippet:

    <script>
      addStyle = (c) => document.head.append(Object.assign(document.createElement("style"), { textContent: c }));
    </script>

Which I am so shocked if that is the case, think should not be the case, and if that really is the case then bug report to Dark Reader (https://github.com/darkreader/darkreader) is highly recommended.

@davetapley
Copy link

@evnchn hmm, I was using ⬇️ to repro (obvs with dark: True):

Not sure how to proceed.

@falkoschindler falkoschindler modified the milestones: 3.5, 3.6 Jan 8, 2026
@falkoschindler
Copy link
Contributor

@evnchn Since it's your PR: How should we proceed? Are you and @davetapley still testing, or should I review? Personally I'd love to hear that it actually serves a purpose in solving #5471 before spending time reviewing and possibly discussing about API details.

@evnchn
Copy link
Collaborator Author

evnchn commented Jan 12, 2026

Let me clean up the conversation history. @davetapley kindly check 🙏

Context: Test page is dark=True, Dark Reader shall NOT invoke.

  1. How Tell browser when dark mode is enabled #5471 is phrased, it would appear that before a8fd25b, Dark Reader is always invoked. If not I am sure he would note the inconsistency.
  2. Unified setDark API. Happens to take care of meta #5483 (comment) 1/2: After a8fd25b, for some bespoke reason, actually it brought a positive impact, as Dark Reader is invoked only on first page visit, and subsequent visits do not invoke. NOTE: While "broke the repro" was stated, it did not break but rather mended functionality.
  3. Unified setDark API. Happens to take care of meta #5483 (comment) 2/2: After this very PR, Dark Reader is invoked never, which is the desired outcome, hence why "with that said, LGTM 😁" was replied.

@falkoschindler TL-DR: a8fd25b muddied waters by mysterious fixing the Dark Reader invocation except the first one, but the purpose of this PR still stands as it fixes it 100%.

I think we can move straight to review and API design. Though you may want to wait for @davetapley and check out the other PRs (many) in the meantime.

@falkoschindler
Copy link
Contributor

Let's re-schedule this PR to 3.7. At the moment I see the following open questions:

  • Should we add add prefix "nicegui-" to the IDs "color-scheme" and "darkreader-lock"? I do think so.
  • Should we really add a "darkreader-log" tag by default? As far as I understand, the user should be able to decide whether to use darkreader or not. It is generally not recommended to force the user to disable darkreader. While a NiceGUI page in dark mode already looks pretty dark, it might not be the darkreader result the user is hoping for.

@falkoschindler falkoschindler modified the milestones: 3.6, 3.7 Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Type/scope: New or intentionally changed behavior review Status: PR is open and needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants