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

feat(rule): add new color-contrast-enhanced rule (WCAG AAA) #3235

Merged
merged 22 commits into from
Nov 29, 2021

Conversation

dan-tripp
Copy link
Contributor

feat(rule): add new color-contrast-enhanced rule (WCAG AAA)

Same code as color-contrast rule, but with different default thresholds.

Closes issue: #3189

@dan-tripp dan-tripp requested a review from a team as a code owner October 22, 2021 00:58
@dan-tripp
Copy link
Contributor Author

Hold on. I think I accidentally put code from another PR of mine into this PR. I'll try to fix that.

@straker straker changed the title Issue3189 feat(rule): add new color-contrast-enhanced rule (WCAG AAA) Oct 22, 2021
@straker
Copy link
Contributor

straker commented Oct 22, 2021

Thanks for updating all the locales as well! My only concern is that this will run by default, which I don't think we want. @WilcoFiers how should we handle that?

@dan-tripp
Copy link
Contributor Author

When Wilco wrote "This rule will have to be off by default" I figured I should put "enabled": false in color-contrast-enhanced.json, so that's what I did. Suggestions are so welcome, words fail me!

@straker
Copy link
Contributor

straker commented Oct 25, 2021

Ha, you are correct! I completely missed that part. I'll look this over more thoroughly later today.

@WilcoFiers
Copy link
Contributor

I think disabling it is the right thing to do here.

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Looked this over, looks good! Thanks again for taking care of all the locale strings and even updating their text with WCAG 2 AAA.

@WilcoFiers I have reviewed the pr and it all looks good. I made sure that all the locale strings were exact copies of their color-contrast equivalent so that should all be good. I'll wait for your approval before merging.

doc/rule-descriptions.md Outdated Show resolved Hide resolved
test/integration/full/contrast/sticky-header.js Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@ describe('color-contrast code highlighting test', function() {
function run(done) {
axe.run(
'#fixture',
{ runOnly: { type: 'rule', values: ['color-contrast'] } },
{ runOnly: { type: 'rule', values: ['color-contrast-enhanced', 'color-contrast'] } },
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, I prefer we give color-contrast-enhanced its own set of integration tests, focused specifically on showing the higher contrast values are properly applied. This PR has no test that shows that a test with 6:1 fails this rule for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I can add those tests. In the next few days, I hope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been otherwise occupied for the last few weeks but I hope to address this soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is something like this what you have in mind? My latest commit, ./test/integration/full/contrast-enhanced/simple.html

@straker
Copy link
Contributor

straker commented Nov 8, 2021

@dan-tripp do you need any help adding separate tests for AAA color-contrast?

@dan-tripp
Copy link
Contributor Author

I have some ideas but haven't implemented them yet because life got in the way. Soon I hope to add them to this PR.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Great work! Thank you @dan-tripp Much appreciated.

And for the books, I reviewed this PR for security.

@WilcoFiers WilcoFiers merged commit bec20fc into dequelabs:develop Nov 29, 2021
@dan-tripp
Copy link
Contributor Author

Nice! Thank you.

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.

3 participants