Fix mutated global styles context #49623
Closed
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
ajlende
added
[Type] Bug
|
Size Change: +51 B (0%) Total Size: 1.35 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7a60a68. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4623199771
|
youknowriad
reviewed
Apr 6, 2023
| @@ -780,6 +783,7 @@ export const toStyles = ( | |||
| // Process styles for block support features with custom feature level | |||
| // CSS selectors set. | |||
| if ( featureSelectors ) { | |||
| // Mutates the passed styles object. | |||
| const featureDeclarations = getFeatureDeclarations( | |||
There was a problem hiding this comment.
If this is the function mutating the object (I see a delete there), why don't we update the function to avoid the mutation instead of cloning the whole tree.
There was a problem hiding this comment.
The deletion/mutation is used to format some styles specially. There's code similar to this:
// special case
if ( duotoneSelector ) {
ruleset += `${ duotoneSelector } { filter: ${ styles.filter.duotone }; }`;
delete styles.filter;
}
// standard case (loop inside `getStylesDeclarations`)
if ( styles.filter.duotone ) {
ruleset += `${ selector } { filter: { ${ styles.filter.duotone }; }`;
}This is how getFeatureDeclarations extracts (and deletes) certain styles, and outputs them under a special selector, and how code for duotone extracts the filter.duotone styles. The rest is left for the standard getStylesDeclarations to handle.
To avoid the delete mutation, we'd have to rewrite this logic. Like building an exclude list of already processed paths and passing it to getStylesDeclarations.
We can continue cloning the styles object, but I think that cloning the entire tree object is too much. We're mutating only the styles extracted by getNodesWithStyles. So, it could be that function that clones the styles when calling nodes.push( { styles: clonedStyles } ). The pickStyleKeys could do the cloning.
|
This no longer appears to be broken in trunk. I'm going to close the PR but please reopen if your testing says differently! |
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 an issue where duotone presets aren't properly being selected in the global styles filters panel, and maybe other things where the
GlobalStylesContextis being used.Why?
In #49577 (comment), @youknowriad uncovered a bug where the duotone property was removed from the context in the duotone filters panel.
How?
The duotone property was being removed in
getFeatureDeclarationswhich is needed fortoStylesto work, so the entiremergedConfigwas cloned before calling it.Testing Instructions
styles.blocks['core/image].filter.duotoneset to one of the theme or default presets, for examplevar(--wp--preset--duotone--midnight).It might also be good to test some more of the blocks that use feature selectors via the
selectorsorsupports.feature.__experimentalSelectorblock.json API.