Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Repaint the bike shed #9916

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP Repaint the bike shed #9916

wants to merge 3 commits into from

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Jan 21, 2024

This PR follows on from the discussion in #5497 . Its happening. Lets go.

Shields badges should be accessible by default. The first stated goal of shields is that badges should be

legible: it should be as easy to read the metadata provided by a badge as it is to read body copy inside of a README file regardless of display resolution

The objective of this PR is to improve the colour contrast (and hence accessibility and readability) of our badges. I'm going to do that by:

  1. Adjusting the palette of named/semantic colours we use to render 99% of our "official" badges (that is service badges on a named route).
  2. Using a more pronounced drop-shadow on badge styles that use a drop-shadow (flat, which is the default and also plastic) to achieve greater contrast.

There are a number of things I am explicitly not doing in this PR:

  • I'm not attempting to achieve LC-75 contrast when using the flat-square or for-the-badge styles, which do not use a drop-shadow. The palette changes should increase contrast when using those styles, but in some cases those styles will still not achieve the target.
  • I'm not attempting to achieve sufficient contrast in 100% of cases. There will still be cases where a user can supply a named CSS colour or hex value (either on a custom badge, or as an override) outside the standard palette that decrease accessibility.
  • I'm not making any changes to the brightness calculations we use to automatically switch the text or logo colour when using custom colours.
  • I'm not making any changes to the handful of "official" badges that are using custom hex values outside of the standard palette, for whatever reason.

Those are not problems we will never tackle, but changes to those things should be out of scope for this PR. Lets start off by making a small change that fixes the most common cases to start with. Those issues can be addressed at another time.

As it stands right now, this PR makes the following changes. As we tweak this PR, I will update this:

colours

colour before after
brightgreen
green
yellowgreen
yellow
orange
red
blue
grey
lightgrey

build status

status before after
passing
failing
error

version

version before after
stable
pre-release

coverage

coverage before after
100%
95%
85%
75%
0%

There are a number of things I have not done in this PR:

  1. This PR makes changes to the badge-maker NPM package. I haven't bumped the package version or updated the changelog in this PR.
  2. We're going to need to do some communication around this change. As well as a pinned issue/discussion, one of the things I want to do is write a blog post. I'll write it before we merge this, but lets nail the code changes down first.

Obviously this change is high impact. Merging this will change the base palette for the first time in ~10 years, affect basically 100% of users and everyone is going to have opinions about it. I am confident we are making this change for the right reasons, but I'd like to make sure we have some kind of consensus on the outcome from the maintainer team and wider community before we merge this.

Maintainers: You can spin up a review app to review changes. I'd be particularly interested if anyone can spot any unintended consequences of this work that I may have missed as well as just feedback on the intended changes outlined in the tables above.

@chris48s chris48s added core Server, BaseService, GitHub auth, Shared helpers npm-package Badge generation and badge templates labels Jan 21, 2024
Copy link
Contributor

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against e7b26cc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pinned issue/discussion ... blog post

+1! I was going to offer to write one if you hadn't. 😄

Speaking from past experience: there's a good chance some users are going to be very upset about this change being hoisted on them. The basic morality of accessibility won't change their minds. They'll want the way it was "working" before, and will think it's presumptuous and rude of the badges/shields team to hoist changes on them.

IME a good way to avoid this kind of rage is to:

  • Give people a way to opt into the legacy behavior
  • Make that way force them to understand why the legacy behavior is bad
  • When a tiny % of devs opt into the legacy behavior, use that as justification for removing it in a few months

I.e. here it could be a dull gray/red checkbox on the shields generation page with a link to the blog post.

How does that strike you?

Copy link
Member Author

@chris48s chris48s Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll wait for an opinion from other maintainers, but I think.. not this.

