Fix styles declarations returning before all properties output #42954
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tellthemachines
added
[Type] Bug
tellthemachines
requested review from
ramonjd,
aaronrobertshaw and
andrewserong
|
Size Change: +102 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Aug 4, 2022
| if ( | ||
| ( key === '--wp--style--root--padding' && | ||
| typeof styleValue === 'string' ) || | ||
| ! useRootPaddingAlign |
There was a problem hiding this comment.
For this condition, is it possible that more declarations will be skipped than desired if ! useRootPaddingAlign evaluates to true? I was wondering if the condition is meant to be just for the --wp--style--root--padding key, like:
if (
key === '--wp--style--root--padding' &&
( typeof styleValue === 'string' || ! useRootPaddingAlign )
) {
return declarations;
}Apologies if I'm not understanding the logic here correctly!
There was a problem hiding this comment.
It looks like fontFamily is one of the few ones that isn't using the style engine, so I found it a little challenging to work out an error state for this one, but I think the following test seems to catch it:
it( 'Should output padding and font family properties if useRootPaddingAwareAlignments is disabled', () => {
const blockStylesWithFontFamily = {
...blockStyles,
typography: {
fontFamily: 'sans-serif',
},
};
expect(
getStylesDeclarations(
blockStylesWithFontFamily,
'body',
false
)
).toEqual( [
'font-family: sans-serif',
'background-color: var(--wp--preset--color--light-green-cyan)',
'padding-top: 33px',
'padding-right: 33px',
'padding-bottom: 33px',
'padding-left: 33px',
] );
} );There was a problem hiding this comment.
Oh, I've found a better real world example with the Duotone feature. In TwentyTwentyTwo, if you add the following to the theme.json file (from #37727) to switch on a particular Duotone for image blocks:
{
"styles": {
"blocks": {
"core/image": {
"filter": {
"duotone": "var(--wp--preset--duotone--foreground-and-secondary)"
}
}
}
}
}| Expected | Actual |
|---|---|
 |
 |
The above change appears to fix it.
There was a problem hiding this comment.
Oh, good catch! Yes, the parentheses should be around the OR as you suggested 😄
andrewserong
reviewed
Aug 4, 2022
There was a problem hiding this comment.
Nice work figuring out this one @tellthemachines (and for adding additional tests 🎉 )! Skipping the output in the loop over extraRules looks like a good approach to me 👍. I just left a comment about the condition to skip the variable output, as it looks like it might be unexpectedly returning early.
Aside from that though, this is otherwise testing nicely for me manually in the site editor with TwentyTwentyTwo and Emptytheme and resolves the issue with color output.
| if ( | ||
| ( key === '--wp--style--root--padding' && | ||
| typeof styleValue === 'string' ) || | ||
| ! useRootPaddingAlign |
There was a problem hiding this comment.
It looks like fontFamily is one of the few ones that isn't using the style engine, so I found it a little challenging to work out an error state for this one, but I think the following test seems to catch it:
it( 'Should output padding and font family properties if useRootPaddingAwareAlignments is disabled', () => {
const blockStylesWithFontFamily = {
...blockStyles,
typography: {
fontFamily: 'sans-serif',
},
};
expect(
getStylesDeclarations(
blockStylesWithFontFamily,
'body',
false
)
).toEqual( [
'font-family: sans-serif',
'background-color: var(--wp--preset--color--light-green-cyan)',
'padding-top: 33px',
'padding-right: 33px',
'padding-bottom: 33px',
'padding-left: 33px',
] );
} );
|
@andrewserong thanks for reviewing and kindly providing the extra test! I fixed the issue, and added font-family to the initial block styles in the test, so now we can check that it gets output on all three tests. |
aristath
approved these changes
Aug 4, 2022
|
@tellthemachines, @andrewserong, @andrewserong would this be a candidate for a 13.8.x release? The root issue is impacting quite a range of things. I just discovered that some theme.json style settings are not getting output, notably those for template parts. With this fix, everything now works like a charm. |
|
@ndiego it's not a huge change so should be fine to add to the next minor release, yes! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #42913.
Why?
When
useRootPaddingAwareAlignmentsis set,getStylesDeclarationsreturns before all the global style properties are output. This means colour changes in global styles aren't reflected in the editor.How?
Change the function logic so it only omits
paddingproperties whenuseRootPaddingAwareAlignmentsis enabled, as well as not outputting the variables if the setting is disabled.Adds tests to check for correct behaviour.
Testing Instructions
"useRootPaddingAwareAlignments": true;"useRootPaddingAwareAlignments"back tofalse;.editor-styles-wrapper, but not both.Screenshots or screencast