Fix: setting a preset color on pullquote default style makes the block invalid

[jorgefilipecosta]

Fixes: #16429

The color mechanism on pull quotes is complex.
On the default style, the main color sets a border while on the solid color style, it sets a background.
The color mechanism uses a class for preset colors and does not set the color values on the attribute, but for borders, we don't have classes.
Previously we used the color mechanism normally for both styles and on the save function given that for presets colors, we don't have the color value as an attribute we queried the block-editor store settings to retrieve the color value and used the value in a border style.
The implementation is not correct, and it was a wrong decision because the save function becomes impure. Although not correct, It was working without block validation issues in the current WordPress version.
Meanwhile, other changes made this existing problem noticeable, and each time we set a preset color on the pullquote default style, the block becomes invalid.

This PR addresses the problem by on the default style even for preset colors setting the customMainColor attribute, instead of using the default color setting mechanism. This allows the color value to be directly available on the save function, so the function can be pure and depends on its attributes only.

How has this been tested?

I added a pull-quote block; I wrote some text, and I choose a preset color.
I added another pull-quote block; I wrote some text; I changed the block style to the solid color; I chose a preset color and verified it the editor applied it as a background; I changed the block style back to the default.
I saved the post with the two blocks and reloaded the editor and verified the blocks continue to be valid (on master they are not).

[retrofox]

Hi @jorgefilipecosta, thanks for dealing with these issues. It seems to be pretty reasonable to make a new iteration to this implementation but wondering since we are going to make a refactoring whether it worths to take a look at the useColors() hook in order to avoid duplicating efforts #16781

[jorgefilipecosta]

Hi @jorgefilipecosta, thanks for dealing with these issues. It seems to be pretty reasonable to make a new iteration to this implementation but wondering since we are going to make a refactoring whether it worths to take a look at the useColors() hook in order to avoid duplicating efforts #16781

Hi @retrofox, this PR will be cherry-picked into WordPress 5.3, so we should try to keep it as simple as possible. The useColors hook is experimental and not part of WordPress 5.3, so I think we can not use it.

[retrofox]

I haven't taken a look at the code yet but did to its functionality. Questions:

front-end env
Why does it apply the background color by inline styles when it's defined through a color palette? It seems that we are going to move to prefer the use of CSS classes instead of inline styles when it's possible.

#17832 (comment)

Although the colors are being applied to the wp-block-pullquote element, there is a stronger rule that overrides what the PullQuote block applies, at least with my testing theme.

Maybe we could add border-color: inherit to the <blockquote /> element ?

[jorgefilipecosta]

Hi @retrofox,

Why does it apply the background color by inline styles when it's defined through a color palette? It seems that we are going to move to prefer the use of CSS classes instead of inline styles when it's possible.

In this PR, we don't apply background colors by inline styles when they are defined through a color palette. We apply border colors with inline styles even if they are defined through a palette, and that was already the case. We do it because themes don't provide classes with border styles. For backgrounds, we use classes because themes provide background classes.

Although the colors are being applied to the wp-block-pullquote element, there is a stronger rule that overrides what the PullQuote block applies, at least with my testing theme.
Maybe we could add border-color: inherit to the

element ?

It seems its a collision with themes I guess we can try to improve the styles. But doing that, we may cause problems in other themes that currently work well. And this problem already happened on previous WordPress releases. Given that we are already late on the RC stage, I would prefer to try in a follow-up PR, not part of the release, and on this PR focus on the immediate issue.

[retrofox]

we don't apply background colors by inline styles

Got confused, I wanted to say to border-color. 🤦‍♂

[retrofox]

It seems its a collision with themes I guess we can try to improve the styles. But doing that, we may cause problems in other themes that currently work well. And this problem already happened on previous WordPress releases. Given that we are already late on the RC stage, I would prefer to try in a follow-up PR, not part of the release, and on this PR focus on the immediate issue.

Sorry, what I'm trying to say is that, if the user sets explicitly the border-color of the pull-quote, should not we try to apply these styles in the front-end without making collisions?

I've set the border color and the text color in the quote:

I expect to see these changes in the front-end. We're facing a similar case in the <NavigationMenu />, where the user is able to set up text and background-color of the menu.
The styles are being applied through either with CSS classes as well as inline styles, but also applying other rules if it's needed, conditionally to the has-text-color or/and has-background-color class/es.

Anyway, I've tested the PR and it works as (almost ;-)) expected. It's up to you pay attention to these comments. Maybe it worths to know the opinion of other folks. And finally, we can keep improving it later.

[youknowriad]

How do currently invalid blocks work with this change? I mean what will happen when they open the editor, will the pull quote be updated, will the color be discarded?

[retrofox]

Just in case, there is a bug in prod right now when applying colors in the Pullquote:

1. Create a PR block
2. Set colors
3. Hard refresh
   

[jorgefilipecosta]

Hi @retrofox, @youknowriad, thank you for your questions/insights.

How do currently invalid blocks work with this change? I mean what will happen when they open the editor, will the pull quote be updated, will the color be discarded?

Hi @youknowriad, I found a solution to keep the named colors and migrate them to custom colors, so for the user, these changes should be transparent. The block should continue to be exactly as before.

Just in case, there is a bug in prod right now when applying colors in the Pullquote:

Hi @retrofox, this pull request is supposed to solve that bug. Are you able to replicate the problem in this branch, or you verified this PR solved the bug?

Sorry, what I'm trying to say is that, if the user sets explicitly the border-color of the pull-quote, should not we try to apply these styles in the front-end without making collisions?

The problem is that what we make to avoid collisions in one theme can have collisions in another theme. I think we can try to improve the styles to minimize common collisions in existing themes. But, I think it is not something we should try in this stage of the release.

[retrofox]

But, I think it is not something we should try in this stage of the release.

Fair enough.

[jorgefilipecosta]

Merging this PR as it is a priority and the more testing it gets, the better. If anyone has any other suggestions, I will happily open a follow-up PR.