Riverlea - contrast issue on Standaloneusers role permissions screen in dark mode

On investigation, this seems to be a general issue with .text-X utility classes in dark mode - they generally aren't contrasting with the dark mode background. There's not many uses of text-success in core but they all face this issue:

afform character count:

search kit permissions indicator:

I think there are two ways to go for a fix:

a) Expect all streams to set crm-c-success and friends to colours that have sufficient contrast with the background colour in all modes (so in dark mode streams would generally need to set a very light green)

b) Avoid utility classes like .text-X - prefer classes that set crm-c-success and crm-c-success-text. Expect streams to make sure these two colours always contrast.

My preference would be for B - it just seems much simpler. I've had a go at the latter for the specific case of -success and will put up a PR.

I've had a go at the latter for the specific case of -success and will put up a PR.

PR here: https://github.com/civicrm/civicrm-core/pull/32391

Hmm - I was initially looking at the model of .label-success, which essentially does what I was expecting. But I've realised we also have --crm-icon-success-color, which in Hackney Brook at least is already contrasting in light mode/dark mode, and perhaps works better for some cases.

So... it's complicated.

(Oops just realising I failed to tag @nicol before)

So... it's complicated.

100%.

Some context: First attempt to simplify emphasis palettes (extensions/riverlea#29 (closed)) didn't go far enough, this open issue tries again: extensions/riverlea#111.

So before rewriting markup anywhere I'd suggest we try to iron it out as much as possible in RiverLea as detailed in that issue…

The problem is that emphasis colours in civi markup can be applied on regions such as dropdowns or notifications which themselves can have either dark or light backgrounds, and so generic 'crm-success' colours will break in one place or the other, depending on the theme's choices. Markup has been written to 'just work' with Greenwich/Bootstrap, and RiverLea largely matches that for Minetta, but that doesn't ensure anything wrt other streams or darkmodes which introduce a wider range of background/forground combinations.

I think there's a few ways to improve this: have a crm-success-light/crm-success-dark set that alternates with background; force bg and foreground pairs (crm-success-bg/crm-success-text); have variables for every context… RiverLea has been veering towards the third option - hence why there's so many --crm-something-succcess.

Discussion so far suggests second option is best bet, but based on your last text-light/text-dark change I'm wondering if the first option has more advantages.

mentioned in issue extensions/riverlea#111

Actually without solving the bigger problem, I think this issue is just a RiverLea bug:

In RiverLea core we have:

.crm-container .text-success {
    color: var(--crm-c-success);
}

And then in RL core variables --crm-c-success: var(--crm-c-green-dark);

The dark mode version then changes --crm-c-success: var(--crm-c-green-light); AND it swaps the colour values of var(--crm-c-green-dark) and var(--crm-c-green-light) over.

ie it's both changing the semantic name from dark to light, but it's also inverting the values of the light/dark colours! If it just did one of these things, then you'd get the light green loading, but instead it's doing both!

I think it would be good to unwind the dark-light switcheroo thing.

But the more general issue still stands.

Of those options, I think the strong argument for 2 is that is the only one that doesn't involve adding more variables, which I think is a big plus for onward maintainability and customisability. Fewer variables makes it much more manageable for people to create streams, and much easier for overall QA of how streams appear.

I'm not actually sure how option 1 would work. The problem as I see it currently is when someone uses .text-success it could be on any coloured background - so we can't guarantee it will work. It could be dark or light but it could also end up on a green background (e.g. your content is shown in a success message). So it's a dangerous class...

I'd agree with option 2 as well…

The problem as I see it currently is when someone uses .text-success it could be on any coloured background - so we can't guarantee it will work. It could be dark or light but it could also end up on a green background (e.g. your content is shown in a success message). So it's a dangerous class...

TBF that's a constrain inherent in Bootstrap - in that situation you'd be better using either .label-success or .bg-success - and which RiverLea support (tho I think .label-* isn't used anywhere in Civi). .text-success is meant as an inline style that only changes the text colour, so imho it's enough if it follows the same rules as the default text colour: ie sufficient contrast against the various background shades (which, incidentally I like reducing down to filter variations of the same initial shadde, so one custom bg colour (or maybe 2) can ripple into the six page bg variables, but that's a separate issue).

Part of the challenge is old Civi had three emphasis colours: greens for success, red for 'required/warnings/do-not-email/etc' and an amber for lots of info alerts. There's some utility classes: alert success error used in Notifications, but they tended to just trigger icons. Bootstrap then added btn-* text-* bg-* panel-* label-* `alert-* for each of default, primary, info, success, warning and danger. These needed matching to the old Civi layouts, & the patterns used in themes already - like Finsbury Park buttons, as you say.

SearchKit lets you create three different Bootstrap components in the same table row, links use bg-*, buttons use btn-* and menus use bg-* - seen here in Minetta & Thames with emphasis warning, success and danger.

Minetta

Thames

Walbrook

Hackney

TBF that's a constrain inherent in Bootstrap

I think it's an inherent issue in Bootstrap. You see it on this page on those util classes - it fails accessibility checks:

I think in the Bootstrap case the expectation is for developers to use those classes in specific contexts where they know the background. But we are trying to make lots of things interoperable.

Does the link example meet contrast standards? This one looks like it wouldn't pass to me?

I'd suggest we try to steer people away from that use.

I also don't see why we style the button and the menu differently - it would make sense to me to make those consistent?

(which, incidentally I like reducing down to filter variations of the same initial shadde, so one custom bg colour (or maybe 2) can ripple into the six page bg variables, but that's a separate issue).

This would be great. I think you suggested something similar for e.g. borders - think that would be great to set as a default cascade in core (streams can override if they really want, but wouldn't need to, and it would be somewhat at their own risk)

Not that I'm advocating for a JS solution, but just so you're aware, we have a js function called CRM.utils.colorContrast which, given a background color, will output either "black" or "white". It's used for the display of tags, because the color of any tag is user-selectable so the text color needs to be automatically adjusted.

That's nice (I had wondered about the tags!).. I can see that fitting with the customiser potentially. Most buttons/bg with emphasis colours have white or black text - so it could let users chose a colour for their six emphasis colours and Civi would auto-generate the light/dark variable.

I think it's an inherent issue in Bootstrap.

True.

I also don't see why we style the button and the menu differently - it would make sense to me to make those consistent?

I've added all eight variations above - it's only Thames and Hackney where the button and menu differ significantly. But they are different syntaxes: the buttons use btn-* and the menu (and also the table cell colouring) uses bg-*.

There seems to be three different problems swirling around, I think:

some streams have different button styles, which don't work in other contexts (Thames/Hackney)
we try to let Streams set one main colour for emphasis (success/info/warning/etc) - and that's applied to both text on the page background, and regions, like buttons with white/black text on it. Hence Thames btn-danger have a light red bg, while Thames bg-danger has a dark red bg as it's just using --crm-c-alert.
some emphasis colours are used elsewhere. Most notable is --crm-c-alert which also appears in _base.css for errors and deleted functions - this also forces an alert colour that looks ok against light bgs:

.crm-container .del,
.crm-container .crm-is_deleted,
.crm-container table.caseSelector td.status-urgent,
.crm-container p.error,
.crm-container .status-removed,
.crm-container .status-overdue,
.crm-container .status-fatal,
.crm-container .status-hold,
.crm-container .status-past,
.crm-container .crmMailing-error-link .crm-contact-deceased,
.crm-container .status-warning,
.crm-container .crm-error,
.crm-container td.tasklist a:link,
#bootstrap-theme .has-error .help-block,
#bootstrap-theme .has-error .control-label,
#bootstrap-theme .has-error .radio,
#bootstrap-theme .has-error .checkbox,
#bootstrap-theme .has-error .radio-inline,
#bootstrap-theme .has-error .checkbox-inline,
#bootstrap-theme .has-error.radio label,
#bootstrap-theme .has-error.checkbox label,
#bootstrap-theme .has-error.radio-inline label,
#bootstrap-theme .has-error.checkbox-inline label {
  color: var(--crm-c-alert);
}

I was looking at crm-c-alert the other day and wondering: why is it distinct from crm-c-danger?

they are different syntaxes: the buttons use btn-* and the menu (and also the table cell colouring) uses bg-*.

Sure they are different classes and things about the padding etc will be different. But I don't see why we wouldn't use the same vars for their color and background-color? (At least by default, unless the stream specifically wants transparent buttons)

I think that's what's happening, no? Only Hackney and Thames diverge from the matching bg/fg pattern between btn-*, bg-* (and, fwiw, label-*!)

Right - I was thrown off by Thames - is that deliberately different? Or has it just sort of accrued? It looks like a lot of the -btn-x rules around here in Thames are a bit redundant, and could maybe be removed?

Also just looking at Walbrook dark mode, I think the eventual result is that the buttons just match the root colours... but there's some back and forth on the assignments (it seems to be setting the button colours, then sets the root colours to the button colours)

@artfulrobot - questions here are for Rich.

I was working on Thames in parallel with a lot of development by @nicol on the core, and also doing so in a rather strident way, since I had other work on. So I'd say I was hacking more than acting from a good understand of all the crm-c-* variables!

@ufundo if it all works with things removed, then let's remove them!

What is the "matching bg/fg pattern"? I'd like to understand. I'd like Thames only to do it's own thing when I really think it's worth it; I suspect in places it's doing its own thing because I didn't understand how to get it working the 'proper' way.

What is the "matching bg/fg pattern"?

Minetta and Walbrook mostly set one emphasis background/bg colour, ie --crm-c-success --crm-c-primary with a matching foreground/fg colour (--crm-c-success-text) and this is used directly in --crm-btn-success and --crm-btn-success-text.. Hackney and Thames set their own buttons variables, mostly.

So I'd say I was hacking more than acting from a good understand of all the crm-c-* variables!

Me too lol. Probably the bigger problem was you were working on a core that was (and is still) changing –and isn't documented! So much of RL was developed as I went - issues emerging only when coming across something for the first time (ie if I remember right, the different btn variables, for e.g., came from trying to port Finsbury Park).

I was looking at crm-c-alert the other day and wondering: why is it distinct from crm-c-danger?

Yeh this is understandably confusing. 'Alert' is used for what Bootstrap calls 'danger'. There isn't a crm-c-danger. But there is a --crm-alert-background-danger because all of the alert variables used alert in the name, so I didn't want to repeat the word, and chose danger instead. Seemed a good idea at the time - problem started with using btn-cancel as a variable!

Happy to either change to --crm-alert-background-alert for consistency, or change the syntax of all of them to match Bootstrap (tho it'd be a big PR!).

I think it would be good to switch that to danger to match the Bootstrap palette. It's a biggish PR but just a relabel, so should be pretty safe: https://github.com/civicrm/civicrm-core/pull/32409

mentioned in issue extensions/riverlea#123

@ufundo - before I try to fix this, a question:

This issue at heart is because we are using the base epmhasis colours to style both text on default page background, and backgrounds to emphasis regions like alerts, buttons, bg-* etc (screengrab below)

Should we a) require that these base emphasis colours can work in both contexts. ie so you couldn't, say, have a soft pastel palette of emphasis buttons because they wouldn't work on page bg. b) have a third variable: ie success-text, success-bg, success-text-on-bg (normally either black or white).

I thought there was a (c) but I can't think of one now. There's a relatively quick fix to this issue by changing the dark-mode colours and changing how they're used in other context, but that's kind of accepting (a) as a rule.

IMHO:

A = require colour choices to work on both backgrounds is quite constraining
B = add more variables is... more variables, and I think at this stage we should be aiming for fewer rather than more

My suggestion for C would be to deprecate the text-success classes, ie steer people away from setting the text colour on a regular background.

If pushed I think I would prefer A to B. It is probably simpler, and maybe there's a silver lining that where accent colours are used as backgrounds (e.g. for buttons) they should always "pop" from the default background colour.

Yeh - given that btn-etc can overwrite crm-etc (hence how the Thames delete button has a light pink bg, but it's bg-danger regions are dark), A doesn't seem too restrictive.

As for C - the context where I think you will struggle to remove it is Search Kit table output - as SearchKit has been out long enough for there to be instances using this in the wild that will break, and the table links pattern (as opposed to button and menu dropdown paterns - my scrreengrabs show all three) is long established Civi UI, tho admittedly not with emphasis colours.

A pull request related to this issue has been opened with the title: #5798 - improve contrast on standaloneusers role display, and circular icons

instances using this in the wild that will break

It's a pretty soft break though right? Some table links that were coloured would no longer be coloured.

We could manage the deprecation by:

updating all the uses in core, there don't seem to be many
removing the ability to add those text-X classes in search kit, so people cant create more
adding a status check for where people are using it that says "coloured text on regular background can have accessibility issues, we'd recommend upgrading affected search kits on your site: [link to search a] [link to search b] [link to search c]"
at some point later down the line, remove the classes

It's a pretty soft break though right? Some table links that were coloured would no longer be coloured.

@ufundo I've no idea how it's being used so don't want to judge… Maybe something that's currently Red switches to Black and therefore an end user thinks there's not an issue they need to look at, and something important is missed and Bad Thing Happens! I dunno - I feel removing functionality, especially when there's stuff out there that's probably using it - needs a wider discussion, and someone other than just us two to decide it.

But I don't think improving contrast ratio in the mean-time is a problem.

FWIW, adding colour just to an icon, as in your PR is a good example of of where .text-*style is useful. By adding bg colour to the whole cell the layout looks a little odd because each column is a different width. You don't get that with adding colours to the icon, it's a common pattern, provided the contrast is ok.

To be clear, these things sound fine:

fixing the contrast ratio on dark mode text-*style
discourage people from using text-*style

This I've no strong opinion:

remove it from SearchKit UI

This doesn't sound so good:

remove RiverLea support for text-*style so text colours all default to --crm-c-text

It's always a bit uncomfortable to remove support for something... but I would say that lots of Bad Things happen because we have much more code to support than we have the capacity to QA... And it's also current A Bad Thing if people are using these classes in ways that don't have sufficient contrast. And we can manage, e.g. status alerts - it wouldn't be immediate, and there's opportunity for people to say "we really need this"...

I was thinking a migration path could be to use something like color-mix (in srgb, var(--crm-c-success), var(--crm-c-text)) in order to tint that text with a colour, but have it somewhat close to the normal text colour that is known to work on any background. From experimenting, it doesn't always look that beautiful (some of the shades come out a bit muddy) - but it has colour and is readable, so ticks those boxes.

looks a little odd because each column is a different width.

Yes, I would love to be able to make those columns same width... that would improve the readability in either version of the colouring. Do you know if there any utility class we can add to the column to do that?

The text colour shouldn't be used in any critical functional way anywa (as that is already bad from an accessibility perspective) - so I think it's reasonable to push people away from it.

I've just added six new 'Emphasis style' snippets to ThemeTest with all the different colour variations - to make it easier to test light/dark mode impact on all of these classes at once…

Really helpful, thanks @nicol

I just made an MR to add icons as well: extensions/themetest!17 (merged)

I think there's now six PR's related to this from us both:

avoid .text-success => easier assurances about colour contrast - https://github.com/civicrm/civicrm-core/pull/32391
riverlea - rename crm-c-alert and friends to crm-c-danger - https://github.com/civicrm/civicrm-core/pull/32409
improve contrast on standaloneusers role display, and circular icons - https://github.com/civicrm/civicrm-core/pull/32411
Colour Contrast Darkmode issues - https://github.com/civicrm/civicrm-core/pull/32413
use crm-c-X-text for .X-text - https://github.com/civicrm/civicrm-core/pull/32418
canonise 3 vars per emphasis colour - https://github.com/civicrm/civicrm-core/pull/32419/files

@ufundo - what do you say we consolidate around PR 6? If we put PR 2 in there as well - and then add the simplification around icon emphasis colours, it's all in one branch/PR to do some proper testing across the streams.

ie for reviewers, many of these PR's look epic and potentially breaking, so all need some solid testing, therefore we might as well bundle them together in a single RL 1.4.2 release focussed on emphasis colours and variable naming consolidation, and then the testing (and any related fixes) can all happen just once?

I realise the issue at the heart of this is a visual/darkmode regression, and the fix we're ending up with is almost like a refactor, so is there also an argument to just merge the small PR number 3 above into 6.0/6.1, even tho it will be changed in 6.2?

(if it'd be helpful to have a quick call just to finalise variable names/process am around today - I think we should also document this.. somewhere..)

Hi @nicol thanks for summarising!

On a train rn but could maybe do a call around 4pm if helpful?

In the meantime, I think:

it makes sense to target 3 to 6.0/6.1 as it is a narrow immediate fix => I've redirected that PR now
it would much better to merge 2 first and separately to any of the others. More, smaller PRs give us much better commit history / debuggability / assurance than combining the relabelling (which should have no functional impact) with the more involved refactoring

Of the remaining 4, I agree 6 is probably the best of the bunch for now. But as I said above I would like to interrogate dropping .text-X more before spending lots more time on it.

it would much better to merge 2 first and separately to any of the others. More, smaller PRs give us much better commit history / debuggability / assurance than combining the relabelling (which should have no functional impact) with the more involved refactoring

I get that with conventional code - you wouldn't merge four functions at once. But I'd estimate it takes two hours to do a manual review of all streams light and dark - and I think there's enough variables being changed that this needs a full manual review. I don't think it's sensible to do that multiple times - but there needs to be a review on the PR, along with needed fixes.

So I guess I'm a little less confident in the idea of the "full manual review". I think even when we try to do that, we still miss things. IMHO we'd be better served by trying to isolate changes, which then allows a) focusing review on the things that a PR should change; b) pinpointing regressions when they inevitably do occur (this regression comes from this specific PR, so we need to look at these specific changes).

Appreciate that's a bit of a personal preference though, can try to merge 2 + 6 next week if that is the way you want to go?

I'm just sharing my experience from making this and finding regressions in the last place you expect. To give an example:

you made a PR changing variable names: https://github.com/civicrm/civicrm-core/pull/31994
I did a manual test of that PR across all 8 stream light/dark variations and found over a dozen regressions which I fixed: https://github.com/civicrm/civicrm-core/pull/32209
And we still missed a regressions on the Extensions screen, because that wasn't on my review list: https://github.com/civicrm/civicrm-core/pull/32381

The way to speed up RiverLea/front-end review process is to automate screengrab generation with something like BackstopJS - REF: extensions/riverlea#33.

Right, that's what I mean about still missing the Extensions screen. That's why I think it's better to merge things into master - then everyone who is doing dev is testing across many different screens / environments etc and will pick things up (many eyes is best), and they can be fixed before they make it to an actual release.

Automated screengrabs would be awesome.

I also think getting the Previewer merged would go some way to speeding testing, but sitting in the review queue.

A pull request related to this issue has been opened with the title: #5798 - fix contrast on standaloneusers role display

Thinking some more about the 3-variable option - there's still a problem that someone can use text-success somewhere the background isn't the predominant page colour. For example, if you use row formatting on search kit, the row might have a yellow background. Then even if you have a shade of green that works on the normal page background, it could disappear.

You could say "well that's your fault when you built the searchkit". But you might not see that it in the preview when building your search kit, because you have some conditional formatting rules which mean that only happens if a row comes along that matches two rules.

I just don't like how text-success opens up the possibility for that kind of error.

mentioned in commit e4ce0e91

I just don't like how text-success opens up the possibility for that kind of error.

@ufundo but it's already in use. You opened this issue because RiverLea does not support it in dark mode. That to me is a regression that has to be fixed. Phasing out the use of text-* because people might use it in ways that clash with the background colour is a separate point - and in many ways is not a theming question but about markup/Civi-style-guide* and would have a slower phase-out. I'm happy to stay out of that decision as you feel strongly about it.

I just want to make sure that the ways that Civi is currently used are supported by RiverLea and text-[emphasis] is currently used in a number of places. Piggbbacking a functional change to Civi core on this theme bug feels like it eats up a lot of time for us both.

* fictional thing, but would be nice if there was one!

Just repeating my comment from before about the PR that was just merged as it's easily lost as it was a reply.

FWIW, adding colour just to an icon, as in your PR is a good example of of where .text-*style is useful. By adding bg colour to the whole cell the layout looks a little odd because each column is a different width. You don't get that with adding colours to the icon, it's a common pattern, provided the contrast is ok.

mentioned in commit 7f5d4eab

added comp:Riverlea triaged labels

Riverlea - contrast issue on Standaloneusers role permissions screen in dark mode

Child items ...

Activity