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

ColorPalette: Ensure text label contrast checking works with CSS variables #47373

Merged
merged 16 commits into from
Feb 7, 2023
Merged

ColorPalette: Ensure text label contrast checking works with CSS variables #47373

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0c5c13b
ColorPalette: Ensure text label contrast checking works with CSS vari…
t-hamano Jan 24, 2023
c4cd604
use `useEffect` to get normalized color value
t-hamano Jan 24, 2023
ebae2d7
Update changelog
t-hamano Jan 24, 2023
9327bc9
Try to detect actual color from rgba
t-hamano Jan 26, 2023
2eec7a6
Resolve conflict
t-hamano Jan 28, 2023
98bf1dc
Use function to get the composite background color
t-hamano Jan 28, 2023
75f9c62
Rewrite useEffect with useCallback
t-hamano Jan 29, 2023
0489602
Merge branch 'trunk' into fix/colorpalette-contrast-check
t-hamano Feb 4, 2023
30ae6bc
Don't consider transparent color
t-hamano Feb 4, 2023
011e099
Don't use ref, simplify normalizeColorValue() function
t-hamano Feb 4, 2023
dc62b0f
Add JSDoc
t-hamano Feb 4, 2023
2b845f7
Add unit tests
t-hamano Feb 4, 2023
b020ea2
Fix conflicts
t-hamano Feb 6, 2023
e225bf5
Refactor unit tests
t-hamano Feb 6, 2023
2945982
Refactor unit test
t-hamano Feb 7, 2023
969bf03
Merge branch 'trunk' into fix/colorpalette-contrast-check
t-hamano Feb 7, 2023
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
1 change: 1 addition & 0 deletions packages/components/src/color-palette/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ function UnforwardedColorPalette(
...otherProps
} = props;
const [ normalizedColorValue, setNormalizedColorValue ] = useState( value );

const clearColor = useCallback( () => onChange( undefined ), [ onChange ] );

const customColorPaletteCallbackRef = useCallback(
Expand Down
58 changes: 0 additions & 58 deletions packages/components/src/color-palette/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,6 @@ const EXAMPLE_COLORS = [
];
const INITIAL_COLOR = EXAMPLE_COLORS[ 0 ].color;

const CSS_VARIABLE_COLORS = [
{ name: 'red', color: 'var(--red)' },
{ name: 'blue', color: 'var(--blue)' },
];

const CSS_VARS_STYLE_PROPS = {
'--red': '#f00',
'--blue': '#00f',
} as React.CSSProperties;

function getWrappingPopoverElement( element: HTMLElement ) {
return element.closest( '.components-popover' );
}
Expand Down Expand Up @@ -211,52 +201,4 @@ describe( 'ColorPalette', () => {
screen.getByRole( 'button', { name: 'Clear' } )
).toBeInTheDocument();
} );

it( 'should show the readable text label even when the background color has a CSS variable.', async () => {
// const user = userEvent.setup();
const onChange = jest.fn();

render(
<div style={ CSS_VARS_STYLE_PROPS }>
<ColorPalette
colors={ CSS_VARIABLE_COLORS }
value={ CSS_VARIABLE_COLORS[ 0 ].color }
onChange={ onChange }
/>
</div>
);

const dropdownButton = screen.getByRole( 'button', {
name: /^Custom color picker/,
expanded: false,
} );

const dropdownButtonStyles = window.getComputedStyle( dropdownButton );
expect( dropdownButtonStyles.color ).toBe( 'rgb(0, 0, 0)' );
} );

it( 'should render dropdown with Hex value transformed from CSS a variable to actual color', async () => {
const user = userEvent.setup();
const onChange = jest.fn();

render(
<div style={ CSS_VARS_STYLE_PROPS }>
<ColorPalette
colors={ CSS_VARIABLE_COLORS }
value={ CSS_VARIABLE_COLORS[ 0 ].color }
onChange={ onChange }
/>
</div>
);

const dropdownButton = screen.getByRole( 'button', {
name: /^Custom color picker/,
expanded: false,
} );

await user.click( dropdownButton );

const dropdownColorInput = screen.getByLabelText( 'Hex color' );
expect( dropdownColorInput ).toHaveValue( 'FF0000' );
} );
} );
22 changes: 22 additions & 0 deletions packages/components/src/color-palette/test/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import {
extractColorNameFromCurrentValue,
showTransparentBackground,
normalizeColorValue,
} from '../utils';

describe( 'ColorPalette: Utils', () => {
Expand Down Expand Up @@ -38,4 +39,25 @@ describe( 'ColorPalette: Utils', () => {
expect( showTransparentBackground( '#f5f5f524' ) ).toBe( false ); // 0.14 alpha.
} );
} );

describe( 'normalizeColorValue', () => {
test( 'should return the value as is if the color value is not a CSS variable', () => {
const element = document.createElement( 'div' );
expect( normalizeColorValue( '#ff0000', element ) ).toBe(
'#ff0000'
);
} );
test( 'should return the background color computed from a element if the color value is a CSS variable', () => {
const element = document.createElement( 'div' );
const { ownerDocument } = element;
const { defaultView } = ownerDocument;
// @ts-ignore
defaultView.getComputedStyle = () => ( {
backgroundColor: '#ff0000',
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Not ideal, of course, but better than nothing. I'm not sure if @mirka has any idea to who we could get getComputedStyle to work our tests (afterall, it seems to work as expected in Theme's tests).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even @jsnajdr , given his recent work on getComputedStyle

Copy link
Member

Choose a reason for hiding this comment

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

JSDOM has a very limited support for CSS, and it doesn't expand CSS variables to their real values. That means that normalizeValue is not possible to unit test with Jest and JSDOM. I would consider if I want to keep the unit test at all.

A less harsh way to "fake" the CSS value is to set the style on the tested element itself:

element.style = { backgroundColor: '#ff0000' };

Then getComputedStyle( element ) will find this style and will return it.

By the way, the normalizeValue function would be better off if its second param was just the element, not a ref:

normalizeColorValue( value: string | undefined, element: HTMLElement | undefined )

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, Jarda!

A less harsh way to "fake" the CSS value is to set the style on the tested element itself:

Good point. @t-hamano , would you mind trying out this approach?

By the way, the normalizeValue function would be better off if its second param was just the element, not a ref:

Agreed! Although this change should have been applied in a previous commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice! Perhaps I was thinking too hard. I think we handled it well with 2945982.

expect( normalizeColorValue( 'var(--red)', element ) ).toBe(
'#ff0000'
);
} );
} );
} );