Button Block: No color contrast warning, regardless of chosen colors#71272
Button Block: No color contrast warning, regardless of chosen colors#71272shrivastavanolo wants to merge 2 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| // Special handling for button blocks - use the button link element for colors | ||
| let targetElement = blockEl; | ||
| if ( blockEl.classList.contains( 'wp-block-button' ) ) { | ||
| const buttonLink = blockEl.querySelector( '.wp-block-button__link' ); | ||
| if ( buttonLink ) { | ||
| targetElement = buttonLink; | ||
| } | ||
| } |
There was a problem hiding this comment.
The similar exceptional cases and hardcoded block classes don't scale. Why?
- What happens if the block classes or markup changes?
- What if other blocks also use a different target for applying colors? Do we try to hardcode all possible cases?
How I would handle a similar case: See how the block editor decides where to apply the colors. Check if there are already existing helpers we can reuse. Try to reuse existing methods and declarative settings for the contrast checker.
P.S. IIRC, special cases like this will use the selectors definition to target the correct elements.
There was a problem hiding this comment.
Hey @Mamaduka Thanks for the review! I updated this based on your comment, now using the block’s selectors.root definition instead of hardcoding classes, with a generic fallback to blockEl. This way it scales if block markup changes or other blocks define different targets. Can you take a look and let me know if this approach works?
2c78e06 to
b4bc812
Compare
| const rootSelector = blockType?.selectors?.root || null; | ||
| let targetElement = blockEl; | ||
|
|
||
| if ( rootSelector ) { | ||
| const [ , ...rest ] = rootSelector.split( ' ' ); | ||
| if ( rest.length ) { | ||
| const innerSelector = rest.join( ' ' ); | ||
| targetElement = blockEl.querySelector( innerSelector ) || blockEl; | ||
| } | ||
| } |
There was a problem hiding this comment.
Have you tried using the getBlockCSSSelector method here? I think that should be the preferred method for getting block selectors.
The following code assumes that the first part of a selector will always be the block element, followed by child element selectors. I wonder if that will always be true, since it's a bit unclear in the docs. https://developer.wordpress.org/block-editor/reference-guides/block-api/block-selectors/.
@aaronrobertshaw, do you have any suggestions on how to handle this case best?
There was a problem hiding this comment.
Thanks for the ping 👍
Have you tried using the getBlockCSSSelector method here? I think that should be the preferred method for getting block selectors.
In terms of retrieving selectors using getBlockCSSSelector would be the best method as it will handle fallbacks when the block hasn't migrated to using the Block Selectors API (e.g. still uses __experimentalSelector). It would also allow for fallbacks when the color.background|text|link selectors having been defined.
do you have any suggestions on how to handle this case best?
I don't have much in the way of solutions to propose at this stage, I'd need time to dig deeper here and unfortunately, I won't have that bandwidth for a month or two at least.
There are a couple of critical issues with the current approach though that I can confirm or highlight which might then help shape possible solutions down the road.
The following code assumes that the first part of a selector will always be the block element, followed by child element selectors. I wonder if that will always be true, since it's a bit unclear in the docs.
This is is definitely a critical bug with the current approach. There is no requirement that the first segment of a selector is the block wrapper's class. This was deliberately the case so that the selectors defined by a block can have full freedom.
So at first glance, I see the following issues:
- Root block class
- No requirement for the block class to be first in the selector
- CSS combinators
- Current splitting approach fails to take into account CSS combinators resulting in malformed selectors
- This approach will break existing blocks such as Table which uses
.wp-block-table > table
- Comma separated selectors
- These selectors probably need handling as they tend to represent use cases where elements may or may not exist depending on block configuration or state
- Color feature selectors
- The current approach assumes all color styles target the root block wrapper
- Color styles can be applied via
selectors.color.background|text|linketc
Crafting a generic solution that works for all blocks will not be a trivial task. Perhaps an interim solution or compromise that covers more use cases can be achieved. Either way, this PR will need some further work before it can be considered for merging.
b4bc812 to
d71a32f
Compare
What?
Closes #39226
The contrast warning is not appearing for Button blocks in WordPress Gutenberg, despite choosing colors with insufficient contrast, due to the color data being retrieved from the wrong node.
Why?
The issue needs to be fixed because the contrast warning is not appearing for Button blocks, even when the text and background colors have low contrast, due to the color data being retrieved from the wrong HTML element (
wp-block-buttoninstead ofwp-block-button__link). This means that users may unintentionally create buttons with inaccessible color combinations.How?
The changes modify the
getBlockElementColorsfunction incontrast-checker.jsto handle Button blocks specially.When a Button block is detected, the code now uses the
.wp-block-button__linkelement instead of the.wp-block-buttonelement to determine the text and background colors. This is because the colors are actually applied to the child div.wp-block-button__link, not the container element.wp-block-button.The changes include adding a conditional statement to check if the block element is a Button block, and if so, using the
.wp-block-button__linkelement to get the text and background colors. This should fix the issue with the contrast warning not appearing for Button blocks.Testing Instructions
Screenshots or screencast
Before
before.mov
After
after.mov