fix(theming): Theme specific variables must be set on the root node

[susnux]

• Resolves: Scoping issue when overriding CSS variables for user-themes #36303
• Resolves: Black table on light theme text#3369

Summary

Currently if you use a css variable which links to an other css variable, like --color-main-background and a theme is enabled, the theme is not used for the variable. See linked issue (same e.g. on forms, see nextcloud/forms#1471 (review)).

The problem is the css variable evaluation order, e.g. consider the browser requests dark color theme but the user selected the bright nextcloud theme, then:

• HTML tag
  • defines --color-main-background for dark color theme (as requested by the browser)
  • defines --table-color-background to be the same as --color-main-background
• Body tag
  • redefines --color-main-background to match the theme (because data-theme-light is set on the body tag)
• Table tag
  • Uses --table-color-background which evaluates to HTML-Tag::--color-main-background and in the context it is set to dark instead of bright.

So the solution is to set data-theme-light on the HTML tag rather than the body tag.

Screenshots

System uses dark color theme, but nextcloud is configured to use the bright theme.
You can see the NcSelect has the wrong background color:

before (currently)	after (with this PR)

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[skjnldsv]

@susnux can you explain what the NcSelect is doing differently that makes our variables behave differently in body than html? 🤔

[susnux]

@susnux can you explain what the NcSelect is doing differently that makes our variables behave differently in body than html? 🤔

As explained above it uses variables defined on the root, that evaluate to another variable, see e.g. here.

--vs-search-input-bg: var(--color-main-background); is defined on the root node, so it will not pickup the overridden variable but the default one.
(The value is computed before passing down to the child)

[skjnldsv]

Ok, let me explain my concerns (not founded/verified yet)

Definition

When loading the page, we load (in order)

1. the default light theme on :root
2. the light, dark, highcontrast-light and highcontrast-dark with appropriate media queries
3. the user defined theme with [data-theme-xxx] attribute on body
   

The goal

The goal was to ensure we have priorities and fallback. Since body css vars would always be superior to :root this ensure that:

1. if no user theme is defined (body[data-theme-xxx]), it fallback to any matching media query (prefer-color-scheme or prefers-contrast)
2. if no media query match, we fallback to the default :root light theme

I'm trying to ensure this doesn't create regressions and trouble the css priority.
I don't have much time to fully review this in depth and testing all uses cases :)

[susnux]

The variables from the theme should have precedence over the default ones, because their specificity score should be higher (because of the data tag).

Edit: Well that's not the case, the specificity of :root and [data-...] is equal, so the last one wins.
To prevent regression from loading order the theming css should be :root[data-theme-...].

[skjnldsv]

To prevent regression from loading order the theming css should be :root[data-theme-...]

No, because that would prevent us from applying themes to other areas of the page (e.g., the Viewer is always in dark mode, whatever theme is currently active)

[susnux]

No, because that would prevent us from applying themes to other areas of the page (e.g., the Viewer is always in dark mode, whatever theme is currently active)

More like :root[data-theme], [data-theme] {...}, this allows to set the theme for different areas while still override the root variables if necessary.

[skjnldsv]

My spider senses are tingling. We should be very careful with theming now.
Why not fix the vue-select instead?

https://github.com/nextcloud/nextcloud-vue/blob/093ce19071b66b076b975255810f6f1642eaf855/src/components/NcSelect/NcSelect.vue#L907
Use the root selector (.select it seems) of the component instead of :root ?

[susnux]

It can be fixed on the NcSelect, especially to fix apps for older nextcloud versions.
But having the theming not overriding the root variables will create a pitfall for app developers as it is quite common to use :root { variables} when defining variables (at least I never saw body { variables} or similar).
Currently I only know of the NcSelect and the text app issue, but I guess more issues like this will appear over time.

[susnux]

https://github.com/nextcloud/nextcloud-vue/blob/093ce19071b66b076b975255810f6f1642eaf855/src/components/NcSelect/NcSelect.vue#L907 Use the root selector (.select it seems) of the component instead of :root ?

Some of the variables are for styling the options list, which is not a child of the select but a modal, so styling must be applied to at least body.

[skjnldsv]

It can be fixed on the NcSelect, especially to fix apps for older nextcloud versions. But having the theming not overriding the root variables will create a pitfall for app developers as it is quite common to use :root { variables} when defining variables (at least I never saw body { variables} or similar). Currently I only know of the NcSelect and the text app issue, but I guess more issues like this will appear over time.

Well, locally scoped variables are much cleaner than everyone putting their variables in the root tbh :/
Also one of the reason we decided to scope all of our css in the vue components

[susnux]

I think this is still a thing, maybe not that important anymore but still something we should look into

• if no user theme is defined (body[data-theme-xxx]), it fallback to any matching media query (prefer-color-scheme or prefers-contrast)

• if no media query match, we fallback to the default :root light theme

1. :root[data-theme] has higher priority over :root ✅
2. media queries do not add specificity meaning, so the media query code must always be included after the :root fallback, then ✅

Also one of the reason we decided to scope all of our css in the vue components

BTW this is one thing we lately have problems with because people think that "scoped" rules are 2-way scoped but that is not the case they still share the class selector meaning two components called foo will share the same :deep(.foo) or global .foo styles which might conflict.
(Only solution for real 2-way scoping is using css modules)

[susnux]

I think we can close this