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.

Sign up for GitHub

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: Fix block color contrast checker #68799

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Block Editor: Fix block color contrast checker #68799

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b925ac6
Block Editor: Fix block color contrast checker
Mamaduka Jan 18, 2025
49286cf
Use reducer hook
Mamaduka Jan 18, 2025
3662d7c
Add comment
Mamaduka Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 64 additions & 30 deletions packages/block-editor/src/hooks/contrast-checker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,60 +1,94 @@
/**
* WordPress dependencies
*/
import { useState, useEffect } from '@wordpress/element';
import { useLayoutEffect, useReducer } from '@wordpress/element';

/**
* Internal dependencies
*/
import ContrastChecker from '../components/contrast-checker';
import { useBlockElement } from '../components/block-list/use-block-props/use-block-refs';

function getComputedStyle( node ) {
return node.ownerDocument.defaultView.getComputedStyle( node );
function getComputedValue( node, property ) {
return node.ownerDocument.defaultView
.getComputedStyle( node )
.getPropertyValue( property );
}

function getBlockElementColors( blockEl ) {
if ( ! blockEl ) {
return {};
}

const firstLinkElement = blockEl.querySelector( 'a' );
const linkColor = !! firstLinkElement?.innerText
? getComputedValue( firstLinkElement, 'color' )
: undefined;

const textColor = getComputedValue( blockEl, 'color' );

let backgroundColorNode = blockEl;
let backgroundColor = getComputedValue(
backgroundColorNode,
'background-color'
);
while (
backgroundColor === 'rgba(0, 0, 0, 0)' &&
backgroundColorNode.parentNode &&
backgroundColorNode.parentNode.nodeType ===
backgroundColorNode.parentNode.ELEMENT_NODE
) {
backgroundColorNode = backgroundColorNode.parentNode;
backgroundColor = getComputedValue(
backgroundColorNode,
'background-color'
);
}

return {
textColor,
backgroundColor,
linkColor,
};
}

function reducer( prevColors, newColors ) {
const hasChanged = Object.keys( newColors ).some(
( key ) => prevColors[ key ] !== newColors[ key ]
);

// Do not re-render if the colors have not changed.
return hasChanged ? newColors : prevColors;
}

export default function BlockColorContrastChecker( { clientId } ) {
const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState();
const [ detectedColor, setDetectedColor ] = useState();
const [ detectedLinkColor, setDetectedLinkColor ] = useState();
const blockEl = useBlockElement( clientId );
const [ colors, setColors ] = useReducer( reducer, {} );

// There are so many things that can change the color of a block
// So we perform this check on every render.
useEffect( () => {
useLayoutEffect( () => {
if ( ! blockEl ) {
return;
}
setDetectedColor( getComputedStyle( blockEl ).color );

const firstLinkElement = blockEl.querySelector( 'a' );
if ( firstLinkElement && !! firstLinkElement.innerText ) {
setDetectedLinkColor( getComputedStyle( firstLinkElement ).color );
}

let backgroundColorNode = blockEl;
let backgroundColor =
getComputedStyle( backgroundColorNode ).backgroundColor;
while (
backgroundColor === 'rgba(0, 0, 0, 0)' &&
backgroundColorNode.parentNode &&
backgroundColorNode.parentNode.nodeType ===
backgroundColorNode.parentNode.ELEMENT_NODE
) {
backgroundColorNode = backgroundColorNode.parentNode;
backgroundColor =
getComputedStyle( backgroundColorNode ).backgroundColor;
function updateColors() {
setColors( getBlockElementColors( blockEl ) );
}

setDetectedBackgroundColor( backgroundColor );
}, [ 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 )
);
Comment on lines +79 to +83
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual fix. The rest is just moving code around for readability.

Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that the actual fix is removing the [ blockEl ] dependency array from the effect. Then it runs on every render, not only on blockEl change (the block's DOM element hardly ever changes), and catches all the color updates.

Running after the current paint but before the next paint is exactly what useEffect does. It runs when the rendered DOM is already painted. You shouldn't need to simulate it with layout effect + rAF.

Copy link
Member Author

@Mamaduka Mamaduka Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jsnajdr, the component had an issue before adding blockEl as a dependency. It didn't always work, and the computed properties from blockEl were stale.

You shouldn't need to simulate it with layout effect + rAF.

I would like to avoid it as well, but this the only solution that works.

Quoting from: #59227.

React does not guarantee that useEffect will be called after the browser paints and, in fact, useEffect can be called before a paint when React is handling a user event (e.g. a click) or if a useLayoutEffect in the component tree updates state (e.g. calls setState). See facebook/react#20863 (comment).

This is probably a better summary for React 18 - facebook/react#24409 (comment).

} );

return (
<ContrastChecker
backgroundColor={ detectedBackgroundColor }
textColor={ detectedColor }
backgroundColor={ colors.backgroundColor }
textColor={ colors.textColor }
linkColor={ colors.linkColor }
enableAlphaChecker
linkColor={ detectedLinkColor }
/>
);
}
Loading