Global Styles: Refactor client side style states to use nodes#78000
Merged
Conversation
talldan
added
[Type] Code Quality
|
Flaky tests detected in 93aafec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25538238225
|
|
Size Change: +25 B (0%) Total Size: 7.92 MB 📦 View Changed
ℹ️ View Unchanged
|
talldan
requested review from
aaronrobertshaw,
andrewserong,
ramonjd and
tellthemachines
|
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. |
tellthemachines
approved these changes
May 7, 2026
Contributor
tellthemachines
left a comment
tellthemachines
left a comment
There was a problem hiding this comment.
LGTM as a refactor! And good to have similar logic in PHP and TS.
Everything works as expected in testing too.
Also reduces the number of lines of css output (styles are grouped under @media rules more).
Whereabouts do you see this? I checked a number of blocks and couldn't find a difference between this branch and trunk in the rules output.
| * | ||
| * - `styles`: theme.json style object for this node. | ||
| * - `selector`: CSS selector used for the node's base declarations. | ||
| * - `selectorSuffix`: optional suffix appended to base and feature selectors. |
Contributor
There was a problem hiding this comment.
Is selectorSuffix only used to add pseudo-selectors to a selector? It seems to from the logic below. If so maybe that could be more explicit in the description. Block selectors are so confusing already 😅
| @@ -1361,6 +1273,7 @@ export const getNodesWithStyles = ( | |||
| ); | |||
| } ) | |||
| .join( ',' ), | |||
Contributor
There was a problem hiding this comment.
I just noticed that we're not outputting responsive styles for elements defined in blocks, e.g. a heading element inside a Group. They work on the front end but not in the editor, so I think we're missing something over here. Probably best left for a follow-up PR though.
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
I'm trying to get my head around where the PHP and JS processing here is intended to diverge and where it isn't. For the focus of this PR it seems pretty close though.
If I'm understanding the flow correctly, the JS side differs in the node ordering.
The PHP processing results in something like:
:root :where(.block) {
color: black;
}
@media (width <= 480px) {
:root :where(.block) {
color: red;
}
}
:root :where(.block:hover) {
color: blue;
}
@media (width <= 480px) {
:root :where(.block:hover) {
color: orange;
}
}The JS side expands responsive states within renderStylesNode(), so the resulting order is more like:
:root :where(.block) {
color: black;
}
:root :where(.block:hover) {
color: blue;
}
@media (width <= 480px) {
:root :where(.block) {
color: red;
}
:root :where(.block:hover) {
color: orange;
}
}That seems like it could produce different cascade behaviour because of levelling the specificity of the selector inside it. For example, if there is no explicit mobile hover color, JS could end up with this:
:root :where(.block:hover) {
color: blue;
}
@media (width <= 480px) {
:root :where(.block) {
color: red;
}
}On mobile hover, both selectors have the same specificity from :root, so the later mobile base rule wins and the hovered block is red. In the PHP ordering, the mobile base rule comes before the default hover rule, so the hovered block would stay blue unless mobile.:hover explicitly overrides it.
Does that make sense?
Contributor
Author
I think before it was adding a media query around every feature selector: gutenberg/packages/global-styles-engine/src/core/render.tsx Lines 1081 to 1082 in 2cf7cbd Whereas now it wraps all the node styles in a single media query.
Good catch, let's fix that! I think it should match the PHP logic, but happy to debate that. I'll add a test for it too. |
Contributor
Author
|
Ok, I think I address the ordering, but it required some more moving stuff around. Tried to keep it closer to the PHP logic though. |
Contributor
Oh well spotted! I can confirm with the latest change the default hover styles work when a non-hover mobile style is set. |
ramonjd
approved these changes
May 8, 2026
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
I just mostly manually tested. Working well for me.
I tested:
- block states in global styles
- block style variations states (checked h1, group, image)
- overriding individual styles at the block level
I ran into something that I think only affects the previews - when I was working with style variations the preview doesn't show the default state styles:
Kapture.2026-05-08.at.12.00.29.mp4
Looks fine in the editor and frontend though, so no blockers here.
Contributor
Author
Yep, I think it's because the preview is at tablet dimensions so it always picks up tablet styles. (edit: added this as a thing to fix on #77817) Thanks for the reviews! |
Member
Oh good point!That went over my head. 😄 🚢 |
talldan
force-pushed
the
refactor/client-side-states-to-nodes
branch
from
7b87629 to
93aafec
Compare
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.
What?
This is a TS equivalent of #77881. It refactors state style rendering (for both pseudo and responsive states) to prefer using nodes instead of generating the styles feature by feature.
Why?
renderStylesNodebecomes the central place to change style rendering logic, it should be a bit less error prone for future changes.Also reduces the number of lines of css output (styles are grouped under
@mediarules more).How
renderStylesNodefunction fromtransformToStylesthat can be used for outputting individual node styles.appendResponsiveStylesandappendPseudoSelectorStyles, replacing them withgetResponsiveStyleNodes/getPseudoStyleNodes+renderStylesNodeTesting Instructions
It should behave the same as in trunk. Some extra unit tests are added to ensure the output is consistent.