We probably could add a ?palette=legacy or something, but I think I am in favour of just ripping the band-aid off once, rather than having to do it twice. In general, with a service it is hard to gradually deprecate a param in the same way you can with a library. For shields.io users ( https://shields.io/ itself), we don't really have a good way to communicate changes to users. We don't know who our users are (because we're not tracking them) so I think I'd be against introducing any param we plan to remove.

I guess one argument for the legacy palette would be that lots of people use shields badges alongside badges from elsewhere e.g:

Screenshot at 2024-01-22 14-25-38

and you might want to have consistency.

I think I'm OK with not providing a legacy palette and shields.io "leading the way" on this.

As I say, I'll wait for others to weigh in.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 just checking in @chris48s, is there anything I can do to help move this forward?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris48s how much burden would you say we'd have (codebase, runtime, issue maintenance, etc.) from any attempts to support both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not thought about this PR for a while, but I will come back to this and think it through. Just to help me answer this question, what's the scenario we're imagining?

  • We keep 2 palettes forever, or
  • We have both for a while, then at some point we drop legacy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having thought about this...

I don't think the code to enable this would be super-complicated.
I've not tried implementing it, but I think the code to add another param, pass it down and use it in the right place would not be a huge diff. It introduces another global optional param and another library param to document. The core rendering path is code we change relatively infrequently though. As such, I think that represents a relatively fixed overhead from a code perspective. We do it once and it doesn't have much of a maintenance burden.

One thing it does complicate a bit is: anywhere we use a custom colour, we need to be aware that it might appear in the context of 2 palettes.

I don't think it makes a big difference at runtime. I can't see it meaningfully impacting performance, for example.

I think the biggest overhead with doing this would be the documentation, communication and support around it. That tends to be the most labour intensive part on an ongoing basis for something like this. We can document stuff as much as we like, but we'll still have to field support requests about it forever. Unfortunately this is harder to quantify but any global query param tends to raise a fairly steady stream of queries over time.

If we do envisage going down the route of adding a ?palette=legacy param to opt into the legacy behaviour, we could go with @JoshuaKGoldberg 's suggestion and:

  • introduce ?palette=[accessible|legacy] but leave legacy as the default to start with
  • publicise that ?palette=accessible exists and ask for feedback - almost nobody will opt in to start with, but it gives us the opportunity to iron out any unforeseen problems (e.g: further tweaks to the palette) with a small group of enthusiasts before it becomes the default
  • having ironed out any issues, we announce/document that this will become the default on date X
  • one date X flip the default to ?palette=accessible and you can opt out with ?palette=legacy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not thought about this PR for a while, but I will come back to this and think it through. Just to help me answer this question, what's the scenario we're imagining?

* We keep 2 palettes forever, or

* We have both for a while, then at some point we drop legacy?

Sorry i forgot to respond to this, but I was originally thinking forever

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming: what does this look like for very pale colors such as aqua, beige, and lime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So those 3 would look like:

  • aqua
  • beige
  • lime

As I say in the top post

  • I'm not attempting to achieve sufficient contrast in 100% of cases. There will still be cases where a user can supply a named CSS colour or hex value (either on a custom badge, or as an override) outside the standard palette that decrease accessibility.
  • I'm not making any changes to the brightness calculations we use to automatically switch the text or logo colour when using custom colours.

I think those things definitely are worth looking at, but I think they should be follow-up PRs IMO.

@JoshuaKGoldberg
Copy link

Total aside: I'd started working on this (https://github.com/JoshuaKGoldberg/shields/tree/render-shadow) and had a hard time getting the local dev server to show my changes. Very happy you're taking this on. Thanks 😄

@PyvesB
Copy link
Member

PyvesB commented Jan 28, 2024

Haven’t tested this, but I'm on board with the spirit of the changes. At first glance, I'll note that "brightgreen" doesn't feel very bright anymore, and yellow feels a little brown-ish.

@chris48s
Copy link
Member Author

Yeah there's a bit of discussion about the brightgreen here #5497 (comment)

Obviously there's a tension between darkening sufficiently to improve contrast while retaining bright/light-ness

@@ -4,15 +4,15 @@ const { fromString } = require('css-color-converter')

// When updating these, be sure also to update the list in `badge-maker/README.md`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR description you did mention:

This PR makes changes to the badge-maker NPM package. I haven't bumped the package version or updated the changelog in this PR.

so just wanted to add the readme file to that list of "to be updated" given this comment

@calebcartwright
Copy link
Member

At first glance, I'll note that "brightgreen" doesn't feel very bright anymore,

This was the first thing that jumped out at me as well. The other feeling I had reading back over this (and the issue) is that I can't visually differentiate between the changed green and brightgreen any more.

That may just be my screen and/or my eyes, and even though large chunks of the discussions went over my head, the reasoning and calculations behind those seem valid and reasonable so I'm definitely not objecting. That's just my human reaction with a "shields user" hat on that I can't readily tell the difference anymore.

I was trying to think of cases where we have badges with a default color scale that includes both, and I think that's largely found in places like rating/score and code coverage badges with a clear textual value that'll help (e.g. my green coverage badge with 99% coverage may have a background color I can't readily differentiate, but the 100 vs 99 percentage text would definitely avoid me misreading the badge)

@chris48s
Copy link
Member Author

While looking at another issue I noticed

color: '7cd958',

Just chucking a link to this here to remind myself when I get a chance to come back to this.

@chris48s
Copy link
Member Author

At first glance, I'll note that "brightgreen" doesn't feel very bright anymore,

This was the first thing that jumped out at me as well. The other feeling I had reading back over this (and the issue) is that I can't visually differentiate between the changed green and brightgreen any more.

I think there might be a bit of an iron triangle here. There are several properties we want to optimise for:

  1. "brightgreen" should still look/feel bright
  2. "brightgreen" and "green" should be noticibly different
  3. there should be sufficient contrast between the text and background colours

and it is proving tricky to find a solution that achieves all 3 of those at the same time.

I guess one question here is: We've concentrated on calculating the contrast between the background and the text, but we haven't really looked at the contrast/difference between the different colours in the scale (at least not quantitatively). Is there any kind of guidance or formula we can use that might inform how different colours in a scale need to be in order to be sufficiently different from each other, accepting that they might not actually appear together?

That may just be my screen and/or my eyes

Yes. This is good feedback though. The entire point of this is to try and make stuff readable on a wider range of screens/eyes.

I was trying to think of cases where we have badges with a default color scale that includes both, and I think that's largely found in places like rating/score and code coverage

There are a number of palettes where we use "green" and "brightgreen" in the same colour scheme. If you have a look over https://github.com/badges/shields/blob/master/services/color-formatters.js but yeah coverage and ratings would be examples.

@JoshuaKGoldberg
Copy link

Is there any kind of guidance or formula we can use that might inform how different colours in a scale need to be in order to be sufficiently different from each other, accepting that they might not actually appear together?

Yes, and it'd be the same contrast algorithm as other parts. If there is enough color contrast between the two colors using whatever algorithm you choose, they're separate; if not, they would be considered as not being separate. The same APCA resources apcacontrast.com mentioned #5497 (comment) can work in theory, except it looks like the website is down right now. 🥲

@chris48s
Copy link
Member Author

chris48s commented May 7, 2024

Here's another thought on the green/brightgreen thing.

  • We can make the green and brightgreen more different, but doing so requires making the brightgreen darker
  • We can make the brightgreen more bright, but doing so makes the green and brightgreen harder to distinguish

so.. what if this is telling us: this should be one colour?

What if we got rid of the distinction between green and brightgreen and the scale went

  • green
  • yellowgreen
  • yellow
  • orange
  • red

and brightgreen becomes a legacy alias for green

I feel like that requires a bit more of a lift because we change the number of possible buckets we can put a score into and we have to review any scale that currently uses both green and brightgreen. It might also make it more difficult to maintain 2 palettes. I've not completely thought that through, but what do we think of that as an idea? Just reduce the number of colours in the palette.

@calebcartwright
Copy link
Member

Just reduce the number of colours in the palette.

I think you're onto something here, though the devil will of course be in the details. I suspect there will also likely need to be a consideration of the different personas (persona A being a long time shield user who is accustomed to the prior palette and feel like their 100% coverage is visually "wrong", and persona B being a new user who's 👍 with the palette and appreciates the contrast and accessibility aspects)

@chris48s
Copy link
Member Author

It has been 6 months since I last worked on this branch, so it is not really fresh in my mind, but I will try and find some time to get my head back into this and see what's involved in reducing the colour palette so there is only one green.

@chris48s
Copy link
Member Author

OK. It has been a really long time since I looked at this, but I spent a bit of time combing though everything to work it out.

My assessment here is that going down to one green only matters where we define a colour scheme which includes both brightgreen and green. If we define a colour scheme that only includes one of those two colours, then it doesn't really matter.

There are 13 files where we define one or more colour schemes that include both brightgreen and green:

  • services/bugzilla/bugzilla.service.js
  • services/codacy/codacy-grade.service.js
  • services/codeclimate/codeclimate-analysis.service.js
  • services/codefactor/codefactor-helpers.js
  • services/color-formatters.js
  • services/mozilla-observatory/mozilla-observatory.service.js
  • services/ossf-scorecard/ossf-scorecard.service.js
  • services/puppetforge/puppetforge-module-endorsement.service.js
  • services/reuse/reuse-compliance-helper.js
  • services/scrutinizer/scrutinizer-quality.service.js
  • services/security-headers/security-headers.service.js
  • services/sonar/sonar-fortify-rating.service.js
  • services/test-results.js

..and then changing those has knock-on effects on tests. I think we would need to update tests in the following 8 files:

  • services/codefactor/codefactor-grade.spec.js
  • services/color-formatters.spec.js
  • services/dub/dub-download.tester.js
  • services/dub/dub-score.tester.js
  • services/greasyfork/greasyfork-rating.spec.js
  • services/mozilla-observatory/mozilla-observatory.spec.js
  • services/ossf-scorecard/ossf-scorecard.tester.js
  • services/website/website.tester.js

Within the places where we have palettes that use both brightgreen and green there are then 2 cases within that:

  1. Schemes that use 5 colours or fewer: For example this 5 point scale uses both brightgreen and green
    const ossfScorecardColorScale = colorScale(
    [2, 5, 8, 10],
    ['red', 'yellow', 'yellowgreen', 'green', 'brightgreen'],
    )
    but we could substitute in orange to keep it as a 5-point scale.
  2. Palettes that use 6 colours: For example this 6 point scale uses both brightgreen and green
    const color = {
    A: 'brightgreen',
    B: 'green',
    C: 'yellowgreen',
    D: 'yellow',
    E: 'orange',
    F: 'red',
    }[grade]
    but if we collapse brightgreen and green into 1 we need to pick 2 of the 6 letters A-F and give them the same colour. More generally: we have to reduce the resolution of the scale.

When it comes to how to tackle this, I think there are 3 ways we can go:

  1. Just alias brightgreen and green to the same hex code in the accessible palette, but keep them aliased to different colours in the legacy palette and call it a day. This is the quickest and simplest thing we can do. Also this would allow us to maintain the existing palette exactly as it is now alongside a new one where we alias the same words to the new palette. This is also the solution that yields the least useful results IMO as it basically takes the "best" and "second best" value/range and makes them both now the best, and this would indiscriminately apply to both cases (including where we could adjust the palette without adjusting the resolution). I am going to rule this one out.
  2. Individually re-assess each case. There are not a huge number of cases to work though. We can make individual decisions about each scale. Keep 5 point (or fewer) scales where we can. If we have to go down from a 6 point scale to a 5, we can make a case by case decision about how to do it for each scale/service. This will yield more useful results to the user, but the downside is: If we wanted to maintain 2 palettes side-by side (including keeping 6-colour scales in all the cases where we currently use them for the "legacy" palette), this is way more complex. We have to pass make the "which palette are we using" variable available all the way down to the stack the service/helper layer or devise some mechanism to allow every colour scheme to define 2 variants, allow the service layer to pass them both up the stack and then make the decision about which one to use based on the "which palette are we using" variable. I don't really fancy doing either of those, although obviously if we decide we just want to rip the band aid off and change the palette this is a complete non-issue and we just do it.
  3. If we decide we do want to maintain 2 palettes, then there is a halfway house option. First step would be to make another PR which adjusts all the colour scaled highlight above to stop using both brightgreen and green but in the existing (legacy) palette. So all 5 point (or fewer) scales we sub in another colour to take out green. All 6 point scales we reduce the resolution to a 5 point scale and still make an individual decision about how to do each one, but we haven't changed any colours yet. Then we could have one palette where green and brightgreen alias to the same hex code and another where they alias to different hex codes but it doesn't matter because there is no colour scheme where both green and brightgreen appear. That would allow the model where you have a "legacy" and "accessible" palette, but it means that the "legacy" palette isn't exactly what you have now because within that palette every 6 point scale has been reduced to a 5.

Coming back to this with fresh eyes and going through all this code has made me realise a couple of additional things.

The first one is relevant to this issue, and I'm going to chuck it into the mix here while we are still making decisions:

If we're going to reduce the number of colours in the standard palette in the name of improving contrast/accessibility, and we think we should go down from 2 greens to 1 green, should we also go down from 2 yellows to 1 yellow?
With completely fresh eyes on this: It seems to me that if brightgreen and green should reduce to just green, yellowgreen and yellow should also reduce to just yellow.
That gives us even fewer colours to play with in a good --> bad scale so we lose a lot of resolution. You've now got green/yellow/orange/red (so 4 point scale, max) and all of the above needs to be re-worked with yellow/yellowgreen as well ad brightgreen/green but it does feel like it gives you a lot more contrast and allows us wiggle room to pick a really nice yellow. Broadly the 3 process options still apply if we do that.
Is that too bold and radical?

The second one is a complete tangent and we don't need to solve it here, so I'm going to move it to another issue, but while I was working through all this stuff I found that there's a lot of places where colour scales are different for no good reasons. e.g: There are places where we're defining a 3 point good --> bad scale that is brightgreen/yellow/red and other places where we're defining a 3 point good --> bad scale that is brightgreen/orange/red and there is not really any good reason why they are different. This feels like something we can clean up, but lets not do it here. Going down to a max 4 point scale would clean this up a lot though as there is way less wiggle room. I'm going to move the discussion of that elsewhere.

@PyvesB
Copy link
Member

PyvesB commented Sep 22, 2024

I'm personally not sold on the idea of having two different colours palettes and supporting both via a URL parameter. I'd be in favour of just going ahead and making the changes presented in the PR description, possibly making brightgreen a little more bright and yellow a little less brown. We don't need to make things perfect, as long as it's not too disruptive nor confusing, any incremental change that improves legibility is a win in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers npm-package Badge generation and badge templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants