Fix editor colors for themes not loading color palettes #22356
Conversation
youknowriad
added
[Type] Bug
|
Size Change: +142 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
fix/themes-not-loading-palette
branch
from
ae003fd
to
36ddce1
Compare
| ( BlockListBlock ) => ( props ) => { | ||
| const { name, attributes } = props; | ||
| const { backgroundColor, textColor } = attributes; | ||
| const { colors } = useSelect( ( select ) => { |
There was a problem hiding this comment.
I tracked the test failure locally. I don't understand why it happens yet but it seems that commenting/removing the useSelect solves the issue. That's very weird. Any ideas why a useSelect call without impact on the rendered component would break tests here cc @aduth @epiqueras @jorgefilipecosta
There was a problem hiding this comment.
What are the symptoms of the issue? (What do you see?)
Based on what I see here, it's not really clear what issue there should be. Is it fair to say "without impact on the rendered component" if colors is used below in determining which props to assign?
I doubt it has any impact on fixing the issue, but a suggestion I might have here is it could be good to avoid the subscription incurred by useSelect until after the point where it's known whether the early return is reached, by creating a separate component which handles the logic after the early return and contains the hook.
I guess unless we figure that most common blocks will support colors, in which case it's probably not much an optimization.
There was a problem hiding this comment.
I agree with the proposal, it will fix the test but potentially hide an issue because the test fails even if we don't apply any style (the early return)
And I don't the symptoms are basically the test failure: it seems like something related to focus maybe cause the "Third paragraph" is not written.
There was a problem hiding this comment.
It sounds like a race condition with the subscriptions.
There was a problem hiding this comment.
This PR is an important fix, I'm tempted to apply @aduth's fix in order to land it even if it hides the failure.
There was a problem hiding this comment.
It's not a bad idea. That's the recommended way to deal with conditional hooks, regardless of tests.
There was a problem hiding this comment.
Actually, no ideally we don't remount the block when applying a color so we shouldn't bail early here (maybe just for the support check)
There was a problem hiding this comment.
Aren't we already rerendering? For each path, a different instance of BlockListBlock is returned. It should still work otherwise though. Maybe worth looking at after beta 1?
|
Hey, this is a difficult situation 😞 Trying to recap what has happened here, for the record:
Would this be a faithful representation of what happened here? If it is, I wouldn't consider this a Block Editor regression, but a theme bug that was only uncovered recently. Note that this fix is also incomplete: for example, it won't fix the situations where themes want to be smart about things (like setting both the text and background color at the same time) and can introduce subtle errors in that case. TwentyTwenty does this. I reckon we under-communicated the removal of inline styles. Do you think is still there a chance we can handle this by publishing a dev note? I understand this is a nuanced situation and why we feel like we need to cover for themes here. So, if we do this, I'd say that:
|
Judging by the number of impacted themes, I don't think so. That said, this becomes useless when we provide presets styles... (experimental-theme.json) |
|
Did we use inline styles for font sizes as well in the past? If so I agree we should do the same but I'm not sure it was the case. |
👍
Agreed. One of the advantages of a "managed CSS" approach.
I was under the impression we did, but I'll check tomorrow morning. |
|
Yeah, font-sizes did the same. Haven't looked at the code for this PR, but, if this is ready, perhaps it can land and we do font-sizes separately. You want to do that or should I help? I guess that's one that needs to land in the next release for coherence. |
| ...propsB, | ||
| }; | ||
|
|
||
| if ( propsA && propsB && propsA.className && propsB.className ) { |
There was a problem hiding this comment.
How can propsA be spread above if it can me nullish?
|
@nosolosw I'd appreciate some help if possible for the font sizes. I'm not going to merge yet though because it's quite clear that this PR makes the e2e tests more unstable than master for the moment. |
| }; | ||
|
|
||
| return <BlockListBlock { ...props } wrapperProps={ wrapperProps } />; | ||
| } |
There was a problem hiding this comment.
| } | |
| }, | |
| 'withColorPaletteStyles' |
|
@youknowriad I've created the equivalent PR for font sizes at #22668 |
youknowriad
added
Needs Dev Note
youknowriad
force-pushed
the
fix/themes-not-loading-palette
branch
from
99d6a8a
to
84f06b1
Compare
|
What's the status of this PR? |
|
The status is that it's ready but I haven't been able to find a way to make the e2e failure go. I'm having hard time understanding the reason for this failure. |
ellatrix
force-pushed
the
fix/themes-not-loading-palette
branch
from
84f06b1
to
bb0ae85
Compare
| : undefined, | ||
| }; | ||
|
|
||
| let wrapperProps = props.wrapperProps; |
youknowriad
removed
the
Needs Dev Note
closes #21931
While ideally themes should load their palette styles in the editor to match the frontend, this PR restores the inline styles that were previously applied on the editor to avoid regressions.
The downside here is that there's a hook that has a very small but non-negligible impact on performance.
Testing instructions