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

Block Editor: Consider removing the contrast checker when block has only links #37549

Closed
gziolo opened this issue Dec 21, 2021 · 3 comments · Fixed by #45639
Closed

Block Editor: Consider removing the contrast checker when block has only links #37549

gziolo opened this issue Dec 21, 2021 · 3 comments · Fixed by #45639
Assignees
Labels
[Feature] Colors Color management [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@gziolo
Copy link
Member

gziolo commented Dec 21, 2021

Description

Discovered this while testing the Comment Reply Link and Comment Edit Link blocks. Both blocks don't contain any regular text because everything is wrapped with a link. It also means that block supports have color controls enabled only for the background and the link. Initially reported in #35774 (comment)

Background color + link color trigger contrast checker warning when they shouldn't:

Screen Shot 2021-10-26 at 16 58 49

I think the checker compares the background color selected and the default text color read from the same div, but we don't care about it at all.

Step-by-step reproduction instructions

  1. Insert the Comments Query Loop block into a template or post/page.
  2. Start editing the Comment Reply Link or the Comment Edit Link block.
  3. Open the settings sidebar and go to the colors panel.
  4. Set the background color to black and the link color to white which is the best possible contrast configuration 😄
  5. The contrast checker will show the warning that the background color and the text color (I assume it's set to black in the theme) needs to be updated to increase the contrast.

Screenshots, screen recording, code snippet

The up to date version looks this way:

Screenshot 2021-12-21 at 09 47 32

Environment info

  • WordPress 5.9 beta 3
  • Gutenberg 12.2 (actually the latest version from trunk)

The browser and OS don't matter here.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor [Feature] Colors Color management labels Dec 21, 2021
@gziolo
Copy link
Member Author

gziolo commented Dec 21, 2021

@jorgefilipecosta, I would be curious to hear your feedback what would be the ideal solution for this edge case? As far as I can see in the codebase, there is no way to use block supports to disable the contrast checker.

I played a bit with a very naive approach where the contrast checker would be disabled when there is no block support enabled for the text color in this place:

enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' && ! gradient && ! style?.color?.gradient
}

So the check would ensure that is only enabled when :

Platform.OS === 'web' && ! gradient && ! style?.color?.gradient && hasTextColor && hasBackgroundColor

The question is whether we want to improve the experience and offer a contrast checker also for links vs background but in that case we would have to convert this issue into an enhancement.

@ivan-ottinger
Copy link
Contributor

One observation related to this issue. In some circumstances, the contrast checker displays also when there's only a background color available on the block:

Markup on 2022-02-10 at 16:05:05

This is a block that displays images in its content only - so it would be better if the checker wasn't displayed in this case.

@ramonjd
Copy link
Member

ramonjd commented Feb 14, 2022

Similar issue over at #38428 (comment) but related to the Separator Block.

Excerpts from that discussion:

The Color Panel's implementation of the ContrastChecker is working as (originally) intended in that it attempts to compare the detected background color with any detected text color, irrespective of whether the selected block opts into color.text or not.

So it's not about whether the component or the selected block has a color, background or otherwise, but whether the selected block's color choices are readable when checked against any detected text color or detected background color.

In the case of the Separator block, even though it technically does not define a color or has any text, a color is nevertheless inherited from the has-background-color classname, and, if we remove that, from the body.

The Separator Block isn't really meant to house text, but the contrast checker detects an inherited color and tries anyway.

I think it would be helpful to completely disable (or enable) the contrast checker for specific blocks, possibly through block.json.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants