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

[BBT-105] Display notification when related background and foreground colours fail accessible colour contrast ratios #108

Merged
merged 7 commits into from
Sep 25, 2024

Conversation

thatisrich
Copy link
Contributor

@thatisrich thatisrich commented Sep 20, 2024

Description

BBT-105 - This PR adds a contrast checker to text and background colour pickers. When the selected colours do not have a sufficient contrast ratio, which may cause accessibility issues, a notification is displayed to inform the user.

The ContrastChecker component has been positioned inbetween the item title and palette selectors so it does not interfere with the grid layout of those components (when placed inside the columns container, the layout would shift pushing the columns out of place which could be confusing). I did experiment with including the ContrastChecker output at both the top and bottom of the container however it felt excessive and could also be confusing.

NOTE - This change only considers the text and background colours, it does not take into consideration any selected gradient colours at this time.

Change Log

  • src/editor/components/StylesColor.js - Imported, and used, the ContrastChecker to compare the text and background colours

Steps to test

  • Select a block or element that has the text and background colour options, such as Elements > Button
  • Select a text colour from the available palette
  • Select a contrasting colour for the background from the palette
  • Notice not warning is displayed
  • Select the same colour for the background that is assigned to text
  • Notice a contrast warning is now displayed

Screenshots/Videos

Demo showing the colour contrast notification when selecting colours

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@thatisrich thatisrich marked this pull request as ready for review September 20, 2024 14:56
@thatisrich thatisrich requested a review from g-elwell September 20, 2024 14:56
@jonnywatersbb
Copy link
Member

jonnywatersbb commented Sep 20, 2024

Hey @thatisrich, thanks for working on this - I think it's a really great addition.

Just having a quick peak and I noticed the position of the notice is causing some issues when using the colour picker, I assume because it's constantly checking if the colour is/isn't meeting the required ratio:

http://bigbite.im/v/XlO27T

That might only be solved by moving it completely away from the selector UI?

I wonder as well (maybe as a new issue) if it's a good idea to show the exact contrast ratio? Similar to something like this in Figma:

Screenshot 2024-09-20 at 15 58 51

It might be hard to understand as a user what the plugin is testing against unless it's shown explicitly (AA or AAA).

@jonnywatersbb jonnywatersbb self-requested a review September 20, 2024 14:59
@thatisrich thatisrich marked this pull request as draft September 20, 2024 15:03
@thatisrich
Copy link
Contributor Author

Ah! That's a really good spot @jonnywatersbb, I've moved the PR back to draft while I work on the fix.

Regarding the display of the pass level, the ContrastChecker is a WordPress component and is fairly self contained. The readme for the component says:

ContrastChecker component determines if contrast for text styles is sufficient (WCAG 2.0 AA) when used with a given background color.

We could pivot from the WordPress component and roll our own solution for this, which would give us more control over the output, if you would prefer the Figma approach?

@jonnywatersbb
Copy link
Member

ah ok! I think for simplicity we should roll with that initially, but perhaps in the notification state that the colour contrast should meet at minimum 4.5:1 to meet WCAG 2.0 AA?

We could always expand on the contrast tools in future as separate issue

@thatisrich
Copy link
Contributor Author

I'll certainly take a look into the feasibility of customising the ContrastChecker message, as the content is defined within the component itself and not available as a prop.

@thatisrich
Copy link
Contributor Author

thatisrich commented Sep 20, 2024

A change has been made to the ContrastChecker display approach, now the colours are passed in only when the mouse buttons are not pressed down.

This allows the user to select a custom colour within a colour picker and avoids a scenario in which the colour picker could visually jump up / down when the contrast notification display was toggled.

A demo selecting a custom colour can be seen here

NOTE - When using the keyboard to select a colour in the colour picker, the jumping issue is not present in the same way and therefore has not been affected by this change.

@thatisrich
Copy link
Contributor Author

I've tried to find a way to edit the message in the ContrastChecker, @jonnywatersbb, but I'm struggling to find a solution that retains the simplicity of the existing components.

The closest approach I have at the moment would be to include the additional content as a supporting component and wrap that, and the ContrastChecker, in a conditional which determines if the ContrastChecker is actually visible or not.

I'll have another think but I wonder if tackling the more in depth information in a custom tool / component would be beneficial to cover this off?

@thatisrich
Copy link
Contributor Author

Just to give an idea of what a custom component could look like for this, I've recorded a quick demo here.

This approach could display a pass / fail for AA and AAA compliance, a tailored message to that compliance, and the contrast ratio. Would this be a preferred approach over the WordPress core component or should we stick with core (and the reduced user feedback message) for this PR?

Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @thatisrich !

I love the custom UI you've prototyped, but my preference would be to stick with the core component for the time being. That said, if it's not too complex I could be swayed the other way. I think we should at least split that idea into a new PR so this one can proceed whilst we decide what to do.

src/editor/components/StylesColor.js Outdated Show resolved Hide resolved
src/editor/components/StylesColor.js Outdated Show resolved Hide resolved
src/editor/components/StylesColor.js Outdated Show resolved Hide resolved
@thatisrich thatisrich requested a review from g-elwell September 25, 2024 10:58
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

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

Works great for me, thanks for your efforts on the @thatisrich !

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.

Display notification when related background and foreground colours fail accessible colour contrast ratios
3 participants