Fix Pullquote text color after unsetting main color #24600
Conversation
ntsekouras
added
[Type] Bug
|
Size Change: +45 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
Thanks for the PR! I have a question regarding the best practice here, because I see a discrepancy in how 2020 and 2019 themes do this. 2020:
Note how selecting the black background color also sets the cream foreground color, resulting in the bug as reported. 2019:
Note how selecting the background color does not set the foreground color, meaning there is no issue. I tend to think that when you choose one color, that one action should not also affect adjacent controls. Maybe I wanted black text on a black background (homemade spoiler block?) — and we'd also have a color contrast warning indicating that what you're doing is a bad idea. However by making a choice for the user, it feels both unnecessarily prescriptive, but also prone to adding a lot of complexity where none should be needed. The 2019 approach seems much better: when a blue background is set, the theme defines the foreground color to be white. But the color remains unset in the editor, and a user can then explicitly choose a different than white foreground color if that's what they want. The 2020 approach, to set an inline color style when the pullquote has a black background, feels like the bug to fix at the theme level, and if it's using block editor APIs, in the block editor as well. What do you think, @kjellr? |
|
Hey @jasmussen ! Thanks for looking at this! The change of text color has to do with themes' There is currently logic in there that if the user have If the user has chosen a text color it remains as is. My change now is specific to the case that we (GB) had previously computed a text color and now we unset it. So it's like taking back our recommendation/change. |
Actually the text color changes to black when selecting white background, but it isn't selected. If you unset the white background you'll see that the color remains to black. |
|
Thank you for the context.
I guess what I'm suggesting is that this may be undesirable behavior (prescriptive, unpredictable), and that better default theme behaviors (the theme providing white text if the background is set to blue) feels like the better overall practice, when combined with the contrast checker. |
I agree for now — if a user makes one color choice, and then clears it, they shouldn't have to clear a second setting. That said, the practice of themes changing the text color css in circumstances like this has always felt a little bit of a hack though. I think there's a ticket somewhere for handling this via |
|
Thanks for looking, good thoughts. I think I'll dare a ping to @nosolosw to see if he has any thoughts here. |
There was a problem hiding this comment.
👋 I think this can go in, the code fixes the issue. 🚢
There are a few related layers to this that I'd like to comment on! Let me unbundle them:
-
This behavior is exclusive of the pullquote block and is the same in every theme => we need to fix it, and this PR does ✔️
-
The pullquote doesn't use the new implicit style attributes => we need to migrate it, see Consolidate the style attributes of blocks #22700 @ntsekouras is this something you're interested in helping with?
-
Do we want this behavior (changing background also changes text)?
If we look at the different approaches that people have tried (pullquote code, but also in theme territory) this looks like something people want. There's this issue to look at this from the context of theme.json #24166 Perhaps a good first step and a follow-up to this PR would be to look into migrating the pullquote block to the implicit style attribute system.
I do have questions about the expectations about this, though: is the existing pullquote a good enough approach (selecting a text color whose lightness is opposite from the background's)? or should be the theme responsibility to provide some kind of color pairing, so to speak?
|
Thanks for looking @nosolosw!
Yes for sure :) !
Great questions! Maybe both are needed and color pairing/grouping ( not necessarily 1:1 ) by themes might be a nice option. |
Description
Fixes: #24440
Currently in a
Solid colorPullquoteif you change the main color and a text color is automatically computed ( for example main color black ), and then you unset the main color, the text color remains set and that can lead toinvisibletext (same color with background).In this PR if we unset the main color and the text color was previously automatically computed, we unset the text color as well.
Checklist: