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

make dropdown arrow larger to meet accessibility requirement #127839

Merged
merged 2 commits into from Jul 12, 2021
Merged

make dropdown arrow larger to meet accessibility requirement #127839

merged 2 commits into from Jul 12, 2021

Conversation

alanrenmsft
Copy link
Contributor

@alanrenmsft alanrenmsft commented Jul 2, 2021

this is a fix for microsoft/azuredatastudio#15767 in azure data studio, but I think this is also good for vscode.

before:
image

after:

image

@isidorn
Copy link
Contributor

isidorn commented Jul 2, 2021

I like this change. Thanks for the PR @alanrenmsft 👏
However leaving up to @misolori to review if he would prefer that we change the color, and not to make it bold.
Both work for me.

Assigning to July milestone since we are not wrapping up June. Miguel please merge once you are happy since in July I am on vacation.

@isidorn isidorn added this to the July 2021 milestone Jul 2, 2021
@isidorn isidorn added the accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues label Jul 2, 2021
@miguelsolorio
Copy link
Contributor

Hmm...personally not a fan of this change as that makes the drop down chevron a different stroke width to the rest of the chevrons used elsewhere, which is inconsistent.

Not sure I follow the logic of making it bold, wouldn't that mean you'd need to make the text also bold? When inspecting with dev tools it tells me it passes the contrast ratio.

@isidorn
Copy link
Contributor

isidorn commented Jul 2, 2021

@misolori good point. That's why I mentioned above that a color change would make more sense. And not making everything bold.
I think we can customise these colors.

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Jul 2, 2021

Also worth noting that all of our icons are using the same color as the foreground, which already needs to pass a 4.5:1 contrast ratio for regular text. So I'm a little confused as to why our text passes but the icons don't pass (all icons are using the same stroke width and color).

@miguelsolorio
Copy link
Contributor

To me this feels like a discrepancy on the dropper inspector as all contrast tools ask for the hex colors, which do pass on our side:

CleanShot 2021-07-02 at 05 50 07@2x

CleanShot 2021-07-02 at 05 50 24@2x

@alanrenmsft
Copy link
Contributor Author

@isidorn @misolori below is the reply I got from the accessibility team, , this dropdown icon looks lighter because there are almost no straight lines and none of the dots get the full color. I agree there are for sure other icons with the same problem, maybe we should make the font weight for all codicons thicker?

image

@miguelsolorio
Copy link
Contributor

Taking another look, when I use the dropper I'm still seeing different results:

image

How do we determine whether something passes when we all have different results?

@alanrenmsft
Copy link
Contributor Author

alanrenmsft commented Jul 8, 2021

@misolori are you zoomed in? on Windows (Light+ theme) I am getting the following and even the darkest dots are less than 3.1:1.
image

I compared the differences between the dropdown chevron and the other one to the right of it, the other one looks darker than it because it is slightly larger. I think instead of making the font bold, we should increase the size by 2px, from 14px to 16px, then the contrast ratio will be ok. what do you think?

before size increase:
image

after:
image

@miguelsolorio
Copy link
Contributor

No zoom here, I'm on a Mac using the default light theme as well.

Glad you spotted the font size difference, they should be the same (16px) so that's an easy enough change we can do that to increase it.

@alanrenmsft
Copy link
Contributor Author

@misolori I have updated the screenshots in the PR description.

Copy link
Contributor

@miguelsolorio miguelsolorio left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for the update!

CleanShot 2021-07-08 at 17 06 23@2x

CleanShot 2021-07-08 at 17 06 38@2x

@alanrenmsft
Copy link
Contributor Author

@misolori I don't have merge permission, I am afraid I will need your help to merge the PR.

@alanrenmsft alanrenmsft changed the title make dropdown arrow bold to meet accessibility requirement make dropdown arrow larger to meet accessibility requirement Jul 9, 2021
@miguelsolorio
Copy link
Contributor

miguelsolorio commented Jul 9, 2021

Yes you do, I'll wait for @isidorn to do one last review before we merge since it also affects the debug drop down.

@miguelsolorio
Copy link
Contributor

@isidorn is now out on vacation so I'll just go ahead and merge this.

@miguelsolorio miguelsolorio merged commit 4cf81a2 into microsoft:main Jul 12, 2021
@miguelsolorio miguelsolorio added icons-product Issues for in-product icons verification-needed Verification of issue is requested labels Jul 12, 2021
@miguelsolorio
Copy link
Contributor

miguelsolorio commented Jul 12, 2021

For verification: use a color contrast tool and ensure the chevron (debug or output dropdown) meets the color contrast ratio of 3:1 or above

@alanrenmsft
Copy link
Contributor Author

@misolori thank you

@isidorn
Copy link
Contributor

isidorn commented Jul 26, 2021

@misolori thanks for merging 👏
@alanrenmsft thanks for the PR 👏

@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues icons-product Issues for in-product icons verification-needed Verification of issue is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants