Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default color presets rendering in the Cover block when defaultPalette is false #58753

Closed
richtabor opened this issue Feb 6, 2024 · 11 comments · Fixed by #58951
Closed

Default color presets rendering in the Cover block when defaultPalette is false #58753

richtabor opened this issue Feb 6, 2024 · 11 comments · Fixed by #58951
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@richtabor
Copy link
Member

richtabor commented Feb 6, 2024

Description

Setting settings.color.defaultPalette in theme.json should remove the default color palette presets from the Cover block placeholder (as it does in WordPress 6.4.X).

With Gutenberg active, the default color presets are unexpectedly rendered as choices within the placeholder.

Step-by-step reproduction instructions

  1. Install and activate the Twenty Twenty Four theme.
  2. Add a page.
  3. Add a cover block to the page.
  4. See the core presets within the placeholder.

Screenshots, screen recording, code snippet

In the screenshot below I am showing how the default color presets are disabled for Twenty Twenty Four, as only the theme's color presets are rendering in the Color settings. But the placeholder is not respecting the theme.json, rendering the core presets alongside the theme's:

CleanShot 2024-02-06 at 16 03 48

Environment info

Does not occur with WordPress 6.4.x, but it does when Gutenberg is active.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@richtabor richtabor added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Feb 6, 2024
@colorful-tones
Copy link
Member

Confirming this is an issue. With the Gutenberg plugin deactivated the core default color palette is not showing and it is respecting the TT4 theme's disabling of core colors. I'm going to try and investigate how and where this regression was introduced.
Screenshot 2024-02-07 at 11 23 13 AM

@juanfra
Copy link
Member

juanfra commented Feb 7, 2024

I checked and can confirm as well. It looks like Gutenberg is adding its own defaults. And there's a comment saying that's for backward compatibility purposes.

@richtabor
Copy link
Member Author

These shouldn't be available in the UI if they're disabled from theme.json

@colorful-tones
Copy link
Member

I got pulled away before I could do a deep dive here. I did do a bit of a Gutenberg GitHub search for defaultPalette and found there have been related efforts to fix this issue before. One example: #50115

@youknowriad
Copy link
Contributor

I'm not entirely sure what triggered this regression but could this something you could help debug? @andrewserong @aaroncampbell @ramonjd

@andrewserong
Copy link
Contributor

Sure, happy to help dig in! 👀

@andrewserong
Copy link
Contributor

From a git bisect, I believe the issue was introduced in #55219 which resulted in the color.palette setting including all colors from the default, theme and custom origins. I've put up a tentative fix in #58869 which appears to work, borrowing from some of the logic in the site editor. TL;DR: we try grabbing each of the origins individually, and skip output of the default palette if defaultPalette is not truthy.

I think that seems to work okay, but it does mean the fix is particular to that component. Feel free to close out that PR if anyone comes up with a better approach!

@youknowriad
Copy link
Contributor

@andrewserong That particular PR #55219 seem to be a somewhat big low level change and I don't believe the reasoning there was discussed properly. Any thoughts on it, what does it mean for all the properties. Should we consider a revert to it?

cc @oandregal @matiasbenedetto I'd love more thoughts on that particular PR.

@youknowriad
Copy link
Contributor

I believe I saw a similar issue raised about the defaultFontSizes as well.

@matiasbenedetto
Copy link
Contributor

That particular PR #55219 seem to be a somewhat big low level change and I don't believe the reasoning there was discussed properly. Any thoughts on it, what does it mean for all the properties. Should we consider a revert to it?

@youknowriad I don't think so. I added my the rationale behind my point of view in this comment from #55219. Maybe we can continue the discussion over there.

@andrewserong
Copy link
Contributor

Update: for anyone following along with this issue, there is discussion over in #55219 (comment) about the idea of reverting #55219 (the PR that introduced merging settings origins, which affected the color palette in this issue). @ajlende has a proposal in #58951 to look at reverting. If the PR is reverted, then I don't think we'll need a separate fix for this issue.

If the PR is not reverted, then #58869 will be a candidate for a fix for 6.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
No open projects
Status: Done
6 participants