Problem/Motivation

Tested on Firefox Private Mode (to avoid local storage leftovers), with or without ThemeButton island activated:

OS: Light OS: Dark
Default admin theme: Auto ✅ ✅
Default admin theme: Light ✅ ❌
Default admin theme: Dark ❌ ✅
Claro theme ✅ ✅

By design, Shoelace doesn't manage user preferences for color scheme (light/dark), it must be manually managed by the "host" using Shoelace (it is what we do ThemeButton) and injected as sl-theme-dark class ("Light" is the default).

If we disable ThemeButton and tweak a little mechanism to map "Default admin" theme to shoelace, we have good results

Mapped to shoelace OS: Light OS: Dark
Default admin theme: Auto No value or sl-theme-dark, according to browser preferences ✅ ✅
Default admin theme: Light No value (Light is default) ✅ ✅
Default admin theme: Dark sl-theme-dark ✅ ✅
Claro: Light Nothing ✅ ✅

But as soon as we activate ThemeButton island in the profile, we have the initial result again, with color mismatch, even without doing any action in the dropdown.

It is a big deal because ThemeButton has been activated by default since #3630921: Minor fixes post preview/iframe last week, a change neither discussed, nor reviewed. 😔

Proposed resolution

First, we need to disable ThemeButton in the default profile, reducing the hurdle, saving valuable room in the toolbar, and keeping this power feature as an opt-in.

But we need to also address the case when people activate it.

It would be very tempting to simply remove ThemeButton from the codebase, but we will lose the storable user preference (only site admin would decide about the color scheme). So, let's keep it until #3606655: Allow user-configurable UI settings - starting with dark mode is shipped to Core.

