Conversation
ramonjd
requested review from
andrewserong,
talldan,
tellthemachines and
youknowriad
ramonjd
added
[Type] Bug
- Generate global styles from merged configuration to avoid infinite loops. - Filter out global styles from base settings and merge with generated styles. - Update editor settings to include experimental features from global settings.
- Remove unused import of getBlockTypes. - Simplify global styles generation by excluding block types. - Update base settings to exclude styles and experimental features, generating them from the merged config instead. - Ensure global settings are correctly applied to experimental features.
ramonjd
changed the title
- Remove unused destructuring of styles and experimental features from settings. - Streamline the process of generating base settings for the site editor.
ramonjd
force-pushed
the
fix/theme-styles-not-persisting-across-views
branch
from
4e68b99 to
7faea80
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Member
Author
|
This also affects the template previews: Kapture.2025-12-12.at.18.23.19.mp4I ran out of time today to work out if there's a better, more universal fix here. |
ramonjd
commented
Dec 12, 2025
Comment on lines
71
to
74
| * Exclude styles and __experimentalFeatures from base settings | ||
| * since we're generating fresh ones from the merged config. | ||
| */ | ||
| const { styles, __experimentalFeatures, ...baseSettings } = settings; |
Member
Author
There was a problem hiding this comment.
This smells a bit. Maybe there's a better way. I ran out of time.
Contributor
There was a problem hiding this comment.
Do we need to explicitly exclude them? Won't the fresh ones just override the stale?
Member
Author
There was a problem hiding this comment.
Good question. Not really, I could remove it.
The intention was to mainly to make the intent clear, and:
- prevent accidentally spreading old styles if the code changes
- avoid potential issues if settings.styles has non-global styles we want to preserve
The ideal state is that we have only one source that generates global styles
- Import private APIs for global styles management. - Generate global styles from merged configuration to streamline settings. - Update pattern settings to include generated styles and experimental features. - Ensure proper memoization of global styles for performance optimization.
Member
Author
Meh, I just added the same fix. |
ramonjd
commented
Dec 12, 2025
Comment on lines
+46
to
+51
| const [ globalStyles, globalSettings ] = useMemo( () => { | ||
| return generateGlobalStyles( mergedConfig, [], { | ||
| disableRootPadding: false, | ||
| } ); | ||
| }, [ mergedConfig ] ); | ||
|
|
Member
Author
There was a problem hiding this comment.
I guess we could make a hook out of all this?
Member
Author
There was a problem hiding this comment.
I mean to share between the two files and any others
Contributor
There was a problem hiding this comment.
If this is a temp fix pending a larger refactor then probs not worth it.
|
Size Change: +5.65 kB (+0.22%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
talldan
reviewed
Dec 12, 2025
| ...baseSettings, | ||
| styles: [ | ||
| ...settings.styles, | ||
| ...globalStyles, |
Contributor
There was a problem hiding this comment.
I don't know the background very well, but the PR description mentions that the style settings were moved to the editor package.
I wonder why the styles need to be set here as part of site-editor settings if the editor package is now the source of truth. Should that happen in the editor package instead so that everything is centralized?
Contributor
There was a problem hiding this comment.
I agree with the idea that ultimately, edit-site (and edit-post) should only pass "non global styles" styles to EditorProvider settings. and the Editor package should be able to read the "global styles" styles directly from the API.
I'd go even further and say even "non global styles" styles should ideally cram from a REST API but we don't really have such thing at the moment.
At least it's worth exploring the first option separately to see if it leads better results.
Member
Author
There was a problem hiding this comment.
💯
My goal was to get things working as I suspect there needs to be a larger piece of work here.
I'm not sure we can read straight from the editor store as it is - there are circular dependencies and max depth loops of death since ExperimentalEditorProvider updates editorStore on every settings change.
Here I went with useSpecificEditorSettings() reading only from editSiteStore (no editorStore subscription) for that reason.
But yeah, editor implementations shouldn't pass global styles to the EditorProvider, which means the EditorProvider should be defensive and exclude them from from syncing, since GlobalStylesRenderer handles these if I'm reading things correctly.
The tricky part is that Pattern previews use BlockPreviews, which use the EditorProvider directly, so GlobalStylesRenderer doesn't run. We could create a BlockPreview wrapper in the editor package that includes GlobalStylesRenderer. I'm guessing that's not all that we'll need.
Contributor
There was a problem hiding this comment.
Don't block previews use the ExperimentalBlockEditorProvider? Unless I'm reading things wrong. That should be independent from any changes we make to Editor?
Member
Author
There was a problem hiding this comment.
Don't block previews use the ExperimentalBlockEditorProvider? Unless I'm reading things wrong.
I was reading it wrong 😄 Thanks for double checking.
So maybe no wrapper required, just the addition of GlobalStylesRenderer here.
tellthemachines
approved these changes
Dec 15, 2025
Contributor
tellthemachines
left a comment
tellthemachines
left a comment
There was a problem hiding this comment.
This is working well for me! Let's get this fix in now and then we can look into something like
ultimately, edit-site (and edit-post) should only pass "non global styles" styles to EditorProvider settings. and the Editor package should be able to read the "global styles" styles directly from the API
without being under pressure to fix an actual regression 😅
Comment on lines
71
to
74
| * Exclude styles and __experimentalFeatures from base settings | ||
| * since we're generating fresh ones from the merged config. | ||
| */ | ||
| const { styles, __experimentalFeatures, ...baseSettings } = settings; |
Contributor
There was a problem hiding this comment.
Do we need to explicitly exclude them? Won't the fresh ones just override the stale?
| ...baseSettings, | ||
| styles: [ | ||
| ...settings.styles, | ||
| ...globalStyles, |
Contributor
There was a problem hiding this comment.
Don't block previews use the ExperimentalBlockEditorProvider? Unless I'm reading things wrong. That should be independent from any changes we make to Editor?
Comment on lines
+46
to
+51
| const [ globalStyles, globalSettings ] = useMemo( () => { | ||
| return generateGlobalStyles( mergedConfig, [], { | ||
| disableRootPadding: false, | ||
| } ); | ||
| }, [ mergedConfig ] ); | ||
|
|
Contributor
There was a problem hiding this comment.
If this is a temp fix pending a larger refactor then probs not worth it.
Member
Author
|
Thanks for testing folks. This PR fixes a pretty gnarly bug but I get that it exposes architectural debt. I agree we can fix the bug and do some planning to address the tech debt. Essentially, edit-site should not generate global styles; GlobalStylesRenderer should be the single source of truth. Follow ups:
I'll see what I can knuckle out... |
Member
Author
I timeboxed an investigation, and I keep coming back to a new store that holds ONLY generated global styles and is accessible by I'll update the inline comments with more info before merging. Edit: I take back a bit of the above. We already have core data, which I guess pertains to the "API" part of this sentence:
|
- Update comments to clarify the direct generation of global styles to avoid circular dependencies. - Ensure synchronization of styles in both useSpecificEditorSettings and usePatternSettings hooks. - Maintain consistency in global styles management across different components.
|
Flaky tests detected in f55a676. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20220481339
|
This was referenced Dec 18, 2025
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
4 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.
closes #73350
What? How?
Selecting a theme style variation in the Site Editor does not persist between the editor view (
canvas=edit) and the main design view (canvas=view). The styles appear correctly in the editor view but remain stale in the main design view.This PR fixes that by generating global styles directly from the merged config in
useSpecificEditorSettings()instead of reading from either store.Why?
A git bisect led me to this PR:
Before PR #72681:
GlobalStylesRendererupdatededitSiteStoreviaupdateSettings()useSpecificEditorSettings()read fromeditSiteStoreviagetSettings()After PR #72681:
GlobalStylesRenderernow updateseditorStoreviaupdateEditorSettings()useSpecificEditorSettings()still reads fromeditSiteStoreviagetSettings()The entity is saved correctly, but the visual representation in the main design view is stale because it's reading from a different store than the one being updated.
Testing Instructions
Screenshots or screencast
Before
Kapture.2025-12-12.at.16.46.12.mp4
After
Kapture.2025-12-12.at.17.34.28.mp4