Repository navigation
ContrastChecker: Fix check for button block colors #71959
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
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
a73b5e2
Update contrast checker to get computed values from root selectors, i…
juanfra c01fc04
Use getBlockCSSSelector to resolve color targets (color.text/backgrou…
juanfra 6b807ae
Apply suggestions from code review.
juanfra c68f94e
Apply suggestions from code review.
juanfra 3cefc8d
Fix static checks
Mamaduka 149e3c2
Add mutation to watch for class or style attribute changes.
juanfra 3e54cbb
Add contrast checker tests.
juanfra a52ba09
Move the observer to its own `useLayoutEffect` with dependencies.
juanfra 7f9af9d
Inline callback
Mamaduka 2da8eda
Adjust some e2e tests
Mamaduka 881fc09
Remove double rAF for MutationObserver
Mamaduka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,9 @@ | |
| * WordPress dependencies | ||
| */ | ||
| import { useLayoutEffect, useReducer } from '@wordpress/element'; | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { store as blocksStore } from '@wordpress/blocks'; | ||
| import { getBlockSelector } from '@wordpress/global-styles-engine'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -15,19 +18,35 @@ function getComputedValue( node, property ) { | |
| .getPropertyValue( property ); | ||
| } | ||
|
|
||
| function getBlockElementColors( blockEl ) { | ||
| if ( ! blockEl ) { | ||
| function getBlockElementColors( blockEl, blockType ) { | ||
| if ( ! blockEl || ! blockType ) { | ||
| return {}; | ||
| } | ||
|
|
||
| const firstLinkElement = blockEl.querySelector( 'a' ); | ||
| const linkColor = !! firstLinkElement?.innerText | ||
| ? getComputedValue( firstLinkElement, 'color' ) | ||
| : undefined; | ||
| // Get color-specific selectors. | ||
| const textSelector = getBlockSelector( blockType, 'color.text', { | ||
| fallback: true, | ||
| } ); | ||
| const backgroundSelector = getBlockSelector( | ||
| blockType, | ||
| 'color.background', | ||
| { fallback: true } | ||
| ); | ||
|
|
||
| const textColor = getComputedValue( blockEl, 'color' ); | ||
| // Find target elements - querySelector handles all the complexity | ||
| const textElement = blockEl.querySelector( textSelector ) || blockEl; | ||
| const backgroundElement = | ||
| blockEl.querySelector( backgroundSelector ) || blockEl; | ||
| const linkElement = blockEl.querySelector( 'a' ); | ||
|
|
||
| let backgroundColorNode = blockEl; | ||
| // Get computed colors from the appropriate elements | ||
| const textColor = getComputedValue( textElement, 'color' ); | ||
| const linkColor = | ||
| linkElement && linkElement.textContent | ||
| ? getComputedValue( linkElement, 'color' ) | ||
| : undefined; | ||
|
|
||
| let backgroundColorNode = backgroundElement; | ||
| let backgroundColor = getComputedValue( | ||
| backgroundColorNode, | ||
| 'background-color' | ||
|
|
@@ -61,28 +80,56 @@ function reducer( prevColors, newColors ) { | |
| return hasChanged ? newColors : prevColors; | ||
| } | ||
|
|
||
| export default function BlockColorContrastChecker( { clientId } ) { | ||
| export default function BlockColorContrastChecker( { clientId, name } ) { | ||
| const blockEl = useBlockElement( clientId ); | ||
| const [ colors, setColors ] = useReducer( reducer, {} ); | ||
|
|
||
| const blockType = useSelect( | ||
| ( select ) => { | ||
| return name | ||
| ? select( blocksStore ).getBlockType( name ) | ||
| : undefined; | ||
| }, | ||
| [ name ] | ||
| ); | ||
|
|
||
| // There are so many things that can change the color of a block | ||
| // So we perform this check on every render. | ||
| useLayoutEffect( () => { | ||
| if ( ! blockEl ) { | ||
| if ( ! blockEl || ! blockType ) { | ||
| return; | ||
| } | ||
|
|
||
| function updateColors() { | ||
| setColors( getBlockElementColors( blockEl ) ); | ||
| } | ||
|
|
||
| // Combine `useLayoutEffect` and two rAF calls to ensure that values are read | ||
| // after the current paint but before the next paint. | ||
| window.requestAnimationFrame( () => | ||
| window.requestAnimationFrame( updateColors ) | ||
| window.requestAnimationFrame( () => | ||
| setColors( getBlockElementColors( blockEl, blockType ) ) | ||
| ) | ||
| ); | ||
| } ); | ||
|
|
||
| // Runs in its own effect with dependencies so the observer is only | ||
| // recreated when the block element or block type changes. | ||
| useLayoutEffect( () => { | ||
| if ( ! blockEl || ! blockType ) { | ||
| return; | ||
| } | ||
|
|
||
| const observer = new window.MutationObserver( () => { | ||
| setColors( getBlockElementColors( blockEl, blockType ) ); | ||
| } ); | ||
|
Comment on lines
+119
to
+121
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't need a double rAF hack; the mutated attributes should already be in the DOM. |
||
|
|
||
| observer.observe( blockEl, { | ||
| attributes: true, | ||
| attributeFilter: [ 'class', 'style' ], | ||
| } ); | ||
|
|
||
| return () => { | ||
| observer.disconnect(); | ||
| }; | ||
| }, [ blockEl, blockType ] ); | ||
|
|
||
| return ( | ||
| <ContrastChecker | ||
| backgroundColor={ colors.backgroundColor } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We just want to check if the link has contents, not a big perf improvement, but
textContentdoes trigger reflow.