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

5 small a11y fixes #1380

Merged
merged 6 commits into from
Dec 15, 2023
Merged

5 small a11y fixes #1380

merged 6 commits into from
Dec 15, 2023

Conversation

adamint
Copy link
Member

@adamint adamint commented Dec 13, 2023

  1. the condition select in filter dialog was missing a label
  2. we did not need labels on the divs in source column
  3. adds slightly more contrast to the subtext here: (foreground-subtext-rest)

image
image
vs
image

  1. neutral-layer-4 was just slightly too light in light theme and caused contrast issues between the application title (in logo header) and the background. It has been ever so slightly darkened, small enough that you really wouldn't even notice the change

  2. fixes insufficient contrast in the red fail ansi log by lightening the color. we could eventually turn this into a badge, but it will require re-working the parser (fixes Console logs: colorization readability #1125)

This PR fixes all of the current accessibility issues caught by automated checkers.

Microsoft Reviewers: Open in CodeFlow

@adamint adamint changed the title 4 small a11y fixes 5 small a11y fixes Dec 13, 2023
@tlmii
Copy link
Member

tlmii commented Dec 13, 2023

  1. neutral-layer-4 was just slightly too light in light theme and caused contrast issues between the application title (in logo header) and the background. It has been ever so slightly darkened, small enough that you really wouldn't even notice the change

I'm confused about this one. Are you referring to this?

image

I don't see any contrast issues with that. It needs to be 3:1 and it is more than 4:1:

image

(the text is --accent-fill-rest, which is #6734FF, the background is --neutral-layer-4, which is #D6D6D6 and the font size is --type-ramp-plus-2-font-size, which is 20px)

Though your screenshot seems to say that --neutral-layer-4 is #6F6F6F so I'm not sure if I'm looking at the right thing?

Edit: Wait, maybe those big color images are the changes for the subtext in 3, not the background in 4?

@adamint
Copy link
Member Author

adamint commented Dec 13, 2023

I don't see any contrast issues with that. It needs to be 3:1 and it is more than 4:1 (:

18pt is 24.5px, which is larger than the text size, so the 4.5 ratio does apply!

The ratio between sizes in points and CSS pixels is 1pt = 1.333px

@adamint
Copy link
Member Author

adamint commented Dec 13, 2023

Yes, so sorry! I meant for those images to be associated with # 3.

@tlmii
Copy link
Member

tlmii commented Dec 13, 2023

Apologies, you're right. I glossed over the pt vs px distinction in the rule vs how we did sizing.

That said, I don't think this is the right fix. --neutral-layer-4 is set by the adaptive color system (recipe) in FAST and we should strive not to change the recipe colors as it can cause us more headaches down the road. Theoretically, all the recipe colors are produced so that all the recipe foregrounds work properly with the recipe backgrounds, etc. If we adjust one of the backgrounds (like --neutral-layer-4) we run the risk that one of the foregrounds won't work with it and then we're back in the same spot, likely without realizing it.

The problem here is probably that I used --accent-fill-rest for the font color. That's wrong from the color system's perspective because it's a fill color and so foreground/background contrast with another fill color wouldn't be guaranteed. The right thing to use would have been --accent-foreground-rest (which is #1d00d0) which meets the contrast requirements with --neutral-layer-4. Given how close in name --accent-fill-rest and --accent-foreground-rest are, I'm guessing that was just a typo by me originally

@tlmii
Copy link
Member

tlmii commented Dec 13, 2023

Assuming this

image

Is the header from the details panel on the Trace page (I think that's right), the background is --neutral-layer-4 so if you don't change that color because of my previous comment, then the change to the font color isn't sufficient anymore. Looks like it'd probably need to go as dark as #5D5D5D. Which I think would be fine everywhere else it is used but I'm not certain. Definitely getting a little dark to maintain the visual contrast with the regular text though...

@tlmii
Copy link
Member

tlmii commented Dec 13, 2023

fixes insufficient contrast in the red fail ansi log

We probably need to validate the whole console logs color scheme. It is taken from the Campbell color scheme used by default in Windows Terminal, so there was an assumption that they had taken foreground/background contrast into consideration with their defaults. But perhaps not.

Edit: Not saying you should do that validation all as part of this PR. Just noting we probably need to do a pass on it (and/or figure out why that's allowed to not pass the contrast requirements in another app) at some point.

@JamesNK
Copy link
Member

JamesNK commented Dec 14, 2023

image

What is this tool?

@tlmii
Copy link
Member

tlmii commented Dec 14, 2023

I can't see the image you posted for some reason, but I'm assuming you're referring to my screenshot showing the color contrast checker. That's Accessibility Insights for Windows. The one for web does some good automated checks, but I use the one for windows to do more ad hoc testing.

@adamint
Copy link
Member Author

adamint commented Dec 14, 2023

Which I think would be fine everywhere else it is used but I'm not certain

I just checked, we're still good everywhere foreground-subtext-rest is used. Reverted the neutral-layer-4 change, changed foreground-subtext-rest.

We probably need to validate the whole console logs color scheme

It appears that magenta (#881798) was the only color we needed to change. Updated to #CD13E8

@tlmii

@tlmii
Copy link
Member

tlmii commented Dec 14, 2023

It appears that magenta (#881798) was the only color we needed to change. Updated to #CD13E8

Might want to change light/bright magenta also as that color (#B4009E) is now darker than regular/dark magenta (#CD13E8).

Adam Ratzman false added 2 commits December 15, 2023 12:03
# Conflicts:
#	src/Aspire.Dashboard/Components/ResourcesGridColumns/SourceColumnDisplay.razor
@adamint adamint enabled auto-merge (squash) December 15, 2023 17:04
@adamint
Copy link
Member Author

adamint commented Dec 15, 2023

Good call. Updated bright magenta. @tlmii for re-review

@adamint adamint merged commit 68944fe into dotnet:main Dec 15, 2023
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Console logs: colorization readability
3 participants