backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

[A11Y] Native dark mode support (at least for the even admin theme) #4778

Open jlfranklin opened 3 years ago

jlfranklin commented 3 years ago

Dark mode has become a popular feature for all electronic mediums, from desktops to mobile apps to web. Modern browsers broadly support the preferred-color-scheme @media selector allowing a site to automatically match the user's device preference, and many websites allow a user to forcibly set dark mode, such as the StackExchange family of sites.

However, it can't be done by the themes alone. The backdrop_get_logo() function returns a single graphic without consideration for the light or dark mode. The Backdrop logo, for example, works well with light mode pages, less so with dark mode.

To fully support Dark Mode in Backdrop, the following items would need to be included:

See also #3990 and #3991.

klonos commented 3 years ago

Thanks for opening this request @jlfranklin ...seems that there's lots of things to consider and implement, but we can take baby steps. Perhaps this issue can serve as a meta?

jlfranklin commented 3 years ago

Making this a meta is a good idea. I'm sure that's not everything we need to do, and we'll be adding to the list as we dive into it.

All I wanted was to serve up the Silkscreen logo on my own site with either black or white text, depending on where it was displayed. At first, I thought I'd just add a second dark mode logo to the system settings page, and make backdrop_get_logo() a little smarter, returning the right one based on the theme. To do that, backdrop_get_logo() would have to know if the theme is natively a light or dark theme, or support switching to dark mode, which should be a setting somewhere in the theme object or determined by a new theme_has_dark_mode() function, similar to theme_has_color_support().

How would theme_has_dark_mode() even know? Look in the theme info for stylesheets[dark]? The others (all, 'screen, and 'print) are media types. Is there a dark mode media type? (research CSS dark mode, find the preferred-color-scheme @media ... not exactly type.) And that's not a good indicator, as a theme can be natively dark and not have anystylesheets[dark] entries.

Or maybe backdrop_get_logo() should return both, and let the caller choose the correct path. Or take an optional argument to ask for the dark version.

A couple hours later, it was clear to me this would take a lot of careful planning, expertise in CSS, and intimate knowledge of the theming layer to do this right.

klonos commented 3 years ago

Might be worth checking how it's done in https://www.drupal.org/project/gin for example. I mean, if their dark mode is an entire subtheme with CSS overrides, or a different color scheme etc.

indigoxela commented 3 years ago

Might be worth checking how it's done in https://www.drupal.org/project/gin

They're doing it with a CSS class added to body (gin--dark-mode). Not sure if that's the best approach for Backdrop.

jlfranklin commented 3 years ago

They're doing it with a CSS class added to body.

We already add page-wide status (.node-type-post, .has-flexbox), adding a body class makes sense as part of the solution. There would still need to be something like backdrop_user_dark_mode() returning the user's preference (dark, light, auto).

jlfranklin commented 3 years ago

indigoxela added the contrib candidate label

Adding support to Color may require some in-core changes. Most of it can be done with a lot of hook_form_alter() to rebuild the Color config pages, or possibly duplicate them for dark mode.

I expect updating core themes will need core patches. So will the caching layer.

It is possible we can do more modest changes in core -- things like adding some backdrop_alter() calls in key places and adding some pre-defined cache tags -- then leave the rest to a contrib module. We'll find out as we get into it.

indigoxela commented 3 years ago

Adding dark mode support to core themes will be difficult. There are already several discussions in this issue queue about how CSS changes can be made without breaking stuff for contrib or custom subthemes.

Adding a CSS class to the body needs no core change, hook_preprocess_HOOK() handles that.

Regarding changes in caching. I wouldn't recommend to change something substantial like that just for a dark mode. I'd rather use Javascript and cookies.

We'll find out as we get into it.

Exactly. :wink:

jlfranklin commented 3 years ago

There is a Drupal 8 Dark Mode module, but it just switches the active theme based on a schedule. Porting that won't accomplish what we're trying to do here.

domaingood commented 3 years ago

I think we have have more work then make dark mode in core.Its not important now.

docwilmot commented 3 years ago

A quick solution that would allow contrib to do the heavier lifting could be to add a setting on admin/config/system/site-information to "Allow theme modes" and leave that disabled in new installs. That would activate a theme settings checkbox per theme, and support for that to the logo block. So if the user had a (contrib) theme that supports dark mode he could turn that on. Then of course we can add dark mode to Basis and Seven at our convenience.

indigoxela commented 3 years ago

Not that I even consider working on it (not enough time, other priorities), but I'm pretty sure that everything could be done in contrib, with a combination of a theme with a module. The theme provides the stylesheets, the module lets users configure their preferences.

docwilmot commented 3 years ago

That is true. Hadnt considered a contrib module, maybe one that alters the logo and other necessary blocks.

jlfranklin commented 3 years ago

Adding a backdrop_alter() inside of backdrop_get_logo() would make it possible for a contrib module to update which logo is shown based on active light/dark modes and add specific light/dark logos to the returned array.

indigoxela commented 3 years ago

Adding a backdrop_alter() inside of backdrop_get_logo() would make it possible for a contrib module to update which logo is shown

The way I understand the intention behind "dark mode" is that users can switch it dynamically, maybe based on preference settings, which makes it impossible to do that in php. There's no per-user-cache :wink:. But I really didn't take a closer look at the dark mode thing.

jlfranklin commented 3 years ago

Disclaimer: IANAFEDev.

From my limited reading, there seem to be a couple common tools and techniques for handling dark mode.

One is using a class on the body tag, and sending four sets of CSS, one where all selectors start body.light-mode, a second where all selectors start body.dark-mode, a third for body.follow-device-mode, and a fourth with non-color related CSS (position, margins, font-size, etc.) For .follow-device-mode, the CSS relies on the preferred-color-scheme media selector to choose which CSS to apply, meaning that third set of CSS is a copy of the first two sets wrapped in @media selectors.

After that, add a control somewhere with a set of radios and a couple lines of JavaScript to let the user (or theme developer) change the color scheme by updating the body class. If we don't provide a means of live-switching, then we can reduce the amount of CSS sent to the user by sending only the color-scheme CSS that matches the body class.

Some tags have features that can help. The picture element supports choosing a source based on media type, and that media type can be prefers-color-scheme: [light|dark|no-preference]. If you're using img tags, then some images will need to be added twice to the HTML, one with class="dark-mode" one with class="light-mode" and a CSS rule body.light-mode img.dark-mode { display: none; } Others may simply need one img tag and a rule like filter: brightness(.8) contrast(1.2);.

(Note to self: check if the Image field can be patched to use the picture element or support multiple img tags via contrib?)

There's no per-user-cache.

True, CSS/JS aggregation and caching is done by creating a hash of the filenames included in the aggregated file. The right color scheme would be picked up by virtue of the color scheme CSS's filename.

I was thinking about the block system's BACKDROP_CACHE_xxx tags: PER_ROLE, PER_USER, PER_PAGE, GLOBAL, and CUSTOM. Would a PER_COLOR_SCHEME be useful to add? It would only apply to CSS added to the block inline, not attached as a file. Color CSS is probably pretty rare inline, so the arguments for/against would come down to Correctness vs Complexity and ROI.

Blocks could use the PER_USER tag, and cache the inline color CSS per-user. (How would this work for Anonymous users?) A PER_COLOR_SCHEME cache tag would prevent bloating the cache with all those per-user copies. Argh, no it wouldn't work well. Color scheme is a separate dimension from role/user/page/global. It's more accurate to cache ($role, $color_scheme) than $color_scheme OR $role. Individual editors or administrators should be able to choose their own color scheme. A more generic cache tagging system would better serve such a need.

ghost commented 3 years ago

Backdrop core includes a lot of icons that are black PNG images (e.g. /admin/dashboard, /admin/config). These would need to be replaced to properly support dark mode. I've added a checkbox to the OP for this.

klonos commented 2 years ago

Relevant: https://stackoverflow.com/questions/52388490/php-how-to-detect-that-users-computer-browser-is-in-dark-mode

allsite commented 2 years ago

Loosely related, I made a little module that turns any admin theme into dark mode. It first uses js to invert the visual page elements. Then it uses css to invert back the image elements. It's very light-weight and I built it just to save my eyes. Native support would be a great feature! image

