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

Improve accessibility #36

Merged
merged 8 commits into from
Apr 19, 2021
Merged

Improve accessibility #36

merged 8 commits into from
Apr 19, 2021

Conversation

esganzerla
Copy link
Contributor

@esganzerla esganzerla commented Jan 19, 2021

Hey folks,

I was reviewing the TodoMVC a11y features and I saw you improved colors already, but there are some more improvements possible.

This MR is improving on:


I also wrote a couple of articles in Medium about what I did:

@sindresorhus
Copy link
Member

sindresorhus commented Jan 31, 2021

While it's good to improve contrast, this looks too ugly.

Before:

Screen Shot 2021-01-31 at 16 49 19

After:

Screen Shot 2021-01-31 at 16 49 25


To name a few things:

  • No green color on checked todo item.
  • The placeholder color was already ok. A placeholder is not meant to have a big contrast.
  • I'm ok with increasing contrast on the SVG's, but I think you did it a tad too much.
  • The border around "All" is too dark.
  • There should not be a focus ring on the input field. It's already clear it's focused based on seeing the caret.
  • The updated screenshot should have the same dimensions and DPI as the previous one.

@esganzerla
Copy link
Contributor Author

Hey @sindresorhus ,

Thanks for the feedback!

I believe the perception of how it looks is mostly a matter of opinion. Nonetheless, I would agree that it could be better.

I have a few counter points about your comments though.


No green color on checked todo item.

I didn't understand this point. Could you expand on it?


The placeholder color was already ok. A placeholder is not meant to have a big contrast.

Could you expand on that? As I understand, not having the big contrast means it's not accessible, as that's the "label" for the field as well.
According to the WebAim contrast checker that is not enough contrast.
Personally I would even go further, as that font is really thin, but I didn't want to change it too much.


I'm ok with increasing contrast on the SVG's, but I think you did it a tad too much.

That was based on WebAim contrast checker and the background used in that block. Do you have some source to check the color, or some extra information on why it shouldn't be that much?


The border around "All" is too dark.

How you suggest a better approach should be?

There should not be a focus ring on the input field. It's already clear it's focused based on seeing the caret.

I see what you mean, and that's an interesting comment. I believe browsers wouldn't have implemented it as a default behavior if the caret was enough though, there is not a lot of highlight to it. We need to also remember that accessibility is for those with low vision that might not see the caret, as it's quite subtle. Also some people might be using using screen magnifier that are not covering the whole screen and not find the focus.
Moreover:
w3c/wcag#1014
w3c/wcag#680


The updated screenshot should have the same dimensions and DPI as the previous one.

I can do that, for sure. Out of curiosity, give this is not being applied in a fixed layout and the image seems to have enough quality to see it clearly (at least for me), could you explain why that is important?

@sindresorhus
Copy link
Member

I didn't understand this point. Could you expand on it?

The checkmark should be green.

Could you expand on that? As I understand, not having the big contrast means it's not accessible, as that's the "label" for the field as well.

Accessibility is not binary. We can improve it without extreme contrast. A placeholder is not meant to be as readable as normal text. We can instead make the font slightly less thin. However, in the future, we could consider supporting the upcoming high-contrast media query.

How you suggest a better approach should be?

Slightly darker than the current.


Regarding the input field. We could maybe do a shadow. Something that looks more pleasing.


I can do that, for sure. Out of curiosity, give this is not being applied in a fixed layout and the image seems to have enough quality to see it clearly (at least for me), could you explain why that is important?

It looks blurry on high-DPI screens.

@esganzerla
Copy link
Contributor Author

Hey @sindresorhus,

Sorry for the delay in sending this changes, I had a few very busy weeks.

I'm pushing some update based on your comments. I hope you consider this better, but we can do some more adjustments if you find it necessary.

Before:
before

After:
after


The checkmark should be green. (on completed items)

I'm colorblind and I missed that 😅 . I don't think we need to fix that in terms of a11y though.

We can instead make the font slightly less thin. (placeholder text)

I made it 400 (before was 300), In my opinion it was better with 300 and slightly darker. Let me know if this is ok, or you prefer to revert back to the original.

Displaying difference of placeholder before and after suggestions

However, in the future, we could consider supporting the upcoming high-contrast media query.

I like that idea. Although my only concern left would be the placeholder, and just because it is currently serving as a label as well.

Slightly darker than the current. (border around the filters)

I reviewed the colors I implemented there and turns out I got it a little bit off tone. Finding the right tone I managed to get "base" a little bit ligther.

The thing is :hover has that base color and .selected was a bit darker. I compromised here setting :hover with that new "base" and made .selected have 1.5 contrast ratio from that, half of the recommended ratio for UI components.

What do you think?

Displaying contrast ration of selected and hover state for filter buttons

Regarding the input field. We could maybe do a shadow. Something that looks more pleasing.

That was a very nice suggestion. I think it looks a lot nicer now!

I also implemented one more change on the toggle-all focused state, to like a bit nicer.

Toggle-all button on focus state before
Toggle-all button on focus state after

It looks blurry on high-DPI screens.

Does this look ok now?

- Replace outline for box-shadow on focused elements.
- Revert input placeholder color and make font-weight 400 (before 300).
- Improve focus state on toggle-all label.
- Add green color on completeded items checkbox.
- Update colors on filter buttons.
- Update screenshot image.
@sindresorhus
Copy link
Member

  • The "check all" button is not vertically aligned with the individual checkboxes.
  • The "clear completed" button is not horizontally aligned with the others.
  • The checkbox buttons should be slightly lighter when not checked.
  • The focus state for the remove button is not centered around the button (I would also round the focus state to make it look better):

Screen Shot 2021-03-22 at 17 28 57

@esganzerla
Copy link
Contributor Author

Hello @sindresorhus,

I pushed some changes. I hope you are ok with them an we can merge this.

The "check all" button is not vertically aligned with the individual checkboxes.

Done

The "clear completed" button is not horizontally aligned with the others.

This was introduced between v2.2.0 and v2.3.0. Nevertheless, I fixed it.

The checkbox buttons should be slightly lighter when not checked.

I feel like anything I try to do here will be a simple guess trying to please you, as the accessibility guidelines indicate this color. I would suggest you change that to a color if you find more suitable.

The focus state for the remove button is not centered around the button

Done

(I would also round the focus state to make it look better)

Because it's in the parathesis I took that more as a suggestion than a requirement. I personally feel like that is inconsitent and doesn't match the whole structure of the app. Feel free to apply that change.

image

@sindresorhus sindresorhus changed the title Improve support to a11y Improve accessibility Apr 19, 2021
@sindresorhus sindresorhus merged commit 981f08a into tastejs:master Apr 19, 2021
@sindresorhus
Copy link
Member

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable outline on focusable elements.
2 participants