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

Jump to bottom

Format library: fix unsetting highlight color #37062

Merged
merged 4 commits into from
Dec 7, 2021
Merged

Format library: fix unsetting highlight color #37062

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
f134ea8
Format library: fix unsetting highlight color
ellatrix Dec 2, 2021
1903775
Fix for named colors and add e2e test
ellatrix Dec 6, 2021
9635057
Use new constant `transparentValue` throughout
mcsf Dec 6, 2021
afec558
Address feedback
ellatrix Dec 6, 2021
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
13 changes: 13 additions & 0 deletions packages/e2e-tests/specs/editor/various/format-library/__snapshots__/text-color.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`RichText should remove highlighting element 1`] = `
"<!-- wp:paragraph -->
<p><mark style=\\"background-color:rgba(0, 0, 0, 0)\\" class=\\"has-inline-color has-cyan-bluish-gray-color\\">1</mark></p>
<!-- /wp:paragraph -->"
`;
exports[`RichText should remove highlighting element 2`] = `
"<!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph -->"
`;
46 changes: 46 additions & 0 deletions packages/e2e-tests/specs/editor/various/format-library/text-color.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* WordPress dependencies
*/
import {
createNewPost,
getEditedPostContent,
clickBlockAppender,
pressKeyWithModifier,
clickBlockToolbarButton,
} from '@wordpress/e2e-test-utils';

describe( 'RichText', () => {
beforeEach( async () => {
await createNewPost();
} );

it( 'should remove highlighting element', async () => {
await clickBlockAppender();

// Add text and select to color.
await page.keyboard.type( '1' );
await pressKeyWithModifier( 'primary', 'a' );
await clickBlockToolbarButton( 'More' );

const button = await page.waitForXPath(
`//button[text()='Highlight']`
);
// Clicks may fail if the button is out of view. Assure it is before click.
await button.evaluate( ( element ) => element.scrollIntoView() );
await button.click();

// Use a color name with multiple words to ensure that it becomes
// active. Previously we had a broken regular expression.
const option = await page.waitForSelector(
'[aria-label="Color: Cyan bluish gray"]'
);

await option.click();

expect( await getEditedPostContent() ).toMatchSnapshot();

await option.click();

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );
8 changes: 5 additions & 3 deletions packages/format-library/src/text-color/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { removeFormat } from '@wordpress/rich-text';
*/
import { default as InlineColorUI, getActiveColors } from './inline';

export const transparentValue = 'rgba(0, 0, 0, 0)';
Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a commit replacing all remaining occurrences of this string in format-library with the new constant. This should decrease the chances of format mismatches creeping in — for example, if the setter uses a hardcoded string like rgba(0,0,0,0) (with no spaces), the logic breaks.

The module constant, coupled with the expanded test coverage, might be enough. Otherwise we could use a regular expression.


const name = 'core/text-color';
const title = __( 'Highlight' );

Expand All @@ -30,7 +32,7 @@ function getComputedStyleProperty( element, property ) {

if (
property === 'background-color' &&
value === 'rgba(0, 0, 0, 0)' &&
value === transparentValue &&
element.parentElement
) {
return getComputedStyleProperty( element.parentElement, property );
Expand All @@ -47,7 +49,7 @@ function fillComputedColors( element, { color, backgroundColor } ) {
return {
color: color || getComputedStyleProperty( element, 'color' ),
backgroundColor:
backgroundColor === 'rgba(0, 0, 0, 0)'
backgroundColor === transparentValue
? getComputedStyleProperty( element, 'background-color' )
: backgroundColor,
};
Expand Down Expand Up @@ -139,7 +141,7 @@ export const textColor = {
if ( key !== 'style' ) return value;
// We should not add a background-color if it's already set
if ( value && value.includes( 'background-color' ) ) return value;
const addedCSS = [ 'background-color', 'rgba(0, 0, 0, 0)' ].join( ':' );
const addedCSS = [ 'background-color', transparentValue ].join( ':' );
// Prepend `addedCSS` to avoid a double `;;` as any the existing CSS
// rules will already include a `;`.
return value ? [ addedCSS, value ].join( ';' ) : addedCSS;
Expand Down
14 changes: 8 additions & 6 deletions packages/format-library/src/text-color/inline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { textColor as settings } from './index';
import { textColor as settings, transparentValue } from './index';

function parseCSS( css = '' ) {
return css.split( ';' ).reduce( ( accumulator, rule ) => {
if ( rule ) {
const [ property, value ] = rule.split( ':' );
if ( property === 'color' ) accumulator.color = value;
if ( property === 'background-color' )
if ( property === 'background-color' && value !== transparentValue )
accumulator.backgroundColor = value;
}
return accumulator;
Expand All @@ -44,9 +44,11 @@ function parseCSS( css = '' ) {

function parseClassName( className = '', colorSettings ) {
return className.split( ' ' ).reduce( ( accumulator, name ) => {
const match = name.match( /^has-([^-]+)-color$/ );
if ( match ) {
const [ , colorSlug ] = name.match( /^has-([^-]+)-color$/ );
// `colorSlug` could contain dashes, so simply match the start and end.
if ( name.startsWith( 'has-' ) && name.endsWith( '-color' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a comment here explaining that colorSlug may be more than one word long, lest someone reintroduce a naive regular expression.

const colorSlug = name
.replace( /^has-/, '' )
.replace( /-color$/, '' );
const colorObject = getColorObjectByAttributeValues(
colorSettings,
colorSlug
Expand Down Expand Up @@ -88,7 +90,7 @@ function setColors( value, name, colorSettings, colors ) {
styles.push( [ 'background-color', backgroundColor ].join( ':' ) );
} else {
// Override default browser color for mark element.
styles.push( [ 'background-color', 'rgba(0, 0, 0, 0)' ].join( ':' ) );
styles.push( [ 'background-color', transparentValue ].join( ':' ) );
}

if ( color ) {
Expand Down