Proposal:

  1. Make Shoelace follows Drupal Default Admin color scheme (not the one stored in config, but the ones in the page markup, the difference is important).
  2. Make ThemeButton switch Drupal Default Admin color scheme instead of shoelace (so they always match)
  3. Don't display ThemeButton when theme is not the Default Admin (to avoid switching color scheme in a theme not supporting it like Claro)
  4. Picking "System" option is deleting the local storage (to mitigate the leftovers issue)
  5. No value means following user preferences from browser, instead of Drupal Default Admin color scheme from config (so Display Builder pages can have a different mode than the rest of the Admin UI, at least until #3606655: Allow user-configurable UI settings - starting with dark mode)

So, the island work like that:

ThemeButton option Storage Admin theme Shoelace
Light LocalStorageManager.set('theme', 'light') No class (default) No class (default)
Dark LocalStorageManager.set('theme', 'dark') html.dark-mode .display-builder.sl-theme-dark
System LocalStorageManager.remove('theme)
Dynamic class, according to user preferences from browser

Test with Default Admin & Claro, and switching between them, with different color values.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau’s picture

Issue summary: View changes
pdureau’s picture

Issue summary: View changes

pdureau’s picture

Assigned: pdureau » Unassigned
Status: Active » Needs review

MR open, ready for review.

Changes:

  • New components/display_builder/js/color_scheme.js is making Shoelace follows Drupal Default Admin color scheme
  • components/theme_menu/theme_menu.js has been changed to switch Drupal Default Admin color scheme instead of shoelace, and to delete the storage when the "System" option is picked
  • The island has been removed from config/optional/display_builder.profile.default.yml
  • src/Plugin/display_builder/Island/ThemeButton.php is not displayed when the admin theme is not Default Admin

And the related tests automatically generated by an LLM: tests/src/Kernel/ThemeButtonTest.php & tests/src/Playwright/Tests/color_scheme.spec.ts

Feature review

On a browser Private/Incognito Mode (to avoid local storage leftovers):

OS: Light OS: Dark
Default admin theme. ThemeButton: Light. ✅ ✅
Default admin theme. ThemeButton: Dark. ✅ ✅
Default admin theme. ThemeButton: System. ✅ ✅
Default admin theme. ThemeButton island disabled. Admin theme: Light. ✅ ✅
Default admin theme. ThemeButton island disabled. Admin theme: Dark. ✅ ✅
Default admin theme. ThemeButton island disabled. Admin theme: Auto. ✅ ✅
Claro theme ✅ ✅

Testing this was an heavy task, I may have done mistakes. Feel free to do your own tests during the review.

csakiistvan’s picture

Assigned: Unassigned » csakiistvan
csakiistvan’s picture

Assigned: csakiistvan » Unassigned
Status: Needs review » Reviewed & tested by the community

@pdureau Your screenshots in #6 cover these cases, so I'm not uploading duplicates.

✅ Tested and works — applied MR !528, rebuilt caches and opened a page layout builder (Theme island disabled) and a Views builder (Theme island enabled, set to Dark) with Default Admin set to Light, Dark, and Auto (the latter under both OS color schemes): the builder always matched the admin UI. On Claro the Theme button is not rendered and the builder stays light. No light/dark mismatch left.

pdureau’s picture

Assigned: Unassigned » mogtofu33
mogtofu33’s picture

Assigned: mogtofu33 » pdureau
Status: Reviewed & tested by the community » Needs work

Gin Dark now gets a light builder and no Theme button, and the builder should not write the admin theme's state.

I suggest reading the computed color-scheme of the body instead, so it works on any theme. An explicit pick wins, otherwise the builder follows the page around it.

Also, already on 1.0.x: in Firefox, Shoelace dark reaches prefers-color-scheme inside our iframes, so a front theme following the system, like DSFR, can render dark there. color-scheme: normal on the Builder and Preview iframes stops it.

pdureau’s picture

Gin Dark now gets a light builder and no Theme button, and the builder should not write the admin theme's state.

Gin the contrib theme? It was part of the test cases, but i can add it and address this.

Also, already on 1.0.x: in Firefox, Shoelace dark reaches prefers-color-scheme inside our iframes, so a front theme following the system, like DSFR, can render dark there. color-scheme: normal on the Builder and Preview iframes stops it.

I am not sure I understand. We want the inside of iframe to follow prefers-color-scheme, right? It is already working like that with DSFR (if the UI skins theme is set to "system"). color-scheme: normal stops it, but we are not using this.

I will have a look anyway.

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs work » Needs review

Gin Dark now gets a light builder and no Theme button, and the builder should not write the admin theme's state.

I have added a commt in https://git.drupalcode.org/project/display_builder/-/merge_requests/528/ to support Gin theme from contrib.

Also, already on 1.0.x: in Firefox, Shoelace dark reaches prefers-color-scheme inside our iframes, so a front theme following the system, like DSFR, can render dark there. color-scheme: normal on the Builder and Preview iframes stops it.

UI Suite DSFR seems to follow prefers-color-scheme (when set to "system"). I don' t see what is wrong there. Can you tell me more?

mogtofu33’s picture

Assigned: mogtofu33 » pdureau
Status: Needs review » Needs work

IMHO Gin contrib support is fine, as long as Default Admin is experimental; we drop it in a follow-up once it is stable.

I would still split the two jobs: Shoelace can follow any theme by reading the computed color-scheme of body, which also covers ui_suite themes through ui_skins and Claro, while the Theme button only switches Default Admin and Gin. I changed my mind when no pick, the builder should follow the page around it, not the browser: an admin theme set to Dark should stay dark in the builder, wdyt?

For DSFR: with a light OS and a dark builder, Firefox gives the iframe prefers-color-scheme: dark, inherited from .sl-theme-dark on the iframe element, so DSFR on "system" renders dark inside the Builder.
Chromium follows the OS, which is why it looks fine there.

mogtofu33’s picture

Something else, I would keep the Theme island in the default profile once the mismatch is fixed.

With no pick the builder follows the page and writes nothing to the host, so an untouched button costs only a toolbar slot, and only on Default Admin and Gin.
A visible switch is what users expect, while an opt-in island in profile config is one editors never find.

Its tooltip should say it applies to the builder only, until #3606655: Allow user-configurable UI settings - starting with dark mode lets it drive the core setting.

pdureau’s picture

Currently rebasing (there was some merge conflicts). I will put back in review once it is done.

A visible switch is what users expect, while an opt-in island in profile config is one editors never find.

I honestly don't know if this is what users expects from an out-of-the-box experience. In your extensive survey last summer, we saw some visual building tools with such theme switch, indeed. But there were standalone tools, not parts of a wider Web platform like Drupal is for us.

In the scope of this ticket, I am not personally against putting back the island in default profile.

But I guess it will be removed anyway when we will address UI/UX a last time before shipping 1.0.0, targeting a clean, comforting, securing, first time experience. To convince the newcomers the tool will be easy to use, the less islands in the shipped profiles the better.

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs work » Needs review

Pipeline is green again.

mogtofu33’s picture

Assigned: mogtofu33 » pdureau

Thanks, I pushed two fixes and put the island back in the default profile, since you are not against it for this ticket.
On Gin, the menu tested ginDarkMode, but Gin sets window.ginDarkmode.

With nothing picked, the menu also wrote the OS preference over the admin theme's Light or Dark, so builder pages could differ from the other admin pages: it now applies a stored pick only, and System restores the admin theme's mode.

Drupal itself now treats light and dark as basic: Default Admin ships Light, Dark and System, and #3606655: Allow user-configurable UI settings - starting with dark mode makes it a per-user setting.
Until that lands, the switch fills the gap for the builder only, and with no pick it keeps the admin theme's mode and writes nothing, so an untouched switch costs one toolbar slot.

For fewer islands in the shipped profiles before 1.0.0, I suggest a dedicated issue: list every island of the default profile, and judge each one on criteria we agree on first, such as discoverability, cost when unused, and what comparable tools ship.
Profiles already let a site remove any island, so the question is only the default, and I would like to settle it on those criteria rather than on preference, mine included.

pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs review » Reviewed & tested by the community

Thanks, I pushed two fixes and put the island back in the default profile, since you are not against it for this ticket.

Let's go : RTBC.

  • mogtofu33 committed 443a49db on 1.0.x authored by pdureau
    fix: #3633745 Avoid light/dark mismatch between parts of the UI
    
    By:...
mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks, let's go.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

pdureau’s picture

Status: Fixed » Closed (fixed)

Shipped in beta10 release.