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

Accordion Example: Correct color contrast (Issue #1132) #1200

Merged
merged 5 commits into from
Oct 25, 2019

Conversation

ZoeBijl
Copy link
Contributor

@ZoeBijl ZoeBijl commented Oct 7, 2019

Resolves a part of #1132

Copy link
Contributor

@smhigley smhigley 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! There are just two places I found contrast (or high contrast mode) issues:

  • the .Accordion.focus border color (line 15 of accordion.css) is only 2.28:1
  • no focus outlines are visible in Windows High Contrast Mode (there's an outline:none .Accordion-trigger style in there, and it could be fixed by switching it to .Accordion-trigger:focus { outline: 4px solid transparent; }).

I also don't see a focus outline on the form fields in Firefox on Windows, so I'm guessing something in the styles is obscuring the native faint dashed outline. Might be beyond the scope of this PR though, depending on how much you feel like changing :)

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Oct 7, 2019

Thanks for the comment and feedback! I’ll change the two (high) contrast things you mentioned.

I kinda want to redo the example at some point. So I’ll leave the Firefox specific thing for another time/person.

@ZoeBijl ZoeBijl added this to the 1.2 Release 1 milestone Oct 8, 2019
Copy link
Contributor

@sh0ji sh0ji 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 great!

Since this is the first time I'm really digging into this issue, I am a bit uncertain about why the button border contrast was a violation (and by extension, the overall accordion's border). My understanding of 1.4.11 is that borders do not need to be 3:1 if some other indicator meets the appropriate requirement, which is what 1.4.11 Understanding says about buttons. I would assume that applies here as well since the primary controls are buttons. In other words, the text contrast should be sufficient to cover 1.4.11: Non-text Contrast because the button text's contrast ratio is 16.1:1.

On that same note, don't the input borders actually fail 1.4.11? They should be 3:1, but they're currently 2.68:1.

Let me know if I'm terribly misunderstanding the requirement here!

@smhigley
Copy link
Contributor

smhigley commented Oct 9, 2019

@sh0ji I think you might be reading the text of the requirement correctly, though in this case it seems like a grey area because the divide between the button and the accordion content could be meaningful.

Even if the button borders aren't a technical 1.4.11 failure, it still seems like better practice to meet at least 3:1. My beyond-AA rule of thumb is to check how usable something is if the borders were removed entirely. In this case it doesn't seem all that wonderful a user experience, so as long as color contrast is being updated, might as well update it for everything.

+1 to the input borders getting better contrast too :)

@sh0ji
Copy link
Contributor

sh0ji commented Oct 10, 2019

Great point, @smhigley. I don't have any qualms with going above and beyond AA of course (seems like there should be a AAA for this) and am not recommending we change this back. I just want to make sure I understand the requirement well so that we know when to address this type of thing in the future.

I had a similar thought about the button borders in the context of the accordion, but the fact that the details/summary elements don't have a border in any browser made me question that. I do think it's good UX to include a visual indicator of the boundaries of clickable regions when it extends beyond the text box. There's a WCAG SC for target size, but not one for target perceivability, so that doesn't seem like an accessibility requirement.

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Oct 17, 2019

Fixed the input border colour in 3775a34.

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Oct 25, 2019

@smhigley and @sh0ji can I get your approval (or request for changes) on this one?

@ZoeBijl ZoeBijl merged commit e80972a into master Oct 25, 2019
@ZoeBijl ZoeBijl deleted the issue1132-colour-contrast branch October 25, 2019 19:09
michael-n-cooper pushed a commit that referenced this pull request Oct 25, 2019
Accordion Example: Correct color contrast (pull #1200)

* Accordion Example: Correct color contrast (Issue #1132)

* Accordion Example: Correct (high) contrast issue (Issue #1132)

* Accordion Example: correct input border contrast (issue #1132)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants