Refactor how the Global Styles access and sets data #35264
Conversation
youknowriad
added
[Type] Code Quality
| function useGlobalStylesUserConfig() { | ||
| const globalStylesId = useSelect( ( select ) => { | ||
| return select( editSiteStore ).getSettings() | ||
| .__experimentalGlobalStylesUserEntityId; |
There was a problem hiding this comment.
Right now this is how we retrieve the current global styles entity ID. This is not great, we shouldn't be depending on the a setting of editSite store, The proposed endpoint here would solve this #35141
| 'wp_global_styles', | ||
| 'content', | ||
| globalStylesId | ||
| ); |
There was a problem hiding this comment.
The way we retrieve user global styles (CPT endpoint) and base global styles (__experimentalGlobalStylesBaseStyles setting) is inconsistent, we should try to unify this. The proposed endpoint here would solve this #35141
| } ); | ||
|
|
||
| return supportKeys; | ||
| } |
There was a problem hiding this comment.
The new hooks useSetting and useStyle might be a bit less performant than the previous APIs in. global-styles-provider (which we should try to remove) because I'm not using "context" at all. I believe it's something we can improve later if we do notice issues, right now I wanted to focus on DevX
|
Size Change: +319 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
| useHasBorderWidthControl( { supports, name } ), | ||
| useHasBorderColorControl( name ), | ||
| useHasBorderRadiusControl( name ), | ||
| useHasBorderStyleControl( name ), |
There was a problem hiding this comment.
Removing the supports property is a simple change that makes the code simpler 👍
| : `styles.blocks.${ blockName }.${ path }`; | ||
|
|
||
| const setStyle = ( newValue ) => { | ||
| const newUserConfig = cloneDeep( userConfig ); |
There was a problem hiding this comment.
I guess we could easily improve the performance and even code readability by using the immutableSet function that we already use on other parts of the codebase:
function immutableSet( object, path, value ) {
return setWith( object ? clone( object ) : {}, path, value, clone );
}
| return ( | ||
| useSetting( 'border.customColor', name ) && | ||
| useSetting( 'border.customColor', name )[ 0 ] && |
There was a problem hiding this comment.
Having to use [ 0 ] when we just need to retrieve a value is the main downside of the new API, but I guess we will need to live with that.
There was a problem hiding this comment.
To improve readability, maybe we could switch to a slightly more verbose syntax?
// ...
const [ isEnabledInSettings ] = useSetting( 'border.customColor', name );
return isEnabledInSettings && supports.includes( 'borderColor' );We'd then apply this syntax across all code changes in the PR
There was a problem hiding this comment.
Yes although more verbose, that syntax is more clear.
|
I wonder a bit about performance and how things will scale but the current solution is not perfect either. I think if we follow the endpoint approach and remove the need to serialize and unserialize things become a little better. If we don't use the endpoint we may use the transient mechanism on entities. |
| parentMenu={ '/blocks/' + name } | ||
| context={ blocks[ name ] } | ||
| /> | ||
| <ContextMenu parentMenu={ '/blocks/' + name } name={ name } /> |
There was a problem hiding this comment.
Why is ContextMenu named this way? Maybe we can find a name that explains a bit better what's in it?
There was a problem hiding this comment.
Yes, I struggled to find a good name for this component. (It's the global styles menu for a give context, a context can be "root" or a specific block, potentially other kind of context in the future)
There was a problem hiding this comment.
Got it! Maybe calling it GlobalStylesMenu would be a more clear name? Not big deal anyway.
|
The API is nice and I think provides a better development experience. The fact that we are not using context may bring some challenges. I guess with the plans we have for themes where during theme switching we may alternate between styles of one and styles of the other we may have cases where we want to preview the styles of different themes. With context that is simple it is matter of wrapping the preview contexts with different global style providers I guess with hooks we can also wrap with different EntityProviders? I would love to leave the door open for this use cases. |
jorgefilipecosta
force-pushed
the
refactor/global-styles-data
branch
from
411a166
to
5852786
Compare
There was a problem hiding this comment.
I wired the hooks to our variable mechanism, excluding backgrounds and gradients (that have a specific issue), everything seems to be working, and we use preset variables, etc.
But I think what we have now is not something we can follow. We are constantly on each hook usage serializing and unserializing the structure, doing merge operations between the base and user styles, and processing the user styles to add and remove their origin on presets.
We need to hold a structure somewhere that represents the result of the merge between base and user styles, that structure is important for the variable retrieving mechanism to work on (and that mechanism is called on every useStyle usage), and that structure can also be used to compile the styles.
Previously that structure was on the GlobalStylesProvider.
I see two possibilities.
The first is we keep a GlobalStylesProvider that contains the merge results. That provider is also responsible for using our entity mechanism to make changes. The useStyle, useSetting hooks would interact with the provider, our API would exactly be the same as on this PR, but the hooks would only work when wrapped in a provider. On that provider, we would keep a central object with the result of merging user and base styles that provider would also compile the stylesheet etc., similar to what we have now.
The other possibility is having the result of the merge between user and base as a transient object (similar to how blocks are transient) on the entity where we have the user styles. Given that the base styles are immutable, this approach may work well.
My changes are in commit 5852786 (#35264). Feel free to discard or apply any change to this commit, The objective of the commit is to show what we need to do to connect the hooks with our variable engine. The changes on file packages/edit-site/src/components/editor/utils.js are a port from PR #34178; because the functions there are better isolated, we should base future work on this version.
Using the context is not out of the question, For now, I wanted to keep this as simple as possible and avoid the "Provider" so that's what I removed the context, but it should be easy to bring back if we ever want to do it (without any change on the consumer components) |
Yes, I'm aware of that, but did you notice any real performance issues? My initial plan was to optimize for devx and iterate to improve performance... Can we have a metric to track maybe? |
|
I think if we noticed perf issues... the context solution is probably the simplest one to go for. |
| @@ -39,6 +79,16 @@ function useGlobalStylesUserConfig() { | |||
| let parsedConfig; | |||
| try { | |||
| parsedConfig = content ? JSON.parse( content ) : {}; | |||
| // It is very important to verify if the flag isGlobalStylesUserThemeJSON is true. | |||
| // If it is not true the content was not escaped and is not safe. | |||
| if ( ! parsedConfig.isGlobalStylesUserThemeJSON ) { | |||
There was a problem hiding this comment.
I don't understand this? How can we retrieve an unsafe content at this point? Should we escape in the backend instead?
There was a problem hiding this comment.
The WordPress escape filters don't provide information about which post type we are escaping etc, they just pass content to the filter, so we rely on the flag isGlobalStylesUserThemeJSON being present to know if we should escape the json or not. An attacker can easily bypass our escaping mechanism by not including the flag isGlobalStylesUserThemeJSON so we need on every usage to verify if the flag is present otherwise things may be unsafe.
If we have a specific endpoint that verification should be done there right now we don't have one so we need to keep this check on the client.
There was a problem hiding this comment.
It's still unclear to me, why not always escape?
There was a problem hiding this comment.
If we have a specific endpoint that verification should be done there right now we don't have one so we need to keep this check on the client.
I guess that's the missing piece for me, let's try to bring that endpoint after this PR. it seems it would solve a lot of complexity/issues.
There was a problem hiding this comment.
It's still unclear to me, why not always escape?
Because you just receive a string of content, you can not escape a given JSON or HTML or anything in the post content with the theme.json rules. That content may be a JSON from a plugin following totally different sets of rules for what should be escaped. The escape functions does not receive anything besides a string of content that flag indicates that content is a user theme.json.
| @@ -105,10 +163,10 @@ export function useSetting( path, blockName, source = 'all' ) { | |||
| let result; | |||
| switch ( source ) { | |||
| case 'all': | |||
| result = get( userConfig, finalPath ) ?? getBaseSetting(); | |||
| result = get( userConfig, finalPath, {} ).user ?? getBaseSetting(); | |||
There was a problem hiding this comment.
This change is confusing to me. Why do we have a .user subkey in the configs?
There was a problem hiding this comment.
This seems to be related to having .user as values for some configs, now that we can easily access all "user", "base" values directly maybe we can normalize all configs and drop this sunkey entirely.
There was a problem hiding this comment.
I think we still need the subkey, base includes core and theme so we need the subkey to differentiate e.g: the UI will show both. So I guess we can normalize to always have subkeys as it is doing now.
There was a problem hiding this comment.
the UI will show
The UI can do
useSetting( 'something', context, 'all' ); // merged
useSetting( 'something', context, 'user' ); // user
useSetting( 'something', context, 'base' ); // base
so the object itself shouldn't have the keys. it's just adds complexity and it's not consistent across settings and styles (and even inside settings)
There was a problem hiding this comment.
How would the UI differentiate between core and theme palettes? The base needs to have subjects for core and theme, the UI shows them differently.
There was a problem hiding this comment.
It could be a different API call (query arg or something). Anyway, it feels like implementation detail for now but I feel it would simplify a lot of things if the "configs" had a consistent shape, regardless how they're retrieved and the arguments (merged, user, base,...).
There was a problem hiding this comment.
it would simplify a lot of things if the "configs" had a consistent shape, regardless of how they're retrieved and the
I agree with that but I think we are being consistent if all the presets no matter where they come from are grouped by origin e.g: in base object we have { core: [], theme: [] }, in user we have { user: [] }, in the merged object we have { core: [], theme: [], user: [] }.
Grouping by origin we have a consistent shape where we can always work on.
This change is confusing to me. Why do we have a .user subkey in the configs?
The addition of the user subkey is done in order to improve consistency we make presets always have an origin subkey.
But I think I understand the point that in an user object having a user subkey is redundant it is also ok for me if the user object does not have the subkey as long as the base ( { core: [], theme: [] } ) and merged objects ( { core: [], theme: [], user: [] } ) have them.
There was a problem hiding this comment.
But I think I understand the point that in an user object having a user subkey is redundant it is also ok for me if the user object
Yes, this is the issue I have with this but it's not specific to the user object for me, it's for all objects. We should have a "core object", a "theme object", a "user object" and a "merged object" all with no subkeys.
There was a problem hiding this comment.
We should have a "core object", a "theme object", a "user object" and a "merged object" all with no subkeys.
On the block editor, we are also going to show different color palettes per origin. So we thought it would be better to pass a single object to the block editor where presets are keyed by origin instead of passing multiple objects and that by origin subkey approach appeared. We can have multiple objects but in that case, we also need to make the block editor aware of the multiple objects, the block editor will never need anything from other sources besides the presets so the by origin approach seemed a better fit.
There was a problem hiding this comment.
Got it 👍 I guess there's drawbacks for each, let's keep it as is then and continue monitoring for the future.
|
@jorgefilipecosta I think some of your changes might have broken the "blocks" panels. |
|
I will leave the overall code review to @oandregal and @jorgefilipecosta , since I'm not very experienced in this area of the repo and its general architecture. But I did some smoke testing and came across a few issues: I wasn't able to add a custom color to the palette global-styles-sidebar-custom-color.mp4I wasn't able to change the text color of the global-styles-heading-text-color.mp4I haven't smoke tested every single combination of block/setting, so there may be more issue like this that I haven't found yet. |
|
@ciampo Both the issues are fixed for me, for the second one I'm not sure if it's a real issue or something about style specificity battle in your particular example. |
I confirm that the first issue is fixed. Regarding the second issue (setting the text color of the Heading block), you were correct on it not being an issue. I believe the "conflict" comes from the fact that in my example, the headings were also links. I tried to create a separate heading without any linked text, and everything worked as expected |
| return [ resultWithFallback, setSetting ]; | ||
| } | ||
|
|
||
| export function useStyle( path, blockName, source = 'all' ) { |
There was a problem hiding this comment.
What are your thoughts on accommodating elements in this API? I'm thinking about this use case: UI controls for elements.
It is something we'll add to the global styles sidebar soon. Potentially, we should also add it to the blocks UI panels (both in the block sidebar & the global sidebar): for example, at the moment, users can't update all the styles of link color despite the theme can. To support this, we'll need the ability to address elements individually. Elements can be at the top or within a block.
In this other conversation I was thinking along these lines:
useSetting( path, { block: 'blockName', element: 'elementName' }, source )
- top level:
useStyle( 'group.property' ) - block level:
useStyle( 'group.property', { block: 'block/name' } ) - element at top-level:
useStyle( 'group.property', { element: 'element/name' } ) - element at block-level:
useStyle( 'group.property', { block: 'block/name', element: 'element/name' } )
Whatever the choice to address this, I feel it should be the same in both the client and the server
There was a problem hiding this comment.
I'd probably do the same for useSetting, although at the moment elements can't be in the settings. They may need that when/if they get added to the block UI panels. We may also have other contexts than blocks, so being an object will allow us to grow the API easily.
There was a problem hiding this comment.
Well right now you can do useStyle( 'elements.link.background' ) if you want to get/set the background of a link for instance. I agree it's not perfect yet and thinking of "elements" as "contexts" might make sense. These APIs are internal for now so there's room for iterations to see what would work well or not.
There was a problem hiding this comment.
Yeah, I know. This PR shouldn't be blocked by this. it's something we can iterate on. Though my concern with that approach is that it ties consumers to a specific theme.json shape, which we tried to avoid by separating "path" and "context" in the hooks/utilities.
| ]; | ||
| }; | ||
|
|
||
| export function useSetting( path, blockName, source = 'all' ) { |
There was a problem hiding this comment.
The useSetting hook on the block editor has a slightly different API the source is part of the path.
useSetting( 'color.palette' ) returns merged color palette.
useSetting( 'color.palette.user' ) returns user color palette.
We may need to consolidate these two approaches.
| name | ||
| ); | ||
| const [ userLinkColor ] = useStyle( | ||
| 'elements.link.color.text', |
There was a problem hiding this comment.
I'm not sure if elements should be treated as part of the path or if they should be something like a block in their own right. But I guess we can follow this approach for now.
There was a problem hiding this comment.
Agreed, it's not clear. See #35264 (comment)
packages/edit-site/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/dimensions-panel.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @youknowriad,
I think the performance issue is noticeable even when no changes are applied and just by navigating around the panel.
I did this test in chrome with 6x CPU throttling ( I think many people have CPU even lower than my machine with throttling). The time it takes to open the color section is huge.
In this PR:
Oct-06-2021.16-01-29.mp4
In trunk:
Oct-06-2021.16-15-22.mp4
packages/edit-site/src/components/global-styles/color-palette-panel.js
Outdated
Show resolved
Hide resolved
|
I also notice some lag when moving the color picker around even without throttling (for example for a custom background color), the UI feels slow and takes some time to represent the choose background color. The trunk is not perfect on this either. |
| return ( | ||
| <> | ||
| <ScreenHeader back="/" title={ __( 'Blocks' ) } /> | ||
| { visibleBlocks.map( ( block ) => ( | ||
| { getBlockTypes().map( ( block ) => ( |
There was a problem hiding this comment.
Reviewing this change, I've noticed that hasBlockMenuItem a few lines above is missing the hasBorderPanel || hasDimensionsPanel checks as well, so we won't list blocks that support only border or dimensions (or both).
There was a problem hiding this comment.
It's included in hasLayoutPanel so we should be fine I think.
There was a problem hiding this comment.
Oh, I missed that hasLayoutPanel is precisely the two I brought up. It's all good.
There was a problem hiding this comment.
When I apply a link color at the block level(paragraph link color) the color is not reflected on the editor but is reflected on the frontend. Inspecting the styles on the editor I see the style is not present. On trunk paragraph link color appears as expected.
There was a problem hiding this comment.
Thank you for all the work on this refactor 👍 I noticed an issue with the block level link color. I also think we should follow up and improve the performance with some throttling the slowness is noticeable just by moving between panels, and even without throttling, I notice some slowness while changing styles fast.
I think these are things we can work on after, provided the link color regression is solved I think we can merge this PR.
|
@jorgefilipecosta I fixed the issue 👍 bad refactoring :) For the performance side, is this something you want to try separately? (Maybe move the |
yes it is something to look separately and not a blocker. |
This PR is not working yet but the code is in a state good enough to showcase what I'm trying to achieve:
global-styles/hooksfile with no dependency to context or whatsoever. Ultimately we should be able to move the wholeglobal-stylesfolder anywhere.contextName(blockName or undefined for root)Notes
useStyleneed to make transformations to values (it can't return the stored value). The current methods to do that ineditor/utilslook a bit too complex to me. I could use some help there cc @jorgefilipecosta. Can we simplify these? should we move them toglobal-styles/utils