feat: add isDarkTheme functions and composables

[ShGKme]

☑️ Resolves

• Sometimes we need to check if the dark theme is enabled
• Currently, apps do it in different ways, and sometimes it's incorrect:
  • You should not use window.matchMedia.('(prefers-color-scheme: dark)'). It checks for the user's system theme, but Nextcloud Dark theme could be enabled even on the light system theme.
  • You should not use [data-themes*=dark] or [data-theme-dark] attributes on the body. It checks for explicitly set dark theme, but a user may use the system theme.
• Add utils and composables to check the Nextcloud dark theme based on --background-invert-if-dark
• ⚠️ Requires dark theme support in docs
• ⚠️ Missing tests

🖼️ Screenshots

🚧 Tasks

• [ ] Add functions
• [ ] Add composables

🏁 Checklist

• [ ] ⛑️ Tests are included or are not applicable
• [x] 📘 Component documentation has been extended, updated or is not applicable
• [x] 3️⃣ Backport to next requested with a Vue 3 upgrade

[susnux]

⚠️ Requires dark theme support in docs

So you might want this a review ;) https://github.com/nextcloud-libraries/nextcloud-vue/pull/5656

[ShGKme]

Fixed TS config for styleguidist (see the last commit)

[ShGKme]

Force-pushed docs fixes according to @susnux suggestions

[Antreesy]

backport?

[ShGKme]

/backport to next

[skjnldsv]

• Currently, apps do it in different ways, and sometimes it's incorrect:

  • You should not use window.matchMedia.('(prefers-color-scheme: dark)'). It checks for the user's system theme, but Nextcloud Dark theme could be enabled even on the light system theme.
  • You should not use [data-themes*=dark] or [data-theme-dark] attributes on the body. It checks for explicitly set dark theme, but a user may use the system theme.

Somehow I missed that PR :see_no_evil: Those two sentences are incorrect. The way I designed the dark theme to behave is exactly as it's described above:

1. Dark mode user settings have the priority.
2. Then the default system/browser settings.
3. Finally, the default is false

This should do the trick just fine:

export function isDarkModeEnabled() {
	const darkModePreference = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
	const darkModeSetting = document.body.getAttribute('data-themes')?.includes('dark')
	return darkModeSetting || darkModePreference || false
}

[skjnldsv]

Small addition, considering the method takes an HtmlElement, we also use the [data-theme-dark] attribute to scope down some element and enforce dark theme. So specifying an element should also add a check to climb up the dom tree up to the body and look for this attribute. If not, then we check for darkModeSetting, then darkModePreference, and finally: false

[ShGKme]

Those two sentences are incorrect

They are still correct. If you use matchMedia - you are losing Nextcloud settings. If you are using Nextcloud settings - you are losing setting to use the system theme.

You must always check both.

[skjnldsv]

You must always check both.

No, user setting will overrule matchMedia. https://github.com/nextcloud/server/blob/381077028adf388a7081cf42026570c6be47b198/apps/theming/lib/Service/ThemeInjectionService.php#L34-L61

[ShGKme]

Small addition, considering the method takes an HtmlElement, we also use the [data-theme-dark] attribute to scope down some element and enforce dark theme. So specifying an element should also add a check to climb up the dom tree up to the body and look for this attribute. If not, then we check for darkModeSetting, then darkModePreference, and finally: false

It is covered by --background-invert-if-dark

[skjnldsv]

It is covered by --background-invert-if-dark

But feels incredibly hackish. I'd like to use proper logic instead of relying on custom css queries. Especially considering --background-invert-if-dark have been designed to be removed in the future

[ShGKme]

You must always check both.

No, user setting will overrule matchMedia. https://github.com/nextcloud/server/blob/381077028adf388a7081cf42026570c6be47b198/apps/theming/lib/Service/ThemeInjectionService.php#L34-L61

Then this is not correct?

This should do the trick just fine:

export function isDarkModeEnabled() {
	const darkModePreference = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
	const darkModeSetting = document.body.getAttribute('data-themes')?.includes('dark')
	return darkModeSetting || darkModePreference || false
}

[skjnldsv]

Then this is not correct?

Well, we return the user setting first, then check the system settings ? This is exactly what I wrote ? :thinking:

[ShGKme]

But feels incredibly hackish. I'd like to use proper logic instead of relying on custom css queries. Especially considering --background-invert-if-dark have been designed to be removed in the future

Yes, it is hackish. And it was already the most popular way to check for the dark theme in Nextcloud apps. This is why I though it is important to have it here in the library in one specific place. To fix it only once if we change it, not in all the places in many apps.

We can explicitly go up the DOM tree to check attributes in every node up to the body. But I don't think this explicitness is more important than performance here.

As an alternative, I can propose explicitly setting a custom CSS property for that like. Aka:

[data-theme-dark] {
  --nextcloud-theme: dark;
  --nextcloud-theme-dark: 1;
}

[ShGKme]

Well, we return the user setting first, then check the system settings ? This is exactly what I wrote ? 🤔

In one comment - yes, you propose to return darkModeSetting || darkModePreference || false.

In another, you said "user setting will overrule matchMedia" which sounds like we don't need darkModeSetting when and darkModePreference is enough.

So I'm a bit confused here

[skjnldsv]

We can explicitly go up the DOM tree to check attributes in every node up to the body. But I don't think this explicit is more important than performance here.

const isDarkTheme = !!document.body.closest('[data-theme-dark]'); should be efficient enough :grin:

As an alternative, I can propose explicitly setting a custom CSS property for that like. Aka:

But I like that option better! :+1:

In another, you said "user setting will overrule matchMedia" which sounds like we don't need darkModeSetting when and darkModePreference is enough.

Maybe my var names are confusing, darkModeSetting is the theming app user config. darkModePreference is the system preference.

[ShGKme]

should be efficient enough 😁

Ok, maybe you are right here. Probably el.closest() is even more performant.

But it would still require for additional matchMedia check, otherwise the system theme is lost.

[ShGKme]

Maybe my var names are confusing, darkModeSetting is the theming app user config. darkModePreference is the system preference.

Yes, and according to your code, we still need both.

That's exactly what I stated in the PR - we cannot rely on only one aspect - we need to check both.

[skjnldsv]

https://github.com/nextcloud/server/pull/51702 :) Thanks for the brainstorm!!

[ShGKme]

Ok, maybe you are right here. Probably el.closest() is even more performant.

Checked. .closest() + matchMedia() and getComputedStyle(el).getPropertyValue() is the same in performance.