toStyles: don't mutate the tree argument#49806
Merged
Merged
Conversation
|
Size Change: -21 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
jsnajdr
commented
Apr 14, 2023
| duotoneStyles.filter = styles.filter; | ||
| delete styles.filter; | ||
| } | ||
|
|
Member
Author
There was a problem hiding this comment.
@ajlende This behavior, where the duotone style is always extracted, and then used only conditionally, when the duotone selector exists, I think it was a bug originally introduced in #37727. It means that if there is a filter.duotone style specified, and no feature selector for it, then the style will be ignored. Instead of being applied to the root selector just like any other style. Not sure if this was ever intended.
The duotone selector was a first ever example of a "feature selector". And the generic feature selector feature was introduced only later. Today, it might be possible to remove the duotone-specific code completely and just use a generic feature selector for it. The only thing that needs to be done is to convert the legacy __experimentalDuotone option into a featured selectors.filter.duotone selector.
Contributor
There was a problem hiding this comment.
Should we go ahead and actually do that?
Member
Author
There was a problem hiding this comment.
By "that" you mean eliminating the duotoneSelector variable and integrate it into featureSelectors? Yes, I could push a commit like this 👍
Contributor
There was a problem hiding this comment.
yes, that's the idea.
Potentially we could add a "shim" within registerBlockType + a deprecated function call.
youknowriad
reviewed
Apr 18, 2023
| const duotoneStyles = {}; | ||
| if ( styles?.filter ) { | ||
| duotoneStyles.filter = styles.filter; | ||
| delete styles.filter; |
Contributor
There was a problem hiding this comment.
Isn't there another fix where we avoid "delete" (avoid mutations at all)? It feels to me like if we keep it around, it can regress again at anytime with some small unintended changes.
Member
Author
There was a problem hiding this comment.
That would be very desirable and I spent some time thinking about how to do it. It wouldn't be a "fix" but rather a major rewrite of the toStyles function.
Currently it processes feature selectors and the root selector in separate loops. Pseudocode:
for ( featSelector of featureSelectors ) {
declarations = [];
for ( style of styles ) {
if ( styleUsesSelector( style, featSelector ) ) {
declarations.push( style );
delete styles[ style ];
}
}
write( `${ featSelector } { ${ declarations.join() } }`;
}
declarations = [];
for ( style of styles ) {
declarations.push( style );
}
write( `${ rootSelector } { ${ declarations.join() } }`;
The second loop processes only styles that haven't been deleted by the first loop.
Rewritten version would have one loop, assigning each style to the right selector:
declarations = new Map<string, string[]>();
for ( style of styles ) {
selector = findFeatureSelector( style ) || rootSelector;
declarations.get( selector ).push( style );
}
for ( [ selector, decls ] of declarations ) {
write( `${ selector } { ${ decls.join() } }` );
}
No delete needed.
Consider this PR a first step towards that goal. It's adding many new unit tests, which will be needed to perform the rewrite safely.
youknowriad
approved these changes
Apr 18, 2023
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
I'm happy to land this to fix the issue, considering we have two todo lists to consider:
- Removing the specific way to define selector for duotone
- Potentially refactor the code to avoid the mutation
jsnajdr
force-pushed
the
update/to-styles
branch
from
4110706 to
412e004
Compare
Member
Author
Yes I can commit to that 🙂 |
bph
added
[Type] Bug
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Alternative to #49623 which doesn't clone the entire
treeobject, but only the style objects that are created bypickStyleKeys. Consumed styles are removed only when the "node" (returned bygetNodesWithStyles) has afeatureSelectorsorduotoneSelectorproperty, and for these nodes thestylesare always created withpickStyleKeys.I'm adding also several new unit tests, to cover various cases: feature selectors, block variation styles, duotone filters...