-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Added contrast verification and readability checking to button. #3131
Added contrast verification and readability checking to button. #3131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3131 +/- ##
==========================================
- Coverage 34.01% 33.86% -0.16%
==========================================
Files 252 253 +1
Lines 6703 6736 +33
Branches 1217 1223 +6
==========================================
+ Hits 2280 2281 +1
- Misses 3730 3756 +26
- Partials 693 699 +6
Continue to review full report at Codecov.
|
Love where this is going. Helping people make content more accessible is awesome. Should a state change or event of some sort when the contrast passing state changes? Thinking of the use case of a plugin that wants to block publishing of non-accessible content. I would love to see some tests of the new code, even though they would essentially be wrappers for testing Also cc'ing @afercia so he sees this PR. |
Nice work! 1. Design-wise, I found it odd that ContrastChecker in the Button block should be rendered inside a PanelBody. I assume this is so that, visually, the notice appears between the two palettes. However, hierarchically, I'm not sure it's the best. I would simply place it as the next sibling of the 2. One rather glaring limitation I found with the Button block is that, if one is using a default color, ContrastChecker can't help the user. Here is an example with a default background color and a custom text color which, together, are clearly not readable: The default color is provided by One hack that I could conceive to work around this would require post- |
Ignoring how technically broken(*) my commit is 😁 , one thing it highlights is that the default color pairing ( (*): If the block had colors already set and the user then reverts them to default, it takes two renders until the contrast notice appears. This is because fallback colors aren't kept in const virtual = this.node.cloneNode();
virtual.style.backgroundColor = ''
virtual.style.display = 'none';
this.node.parentElement.appendChild( virtual );
getComputedStyle( virtual ).backgroundColor // <- grab it
virtual.parentElement.removeChild( virtual ); |
Love where this is going. I also like that there's a simple and clear warning message showing up, rather than a for-on-developers-obtuse contrast ratio. It's a win for the regular user. Note that it seemed to take a while for the message to show up for me — it didn't seem to show up for me until I started tweaking the text color (background alone didn't make it show up).
Here's a sneak peek at a button redesign ;) |
That's my own fault, @jasmussen. :) If you
\o/ |
17f89f6
to
6427758
Compare
Hi @aaronjorbin thank you for your sharing your thoughts. Regarding blocking, although that is not yet addressed in this PR, an idea I have in mind is adding to the contrast component an onFail property. Then parent components or other artifacts using their extensibility API (when ready) can make actions when contrast is failing like not allowing publishing. Hi @mcsf thank you for your commits. I like the idea of using the computed styles, the only with this approach is that the color used on the editor may not be the final ones. Some themes change them. One option is adding the CSS of the theme and trying to read the final colors from there (may not be easy). The other option is using different messages for this case something like "Unless your theme uses different colors from the ones being used in the editor...". And the third option is keep things as they are, our message says "may be". Hi @jasmussen, regarding the refresh problems I added a fix suggested by @mcs, now fallback colors are in the component state and the refresh problems should not happen. The last improvements are available to test in the button component, |
blocks/library/button/index.js
Outdated
grabColors() { | ||
const { background, text } = this.containers; | ||
const { textColor, color } = this.props.attributes; | ||
const { fallbackTextColorInState, fallbackBackgroundColorInState } = this.state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When are these ever set? :)
We can make do with just the truthiness of this.state.fallback*ColorInState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes my logic had a bug, but the results looked like the expected in tests so I did not notice, it was solved :)
27d4072
to
afa200f
Compare
afa200f
to
ed56fd8
Compare
Hi @mcsf, this was rebased and retested :) Could you have a new look and share your thoughts if this is ready or some change is needed? |
554ebb8
to
a992853
Compare
From a UI and functionality pov, this is 👍 👍 to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for working on this!
blocks/contrast-checker/index.js
Outdated
msg = __( 'This color combination may be hard for people to read. Try using a darker background color and/or a brighter text color.' ); | ||
} else { | ||
msg = __( 'This color combination may be hard for people to read. Try using a brighter background color and/or a darker text color.' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but this is a good place for a multiline ternary const assignment.
a992853
to
3a83be4
Compare
… button. This verification is implemented thanks to tinycolor2 lib. React-color (used in our color palette) already depends on it, this lib has no external dependencies. A new component was created that uses tinycolor2 to verify readability and then shows the correct warning message.
As an intermediate step before the next commit
This change should solve some refresh problems on contrast verification.
3a83be4
to
db42881
Compare
This PR aims to address part of #2381, by adding a contrast checking following WCAG 2.0- AA (https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.html) to the button.
Testing
Test some colors in button and verify that when contrast is high no message, in low contrast color pairs a message appear.
Verify that when background is darker than text, we suggest making background darker and/or text lighter and when text is darker we suggest making text darker and/or background lighter
Screenshots (jpeg or gifs if applicable):
Notes
This verification is implemented thanks to tinycolor2 lib. React-color (used in our color palette) already depends on it. This lib has no external dependencies. A new component was created that uses tinycolor2 to verify readability and then shows the correct warning message.