indigoxela commented 2 years ago

Maybe also related: there's a frontend theme with a dark mode switcher: https://backdropcms.org/project/monochrome

So it seems that things got done in contrib land. :wink: As there's obviously no change needed in core - can we close this issue?

ghost commented 2 years ago

@indigoxela Core still includes dark PNG icons that don't work in dark themes (see my comment above), so this issue should stay open at least for that.

klonos commented 1 year ago

Just noting that thanks to @laryn and @saschaeggi (the original theme maintainer) the Gin theme was ported over from Drupal 9 to Backdrop! 🎉 ❤️ ...mentioning this here because that theme has dark mode support:

https://github.com/backdrop-contrib/gin/issues/15

image

I haven't looked at the code yet, but what would belong in core as part of this request here would be this bit (as outlined in the list of items in the issue summary):

  • A system-level, user-overridable setting (similar to timezones) for dark mode allowing for "light", "dark", or "auto" (follow device preference) that modules and themes can use to include the appropriate CSS.

So the actual dark mode implementation would still be left to each contrib theme - what would be in core would be the easily-added dark/light/auto switch in the settings form, and the "internals" that would make dark mode CSS switching work.

klonos commented 1 month ago

@wesruv brought this issue up during the Design/UX meeting earlier today. The discussion starts at 43:22 into the recording. Here: https://youtu.be/l2SAcL907AQ?t=2602

It was mentioned, that there's very good support for light-dark() in all browsers now, and for older browsers/devices that do not support it, we can add another good-old color line before the one that uses light-dark(), and since browsers skip CSS selectors/rules that they don't understand, that would work as a fallback mechanism. For instance, something like this:

:root {
  color-scheme: light dark;
}

...

