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

Use Colors: Add text color detection support. #18547

Merged
merged 4 commits into from
Dec 2, 2019
Merged

Use Colors: Add text color detection support. #18547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
465ce64
Use Colors: Add text color detection support.
epiqueras Nov 15, 2019
a537bc0
Use Colors: Support specific selectors for color detection.
epiqueras Nov 15, 2019
45c81d4
Use Colors: Pass down refs to contrast checker to avoid stale values.
epiqueras Nov 26, 2019
9ae645f
Heading: Detect inherited text color for contrast checking.
epiqueras Nov 26, 2019
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
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

130 changes: 87 additions & 43 deletions packages/block-editor/src/components/colors/use-colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import memoize from 'memize';
import classnames from 'classnames';
import { map, kebabCase, camelCase, startCase } from 'lodash';
import { map, kebabCase, camelCase, castArray, startCase } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -48,7 +48,8 @@ const ColorPanel = ( {
colorSettings,
colorPanelProps,
contrastCheckers,
detectedBackgroundColor,
detectedBackgroundColorRef,
detectedColorRef,
panelChildren,
} ) => (
<PanelColorSettings
Expand All @@ -63,9 +64,13 @@ const ColorPanel = ( {
backgroundColor = resolveContrastCheckerColor(
backgroundColor,
colorSettings,
detectedBackgroundColor
detectedBackgroundColorRef.current
);
textColor = resolveContrastCheckerColor(
textColor,
colorSettings,
detectedColorRef.current
);
textColor = resolveContrastCheckerColor( textColor, colorSettings );
return (
<ContrastChecker
key={ `${ backgroundColor }-${ textColor }` }
Expand All @@ -80,11 +85,12 @@ const ColorPanel = ( {
backgroundColor = resolveContrastCheckerColor(
backgroundColor || value,
colorSettings,
detectedBackgroundColor
detectedBackgroundColorRef.current
);
textColor = resolveContrastCheckerColor(
jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
textColor || value,
colorSettings
colorSettings,
detectedColorRef.current
);
return (
<ContrastChecker
Expand Down Expand Up @@ -192,37 +198,72 @@ export default function __experimentalUseColors(
);

const detectedBackgroundColorRef = useRef();
const BackgroundColorDetector = useMemo(
() =>
contrastCheckers &&
( Array.isArray( contrastCheckers ) ?
contrastCheckers.some(
( { backgroundColor } ) => backgroundColor === true
) :
contrastCheckers.backgroundColor === true ) &&
withFallbackStyles( ( node, { querySelector } ) => {
if ( querySelector ) {
node = node.parentNode.querySelector( querySelector );
}
let backgroundColor = getComputedStyle( node ).backgroundColor;
while ( backgroundColor === 'rgba(0, 0, 0, 0)' && node.parentNode ) {
node = node.parentNode;
backgroundColor = getComputedStyle( node ).backgroundColor;
const detectedColorRef = useRef();
const ColorDetector = useMemo( () => {
let needsBackgroundColor = false;
let needsColor = false;
for ( const { backgroundColor, textColor } of castArray( contrastCheckers ) ) {
if ( ! needsBackgroundColor ) {
needsBackgroundColor = backgroundColor === true;
}
if ( ! needsColor ) {
needsColor = textColor === true;
}
if ( needsBackgroundColor && needsColor ) {
break;
}
}
return (
( needsBackgroundColor || needsColor ) &&
withFallbackStyles(
Copy link
Member

@jorgefilipecosta jorgefilipecosta Nov 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We need to re-execute the color detection when a color changes. On the paragraph block, we want to automatically detect background and text color contrastCheckers: { backgroundColor: true, textColor: true, fontSize: fontSize.size },, we can also change both colors. The contrast warnings don't update when a color challenges but if we go to the code editor and switch back the visual editor to force the component to remount the warnings appear as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be working after your last rebase?

Copy link
Member

Choose a reason for hiding this comment

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

No, I think the warnings still don't appear when expected:
Nov-26-2019 09-04-09

We can see the warning appears when it should not, and disappears when it should appear. If I type something and the warning should appear, it appears after I type. Not sure why typing interferes with this, but we should make sure the color computation does not happen on each character typed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was happening because setting a ref does not re-render the component, so the contrast checker used a stale value until the component had any sort of update.

Fixed: 45c81d4.

(
node,
{
querySelector,
backgroundColorSelector = querySelector,
textColorSelector = querySelector,
}
) => {
let backgroundColorNode = node;
let textColorNode = node;
if ( backgroundColorSelector ) {
backgroundColorNode = node.parentNode.querySelector(
backgroundColorSelector
);
}
if ( textColorSelector ) {
textColorNode = node.parentNode.querySelector( textColorSelector );
}
let backgroundColor;
const color = getComputedStyle( textColorNode ).color;
if ( needsBackgroundColor ) {
backgroundColor = getComputedStyle( backgroundColorNode )
.backgroundColor;
while (
backgroundColor === 'rgba(0, 0, 0, 0)' &&
backgroundColorNode.parentNode
) {
backgroundColorNode = backgroundColorNode.parentNode;
backgroundColor = getComputedStyle( backgroundColorNode )
.backgroundColor;
}
}
detectedBackgroundColorRef.current = backgroundColor;
detectedColorRef.current = color;
return { backgroundColor, color };
}
detectedBackgroundColorRef.current = backgroundColor;
return { backgroundColor };
} )( () => <></> ),
[
colorConfigs.reduce(
( acc, colorConfig ) =>
`${ acc } | ${ attributes[ colorConfig.name ] } | ${
attributes[ camelCase( `custom ${ colorConfig.name }` ) ]
}`,
''
),
...deps,
]
);
)( () => <></> )
);
}, [
colorConfigs.reduce(
( acc, colorConfig ) =>
`${ acc } | ${ attributes[ colorConfig.name ] } | ${
attributes[ camelCase( `custom ${ colorConfig.name }` ) ]
}`,
''
),
...deps,
] );

return useMemo( () => {
const colorSettings = {};
Expand All @@ -249,9 +290,9 @@ export default function __experimentalUseColors(
const customColor = attributes[ camelCase( `custom ${ name }` ) ];
// We memoize the non-primitives to avoid unnecessary updates
// when they are used as props for other components.
const _color = ! customColor ?
colors.find( ( __color ) => __color.slug === color ) :
undefined;
const _color = customColor ?
undefined :
colors.find( ( __color ) => __color.slug === color );
acc[ componentName ] = createComponent(
name,
property,
Expand All @@ -261,7 +302,9 @@ export default function __experimentalUseColors(
customColor
);
acc[ componentName ].displayName = componentName;
acc[ componentName ].color = customColor ? customColor : ( _color && _color.color );
acc[ componentName ].color = customColor ?
customColor :
_color && _color.color;
acc[ componentName ].slug = color;
acc[ componentName ].setColor = createSetColor( name, colors );

Expand All @@ -287,7 +330,8 @@ export default function __experimentalUseColors(
colorSettings,
colorPanelProps,
contrastCheckers,
detectedBackgroundColor: detectedBackgroundColorRef.current,
detectedBackgroundColorRef,
detectedColorRef,
panelChildren,
};
return {
Expand All @@ -296,7 +340,7 @@ export default function __experimentalUseColors(
InspectorControlsColorPanel: (
<InspectorControlsColorPanel { ...wrappedColorPanelProps } />
),
BackgroundColorDetector,
ColorDetector,
};
}, [ attributes, setAttributes, detectedBackgroundColorRef.current, ...deps ] );
}, [ attributes, setAttributes, ...deps ] );
}
epiqueras marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 3 additions & 3 deletions packages/block-library/src/heading/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ function HeadingEdit( {
onReplace,
className,
} ) {
const { TextColor, InspectorControlsColorPanel, BackgroundColorDetector } = __experimentalUseColors(
const { TextColor, InspectorControlsColorPanel, ColorDetector } = __experimentalUseColors(
[ { name: 'textColor', property: 'color' } ],
{
contrastCheckers: { backgroundColor: true },
contrastCheckers: { backgroundColor: true, textColor: true },
},
[]
);
Expand All @@ -56,7 +56,7 @@ function HeadingEdit( {
</InspectorControls>
{ InspectorControlsColorPanel }
<TextColor>
<BackgroundColorDetector querySelector='[contenteditable="true"]' />
<ColorDetector querySelector='[contenteditable="true"]' />
Copy link
Member

Choose a reason for hiding this comment

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

In the button block, the background and color elements are not the same. Would it make sense to have a colorSelector and backgroundSelector? To allow the detector to work when background and color use different elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was just about to ask you that 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

<RichText
identifier="content"
tagName={ tagName }
Expand Down