Add preference to switch between AA & AAA levels for color-contrast checks #41363
Conversation
|
Size Change: +199 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
| @@ -50,11 +52,15 @@ function ContrastChecker( { | |||
| description: __( 'link color' ), | |||
| }, | |||
| ]; | |||
| const strictContrastCheck = select( preferencesStore ).get( | |||
There was a problem hiding this comment.
I just wanted to note that preferences for core/edit-post will be only available in the edit post screen, but never in other cases.
In general, in the block editor, we should rather pass down such setting so it could be read as any other block editor setting. @talldan and @youknowriad, how is it handled in other places?
There was a problem hiding this comment.
Apologies for the late response, have been AFK.
I think it's worth thinking about whether we want one WordPress-wide setting for this (i.e. if I toggle it on in the post editor, does it also become active in other editors?). There aren't any other user visible preferences that do this at the moment, they're all editor specific. But color contrast does seem like a policy-level thing for a user where they wouldn't really want this on in one editor an off in another, so perhaps this is the first preference where we should think about making this happen. What do you think?
If so, we can use a shared name like 'core' instead of 'core/edit-post'.
If not, then using an editor setting is the historical way to do this. The editor settings are often extended on initialisation of an editor, and some preferences become editor settings. Here's an example for the post editor:
gutenberg/packages/edit-post/src/editor.js
Lines 109 to 156 in a265d13
The other editors are pretty much the same apart from the site editor, which does this in a selector. Doing it this way means the component can just read the editor setting without having to be concerned with which editor is active.
There was a problem hiding this comment.
The other issue you mentioned about needing to refresh might be because this is select and not useSelect.
There was a problem hiding this comment.
Thank you for the advice on using useSelect, that worked 👍
color contrast does seem like a policy-level thing for a user where they wouldn't really want this on in one editor an off in another
It seems to already work fine on my end... 🤔
To test this, I added in https://github.com/WordPress/gutenberg/pull/41363/files#diff-6bf1ecd7162a5f625e50d3c434dec2da49ee8143f738358c4c02f4c537af73afR75 console.log( isReadableOptions.level ).
In the post-editor, I changed the preference to AAA. Then checked the post-editor and the site-editor and both were using AAA as expected.
I then switched back to AA, and sure enough, both the site-editor and post-editor were using AA for the contrast-checks.
Am I missing something?
There was a problem hiding this comment.
It seems to already work fine on my end...
Ah, I hadn't tested the other editors.
As @gziolo says, 'core/edit-post' is for preferences that apply only to the post editor. If it's intended to be a cross-editor preference, then using a value like 'core' for the scope would be better - select( preferencesStore ).get( 'core', 'strictColorContrastChecks' ). The first param is like a namespace that the preferences will be grouped under.
I think a preference toggle should also be added to the site editor's preference modal too. A toggle in the widget editors may also be a good idea, as this preference also affects those editors. In the long run it probably makes more sense for a preference like this to be shown in the global WordPress settings menu, but this is something that hasn't been done before.
As this is the first cross-editor preference, I'm not sure if it needs some additional design treatment to show that toggling it in one place applies it in multiple editors. Some additional help text below the label might be an easy option.
There was a problem hiding this comment.
Hmmmm should we use 'core', or would something like 'core/global' be more suitable? 🤔
There was a problem hiding this comment.
I think just 'core' is fine.
|
This is such a valuable enhancement. Thank you for working on this ❤️ |
aristath
changed the title
| @@ -145,6 +145,13 @@ export default function EditPostPreferencesModal() { | |||
| label={ __( 'Display block breadcrumbs' ) } | |||
| /> | |||
| ) } | |||
| <EnableFeature | |||
There was a problem hiding this comment.
I mentioned in another issue that it'd make sense to introduce a new Accessibility panel to the settings modal instead of folding everything into appearance.
There was a problem hiding this comment.
Is this something that would block this PR? I believe that if at some point we introduce an a11y panel, then we can move that option there (if and when that happens)
There was a problem hiding this comment.
It shouldn't block this, just connecting the dots since it's been mentioned in a few PRs adding features but hasn't gotten much attention.
aristath
force-pushed
the
try/improve-contrast-checker
branch
from
e742589
to
fbf53f1
Compare
aristath
force-pushed
the
try/improve-contrast-checker
branch
from
ac05de1
to
6fbfb09
Compare
priethor
added
[Focus] Accessibility (a11y)
|
@aristath what's left on this one to get it through? |
Nothing as far as I can remember... just a rebase and a review. I'll rebase it and then we can test it again 'cause a lot has changed in Gutenberg since this PR was created |
aristath
force-pushed
the
try/improve-contrast-checker
branch
from
6fbfb09
to
b4b5538
Compare
|
Flaky tests detected in b4b5538. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7057226447
|
|
Rebased the PR, checked that it still works, and updated the |
|
Hey @aristath 👋 With this setting getting stored in the users preferences, do you think there is a way a site could enforce this setting to be applied globally? Regadlress this is a really valuable feature that shouldn't get held up by this request. But it would be really neat for a site owner to define once in code whether they want to enforce strict color checks globally so that it applies to all users :) |
I honestly have no idea... It can probably be done using a filter somewhere, but generally speaking, it would be useful to have something like a |
There was a problem hiding this comment.
This mostly still looks good, I left a couple of comments.
I agree with @fabiankaegy that conceptually this might be more of a site-wide policy. If users need to override it, it could still be a user preference, the site setting could be passed into the editor as part of the editor settings and act as the default value for the preference.
| const colorContrastCheckAAA = useSelect( ( select ) => { | ||
| const { isFeatureActive } = select( editPostStore ); | ||
| return isFeatureActive( 'strictColorContrastChecks' ); | ||
| }, [] ); |
There was a problem hiding this comment.
This can be selected from the preferences store instead of using isFeatureActive (which I think is deprecated, or should be deprecated).
| const { strictContrastCheck } = useSelect( | ||
| ( select ) => ( { | ||
| strictContrastCheck: select( preferencesStore ).get( | ||
| 'core/edit-post', |
There was a problem hiding this comment.
When preferences are saved they're grouped into objects like this:
{
'core': {
// preferences shared across editors (there aren't any at the moment)
},
'core/edit-post': {
// preferences that are only for the post editor
},
'core/edit-site': {
// preferences that are only for the site editor
}
}So you'd need to decide what the right value here should be. A problem is that this is in the 'block-editor' package which is used across both the post and site editor, so having 'core/edit-post' hardcoded doesn't seem like the right option.
I think it would be good to have the strictContrastCheck passed in as a prop to the component rather than have the preference built-in to solve that. You'd then have more control over whether to use 'core', 'core/edit-post' or 'core/edit-site' as the place to save the preference value.
What?
This PR adds a new
strictColorContrastCheckspreference. By default it is disabled so we maintain the current behavior and there are no backwards-compatibility concerns, but the user can - if they choose to do so - enable stricter color-contrast checks, which will switch the level fromAAtoAAA.Why?
In some countries and industries, it is required by law to comply with WCAG:AAA. Our current contrast-checks have a hardcoded level of
AAfor the color checks. Allowing users to select a stricter level of compliance is a courtesy that can make life easier for many.How?
strictColorContrastCheckspreferencelevelinisReadableOptionsfromAAtoAAAif the feature is enabledbrightness()toluminance(). That is more correct for our scenario, since we're looking for the relative brightness - which is what also gets used in theisReadable()call internally.Testing Instructions
A good color combination to check is
#000000for the background, and#547948for the text-color. That combo passes AA but fails AAA.Notes
For some reason when I change the preference, I need to reload the page for it to take effect. I don't know if I missed something in this PR or if it's a more generic issue, feedback would be appreciated 🙇
Screenshots or screencast
Related to: #31223, #37448 (comment)