.my-element {
  background-color: #000000;
  background-color: light-dark(#000000, #ffffff);
}

Some resources around this:

There seemed to be consensus in the meeting that we should be able to achieve this in core; at least for the admin theme. So I'm removing the contrib candidate tag from the issue. Also adding [A11Y] to it.

saschaeggi commented 1 month ago

I looked into this new way of handling colors for Gin as well a while ago and was really hyped but when looking a bit deeper into it and how to integrate it we decided (at least for now) against it as it doesn't really provide a benefit as you have to define and use this syntax everywhere to make it work:

light-dark(#aaaaaa, #bbbbbb);

which you'll end up with a lot more CSS code as you have to use light-dark() everywhere you use a color to not break Darkmode. While you can easily define colors this way with the same level of control (or even more, see below) but less code:

:root {
  --var-1: #aaa;
}

.dark-mode {
  --var-1: #bbb;
}

.high-contrast-mode {
  --var-1: #000;
}

.my-class {
  background-color: var(--var-1);
}

The only thing left then is to add a small handler to check for custom set Light/Darkmode or when set to automatic to check for prefers-color-scheme. You can have a look at how we handle this in Gin. This way to only need to use var() with one value all over and you can also let users manually enable/disable Darkmode (as maybe I have my system in Darkmode but I prefer Backdrop in Lightmode).

cc @laryn

wesruv commented 1 month ago

@saschaeggi Is that because you already have dark mode support and adding light-dark() would be more of a headache?

With Backdrop having nothing for dark mode right now, or any CSS custom properties in core, light-dark() seems like a better solution for core for now.

Getting a CSS Custom Property standard and figuring out the implementation feels like a much higher bar to me, but maybe I'm missing something?

-- Edit Addition -- I'm thinking getting this working in the default admin theme would be a great first step.

laryn commented 1 month ago

@wesruv I think the point (or part of the point) is that you can't really do either method halfway. If you have some color definitions that are configured for light or dark mode, and some that aren't, things are going to look pretty messed up on at least one mode (probably dark mode). So the implementation of either from scratch is a bit of a lift, but using custom properties as described makes long term maintenance much easier as your colors are all defined in one place (in a central CSS file with the custom property definitions) and you don't need separate code plus a fallback every time you want to use a color.

Agreed, it would be awesome to have light and dark modes available out of the box! 👍

saschaeggi commented 1 month ago

tl;dr: I'm only sharing my personal experience with what has been the easiest way to integrate a Darkmode (as I have worked on integrating Darkmode into multiple products like Drupal, GitLab, etc.).

and you don't need separate code plus a fallback every time you want to use a color.

I think @laryn did bring up another fair point which the proposed method lacks: ease of use for developers. If you decide to use the light-dark() syntax, every contrib module needs to also adapt the use of this syntax which adds:

While being able to just use a single CSS3 variable on the other hand is pretty fail proof and easy to adapt. Devs just need to swap their fixed color vars with the CSS3 var() equivalent provided by core.

I think it's important to provide an easy way which will allow maintainers of contrib modules to easily adapt Darkmode for their modules.

wesruv commented 4 weeks ago

So I'm not saying I don't want to use any CSS Custom Properties, more that I don't want to rely solely on CSS Custom properties for dark mode. Definitely hear the maintenance concern of having static colors everywhere, especially with multiple color modes.

Getting basic colors of the main palettes in vars feels like a great idea as part of this effort.

If we relied solely on Custom Props, my concern is the added work of having a good naming and inheritance scheme for all colors (backgrounds, font color, borders, box shadows, etc) would:

I think dipping our toes in CSS Custom Props in core to see what works/what doesn't will get us dark mode faster, and create lower risk of architectural tech debt.

wesruv commented 3 weeks ago

I made some great progress yesterday: https://github.com/backdrop/backdrop/pull/4818

My hesitance for "too many CSS Custom Properties" seems unwarranted, colors seem to be tied to their function pretty well, so making variables by color and switching them on dark mode seems to work 90% of the time.

I'm reducing the number of colors, I used grep and made a spreadsheet to count the number of times a color occurs in the theme: https://docs.google.com/spreadsheets/d/1SJYu8-jjysHA3ECONJpcNcyhgvxbxQq_P-lEJQ6s604/edit?gid=2134783428#gid=2134783428

Started with 58, I found at least 10 colors that were so close to another color that I got rid of one.

All colors are defined in seven.palette.css, so far I'm not feeling a need for functional palette vars (e.g. link-color or button-background as opposed to red-50).

Sometimes the inverse color doesn't quite work as well, so in those scenarios I'm using light-dark to choose a different palette color.

Other times I'm not sure how the color fits into the system so (at least for now) I'm not converting them to a variable.

I've clicked around on all the major screens and think this is at MVP stage, I think I'm going to mess around with the palette vars a bit more, there are some 'grays' that have some color, and I want to figure out if they need that, or if I can simplify a bit.

I'm also thinking the status color palettes and the secondary accent (lime green) need a bit more love, I might have oversimplified some, and I think there are more shades, but I haven't found where they crop up.

One last thing, is I'm wondering how and if I should be doing any of this in the core, non-theme, stylesheets. E.g. should system messages base stylesheet have some defaults? If so, does it use CSS Vars? If so where do they originate (the theme? Does core have some of it's own? etc). Not sure that we have to solve that for Seven to get dark mode support, and could punt this problem down the road a bit after we see how this works.

wesruv commented 3 weeks ago

Please feel free to start testing this! I probably wouldn't deploy it to a customer site yet, but it is feeling very solid so far.

To use it, you can just copy the seven theme folder from my branch and paste it over your own. I don't think I'm ready for other's contributions yet, in case I rework the palette var naming/organization, it could get tricky to merge. Hope to get to that point soon though.

laryn commented 3 weeks ago

I updated the branch with the latest commits from core so that the Tugboat sandbox would build and people can test there.

indigoxela commented 3 weeks ago

Started with 58, I found at least 10 colors that were so close to another color that I got rid of one....

Yeah, Seven's "50 shades of gray" - that's something that could get simplified a lot, I think. :grinning: And then Layout ships with own sets and then there's Views UI (with its contrast probs)... Less is better. Even now there's lots of those grayscale vars (15 - really?).

Some don't seem to fit well, yet... like this border color (I mean, who died? :stuck_out_tongue_winking_eye:):

border-too-dark

But overall the progress here is great! :tada: Dark-mode support works fine.