From d7281f476351b6f9ccacbe8dcc1b258e432d7579 Mon Sep 17 00:00:00 2001 From: ramonjd Date: Tue, 16 Aug 2022 15:35:02 +1000 Subject: [PATCH 1/3] A follow up for https://github.com/WordPress/gutenberg/pull/43166 The editor did not correctly resolve dynamic refs and the style engine does not yet support resolution. This commit adds a utility function to resolve dynmamic refs and add conditions to prevent recursive refs. --- .../test/use-global-styles-output.js | 52 +++++++++++++++++ .../components/global-styles/test/utils.js | 58 +++++++++++++++++-- .../global-styles/use-global-styles-output.js | 50 +++++++++------- .../src/components/global-styles/utils.js | 34 +++++++---- 4 files changed, 156 insertions(+), 38 deletions(-) diff --git a/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js b/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js index 8e8686ca4eed8e..17e7632956193e 100644 --- a/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js +++ b/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js @@ -756,5 +756,57 @@ describe( 'global styles renderer', () => { 'font-family: sans-serif', ] ); } ); + + it( 'Should resolve dynamic references', () => { + const blockStylesWithRef = { + color: { + background: { + ref: 'styles.color.text', + }, + text: 'king-crimson', + }, + }; + const tree = { + styles: blockStylesWithRef, + }; + expect( + getStylesDeclarations( + blockStylesWithRef, + '.wp-selector', + true, + tree + ) + ).toEqual( [ + 'color: king-crimson', + 'background-color: king-crimson', + ] ); + } ); + + it( 'Should skip dynamic references that point to other dynamic references', () => { + const blockStylesWithRef = { + spacing: { + padding: { + ref: 'styles.spacing.margin', + }, + margin: { + ref: 'styles.typography.letterSpacing', + }, + }, + typography: { + letterSpacing: '20px', + }, + }; + const tree = { + styles: blockStylesWithRef, + }; + expect( + getStylesDeclarations( + blockStylesWithRef, + '.wp-selector', + false, + tree + ) + ).toEqual( [ 'margin: 20px', 'letter-spacing: 20px' ] ); + } ); } ); } ); diff --git a/packages/edit-site/src/components/global-styles/test/utils.js b/packages/edit-site/src/components/global-styles/test/utils.js index 7d0e3464557f61..8def75c026ecfb 100644 --- a/packages/edit-site/src/components/global-styles/test/utils.js +++ b/packages/edit-site/src/components/global-styles/test/utils.js @@ -1,7 +1,11 @@ /** * Internal dependencies */ -import { getPresetVariableFromValue, getValueFromVariable } from '../utils'; +import { + getPresetVariableFromValue, + getValueFromVariable, + resolveDynamicRef, +} from '../utils'; describe( 'editor utils', () => { const themeJson = { @@ -163,7 +167,7 @@ describe( 'editor utils', () => { expect( actual ).toBe( stylesWithRefs.styles.color.text ); } ); - it( 'returns the originally provided value where value is dynamic reference and reference does not exist', () => { + it( 'returns the originally provided variable where value is dynamic reference and reference does not exist', () => { const stylesWithRefs = { ...themeJson, styles: { @@ -178,10 +182,12 @@ describe( 'editor utils', () => { ref: 'styles.color.text', } ); - expect( actual ).toBe( stylesWithRefs.styles.color.text ); + expect( actual ).toEqual( { + ref: 'styles.color.text', + } ); } ); - it( 'returns the originally provided value where value is dynamic reference', () => { + it( 'returns the originally provided variable where value is dynamic reference', () => { const stylesWithRefs = { ...themeJson, styles: { @@ -199,7 +205,49 @@ describe( 'editor utils', () => { ref: 'styles.color.text', } ); - expect( actual ).toBe( stylesWithRefs.styles.color.text ); + expect( actual ).toEqual( { + ref: 'styles.color.text', + } ); + } ); + } ); + } ); + + describe( 'resolveDynamicRef', () => { + it( 'returns the resolved value', () => { + const tree = { + styles: { + typography: { + letterSpacing: '20px', + }, + }, + }; + expect( + resolveDynamicRef( + { + ref: 'styles.typography.letterSpacing', + }, + tree + ) + ).toEqual( '20px' ); + } ); + + it( 'returns the originally provided value where a value cannot be found', () => { + const tree = { + styles: { + typography: { + letterSpacing: '20px', + }, + }, + }; + expect( + resolveDynamicRef( + { + ref: 'styles.spacing.margin', + }, + tree + ) + ).toEqual( { + ref: 'styles.spacing.margin', } ); } ); } ); diff --git a/packages/edit-site/src/components/global-styles/use-global-styles-output.js b/packages/edit-site/src/components/global-styles/use-global-styles-output.js index 16f2dacba411f7..b4aeb1a30f9539 100644 --- a/packages/edit-site/src/components/global-styles/use-global-styles-output.js +++ b/packages/edit-site/src/components/global-styles/use-global-styles-output.js @@ -32,7 +32,12 @@ import { /** * Internal dependencies */ -import { PRESET_METADATA, ROOT_BLOCK_SELECTOR, scopeSelector } from './utils'; +import { + PRESET_METADATA, + ROOT_BLOCK_SELECTOR, + scopeSelector, + resolveDynamicRef, +} from './utils'; import { GlobalStylesContext } from './context'; import { useSetting } from './hooks'; @@ -208,11 +213,21 @@ export function getStylesDeclarations( return declarations; } const pathToValue = value; - if ( first( pathToValue ) === 'elements' || useEngine ) { + let styleValue = get( blockStyles, pathToValue, false ); + + if ( + first( pathToValue ) === 'elements' || + // The style engine cannoresolveDynamicReft handle dynamic refs yet. + ( useEngine && ! styleValue?.ref ) + ) { return declarations; } - const styleValue = get( blockStyles, pathToValue ); + styleValue = resolveDynamicRef( styleValue, tree ); + // If get() finds no style value or the ref points to another ref (invalid), return. + if ( styleValue === false || !! styleValue?.ref ) { + return declarations; + } // Root-level padding styles don't currently support strings with CSS shorthand values. // This may change: https://github.com/WordPress/gutenberg/issues/40132. @@ -226,30 +241,30 @@ export function getStylesDeclarations( if ( !! properties && typeof styleValue !== 'string' ) { Object.entries( properties ).forEach( ( entry ) => { const [ name, prop ] = entry; - - if ( ! get( styleValue, [ prop ], false ) ) { + const propStyleValue = get( styleValue, [ prop ], false ); + if ( ! propStyleValue ) { // Do not create a declaration // for sub-properties that don't have any value. - return; + return declarations; } const cssProperty = name.startsWith( '--' ) ? name : kebabCase( name ); + declarations.push( `${ cssProperty }: ${ compileStyleValue( - get( styleValue, [ prop ] ) + propStyleValue ) }` ); } ); - } else if ( get( blockStyles, pathToValue, false ) ) { + } else { const cssProperty = key.startsWith( '--' ) ? key : kebabCase( key ); + declarations.push( - `${ cssProperty }: ${ compileStyleValue( - get( blockStyles, pathToValue ) - ) }` + `${ cssProperty }: ${ compileStyleValue( styleValue ) }` ); } @@ -274,18 +289,9 @@ export function getStylesDeclarations( ? rule.key : kebabCase( rule.key ); - let ruleValue = rule.value; - if ( typeof ruleValue !== 'string' && ruleValue?.ref ) { - const refPath = ruleValue.ref.split( '.' ); - ruleValue = get( tree, refPath ); - // Presence of another ref indicates a reference to another dynamic value. - // Pointing to another dynamic value is not supported. - if ( ! ruleValue || !! ruleValue?.ref ) { - return; - } + if ( typeof rule.value === 'string' ) { + output.push( `${ cssProperty }: ${ rule.value }` ); } - - output.push( `${ cssProperty }: ${ ruleValue }` ); } ); return output; diff --git a/packages/edit-site/src/components/global-styles/utils.js b/packages/edit-site/src/components/global-styles/utils.js index 4c9d7bae19f7a1..3b5d8447284469 100644 --- a/packages/edit-site/src/components/global-styles/utils.js +++ b/packages/edit-site/src/components/global-styles/utils.js @@ -238,18 +238,9 @@ function getValueFromCustomVariable( features, blockName, variable, path ) { * @return {string|*|{ref}} The value of the CSS var, if found. If not found, the passed variable argument. */ export function getValueFromVariable( features, blockName, variable ) { + variable = resolveDynamicRef( variable, features ); if ( ! variable || typeof variable !== 'string' ) { - if ( variable?.ref && typeof variable?.ref === 'string' ) { - const refPath = variable.ref.split( '.' ); - variable = get( features, refPath ); - // Presence of another ref indicates a reference to another dynamic value. - // Pointing to another dynamic value is not supported. - if ( ! variable || !! variable?.ref ) { - return variable; - } - } else { - return variable; - } + return variable; } const USER_VALUE_PREFIX = 'var:'; const THEME_VALUE_PREFIX = 'var(--wp--'; @@ -321,3 +312,24 @@ export function scopeSelector( scope, selector ) { return selectorsScoped.join( ', ' ); } + +/** + * Attempts a lookup of the theme.json tree to fetch the value of dynamic ref. + * This function exists until the style engine supports ref resolution. + * + * @param {string|*} styleValue An incoming style value. + * @param {Object} tree GlobalStylesContext config, e.g., user, base or merged. Represents the theme.json tree. + * @return {string|*} The value of the dynamic ref, if found. If not found, `false`. + */ +export function resolveDynamicRef( styleValue, tree ) { + const ref = get( styleValue, [ 'ref' ], false ); + if ( typeof ref === 'string' ) { + const resolvedValue = get( tree, ref.split( '.' ) ); + // Presence of another ref indicates a reference to another dynamic value. + // Pointing to another dynamic value is not supported. + if ( !! resolvedValue && ! resolvedValue?.ref ) { + return resolvedValue; + } + } + return styleValue; +} From 29bec5eda2900b3ef0a57544de975579c6196942 Mon Sep 17 00:00:00 2001 From: ramonjd Date: Tue, 16 Aug 2022 17:00:54 +1000 Subject: [PATCH 2/3] Fixed tests --- .../components/global-styles/test/use-global-styles-output.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js b/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js index 17e7632956193e..ad1062869d6926 100644 --- a/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js +++ b/packages/edit-site/src/components/global-styles/test/use-global-styles-output.js @@ -777,8 +777,8 @@ describe( 'global styles renderer', () => { tree ) ).toEqual( [ - 'color: king-crimson', 'background-color: king-crimson', + 'color: king-crimson', ] ); } ); From 7e71b16fd5d1bf52466fdb67d82002b8ddc21bac Mon Sep 17 00:00:00 2001 From: ramonjd Date: Tue, 30 Aug 2022 11:24:40 +1000 Subject: [PATCH 3/3] Return empty string for circular refs so that editor controls can work with a string value, albeit empty. --- .../components/global-styles/test/utils.js | 30 ++++++++++++++----- .../global-styles/use-global-styles-output.js | 6 ++-- .../src/components/global-styles/utils.js | 7 +++-- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/packages/edit-site/src/components/global-styles/test/utils.js b/packages/edit-site/src/components/global-styles/test/utils.js index 8def75c026ecfb..581997f4574841 100644 --- a/packages/edit-site/src/components/global-styles/test/utils.js +++ b/packages/edit-site/src/components/global-styles/test/utils.js @@ -182,12 +182,10 @@ describe( 'editor utils', () => { ref: 'styles.color.text', } ); - expect( actual ).toEqual( { - ref: 'styles.color.text', - } ); + expect( actual ).toEqual( '' ); } ); - it( 'returns the originally provided variable where value is dynamic reference', () => { + it( 'returns an empty string where value is dynamic reference', () => { const stylesWithRefs = { ...themeJson, styles: { @@ -205,9 +203,7 @@ describe( 'editor utils', () => { ref: 'styles.color.text', } ); - expect( actual ).toEqual( { - ref: 'styles.color.text', - } ); + expect( actual ).toEqual( '' ); } ); } ); } ); @@ -231,6 +227,26 @@ describe( 'editor utils', () => { ).toEqual( '20px' ); } ); + it( 'returns empty string if reference to another ref is found', () => { + const tree = { + styles: { + typography: { + letterSpacing: { + ref: 'styles.typography.fontSize', + }, + }, + }, + }; + expect( + resolveDynamicRef( + { + ref: 'styles.typography.letterSpacing', + }, + tree + ) + ).toEqual( '' ); + } ); + it( 'returns the originally provided value where a value cannot be found', () => { const tree = { styles: { diff --git a/packages/edit-site/src/components/global-styles/use-global-styles-output.js b/packages/edit-site/src/components/global-styles/use-global-styles-output.js index b4aeb1a30f9539..79eb9cd1bf206d 100644 --- a/packages/edit-site/src/components/global-styles/use-global-styles-output.js +++ b/packages/edit-site/src/components/global-styles/use-global-styles-output.js @@ -213,11 +213,11 @@ export function getStylesDeclarations( return declarations; } const pathToValue = value; - let styleValue = get( blockStyles, pathToValue, false ); + let styleValue = get( blockStyles, pathToValue, '' ); if ( first( pathToValue ) === 'elements' || - // The style engine cannoresolveDynamicReft handle dynamic refs yet. + // The style engine cannot handle dynamic refs yet. ( useEngine && ! styleValue?.ref ) ) { return declarations; @@ -225,7 +225,7 @@ export function getStylesDeclarations( styleValue = resolveDynamicRef( styleValue, tree ); // If get() finds no style value or the ref points to another ref (invalid), return. - if ( styleValue === false || !! styleValue?.ref ) { + if ( styleValue === '' || !! styleValue?.ref ) { return declarations; } diff --git a/packages/edit-site/src/components/global-styles/utils.js b/packages/edit-site/src/components/global-styles/utils.js index 3b5d8447284469..1a4fa383d5ba22 100644 --- a/packages/edit-site/src/components/global-styles/utils.js +++ b/packages/edit-site/src/components/global-styles/utils.js @@ -319,7 +319,7 @@ export function scopeSelector( scope, selector ) { * * @param {string|*} styleValue An incoming style value. * @param {Object} tree GlobalStylesContext config, e.g., user, base or merged. Represents the theme.json tree. - * @return {string|*} The value of the dynamic ref, if found. If not found, `false`. + * @return {string|*} The value of the dynamic ref, if found. If not found, returns `styleValue`. If a reference to another ref is found, returns an empty string. */ export function resolveDynamicRef( styleValue, tree ) { const ref = get( styleValue, [ 'ref' ], false ); @@ -327,7 +327,10 @@ export function resolveDynamicRef( styleValue, tree ) { const resolvedValue = get( tree, ref.split( '.' ) ); // Presence of another ref indicates a reference to another dynamic value. // Pointing to another dynamic value is not supported. - if ( !! resolvedValue && ! resolvedValue?.ref ) { + if ( !! resolvedValue?.ref ) { + return ''; + } + if ( !! resolvedValue ) { return resolvedValue; } }