Twenty Twenty-One: Text color is not reflected when having background color

[wildworks]

Originally reported as a Gutenberg issue: ​https://github.com/WordPress/gutenberg/issues/51828

In #52129, there was a problem with text being obscured in dark mode. The approach taken to solve this was to override the text color with var(--table--has-background-text-color) variable if it had a background color.
Later, however, the table block now supports text color and can be set to any text color.
The text color changed by block support is not applied because it is overwritten by the color of the var(--table--has-background-text-color) variable mentioned above.

[wildworks]

In the striped table, if a background color is set, it is overwritten by the text color of the theme

[shailu25]

Patch added

[harshgajipara]

Test Report for https://core.trac.wordpress.org/attachment/ticket/58608/58608.diff Patch.

Backend:
=======
Before patch: ​https://prnt.sc/Uhz7lE9AgVfK
After patch: ​https://prnt.sc/4qrJkX_3wIVU

Front end:
=========
Before patch: ​https://prnt.sc/CWk7YDCTMN7T
After patch: ​https://prnt.sc/1WuyLxZK8Ky6

https://core.trac.wordpress.org/attachment/ticket/58608/58608.diff Patch is Working fine

[alvitazwar052]

Test Report

Patch tested: https://core.trac.wordpress.org/attachment/ticket/58608/58608.diff

Environment

• OS: macOS 12.4
• Web Server: Nginx
• PHP: 7.4.33
• WordPress: 6.3-alpha-55505-src
• Browser: Chrome
• Theme: Twenty-twenty-one

ScreenCast

Before

• Backend: ​https://prnt.sc/Zqv5GC2lypkA
• Frontend: ​https://prnt.sc/IxPFdeS_PhcE

After

• Backend: ​https://prnt.sc/tAhxLMk4dQH3
• Frontend: ​https://prnt.sc/Li07XVfwDrlp

Actual Results

• The patch is working as expected✅

[karmatosed]

Moving to this release for consideration.

[hmbashar]

Test Report

Patch tested: 58608.diff

Environment

• WordPress: 6.6-alpha-57778-src
• PHP: 8.3.7
• Server: nginx/1.25.4
• Database: mysqli (Server: 8.3.0 / Client: mysqlnd 8.3.7)
• Browser: Chrome 125.0.0.0
• OS: macOS
• Theme: Twenty Twenty-One 2.2
• MU Plugins: None activated
• Plugins:
  • FakerPress 0.6.6
  • Test Reports 1.1.0

Actual Results

1. Resolved an issue with the editor through a patch, but still experiencing a possible frontend problem.

Before Patch

Editor ​https://prnt.sc/TdexpuQN94qC
Frontend ​https://prnt.sc/jU-e5H0HoZ-y

After Patch

Editor ​https://prnt.sc/NDsbrSFVxpfG
Frontend ​https://prnt.sc/nJoXYVUYp52i

[karmatosed]

As this in testing still possibly has issues, let's punt to future release and have that reviewed. Thank you everyone this shows the importance of testing.

[karmatosed]

Assigning to myself to test for possible commit.

[karmatosed]

I also experience a front-end issue still @shailu25 can you check your patch please?

[shailu25]

@karmatosed Thanks for the feedback.

I will update the patch accordingly.

[shailu25]

Patch Refreshed.

[wildworks]

@shailu25 Thank you for submitting a patch.

As I understand it, the style of this theme is managed by Sass, and the CSS files are generated automatically, so we cannot make changes directly to the CSS files. We will need to make changes using the following steps:

• Go to the theme directory: cd src/wp-content/themes/twentytwentyone/
• Install the npm library: npm install
• Run watch mode: npm run start
• Make changes to the file
• Test your changes
• When testing is complete, build the style: npm run build

Once the build is complete, you will see that many files have been updated, not just the sass files you changed, but also ie.css and style.css.map, for example, but these are expected changes.

You will need to update this section to resolve the front-end issue:

​https://github.com/t-hamano/wordpress-develop/blob/a799101e46ef0a042814c189a6331c23a52a100c/src/wp-content/themes/twentytwentyone/assets/sass/05-blocks/table/_style.scss#L32

Additionally, when testing a patch, we will need to make sure it works properly in dark mode as well. Go to "Appearance > Customize > Colors & Dark Mode" and check "Dark Mode support". A button called "Dark Mode: On/Off" should appear in the bottom right of the screen.

[shailu25]

The patch requires additional revisions based on the above comment.

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/8058

[shailu25]

@wildworks I have Created ​New PR as per mentioned in above comment

Thanks for the PR!

This PR works as expected, but I'm wondering why the following style even exists in the first place:

table,
.wp-block-table {
	&.is-style-regular .has-background,
	&.is-style-stripes .has-background,
	&.is-style-stripes .has-background thead tr,
	&.is-style-stripes .has-background tfoot tr,
	&.is-style-stripes .has-background tbody tr {
		color: var(--table--has-background-text-color);
	}
}

Because the following two CSS variables both point to #28303d, the cell's text color will not change regardless of whether the table has a background color or is the Stripes style:

• Cell text color when no background color is applied to the table: var(--global--color-primary) is applied
• Cell text color when a background color is applied to a table: var(--global--color-dark-gray) is applied

My guess is that removing this style entirely would be the ideal approach. Perhaps this style was needed when the Table block didn't support color.

cc @WordPress/block-themers For additional feedback

Twenty Twenty-One is not a block theme.

Please remember that the bundled themes, according to instructions from Matt, must be backwards compatible,
It is not enough to support a version where the block has specific color settings available.

Twenty Twenty-One is not a block theme.
It is not enough to support a version where the block has specific color settings available.

I understand this of course.

This style was added in ​this commit. According to the commit message, this style was added to solve an issue with dark mode. But there is a similar style ​here, so it looks fine as far as I've tested. Will there be any backward compatibility issues?

according to instructions from Matt

What is "instructions from Matt"? Why is Matt's name being mentioned here? I'm a little confused.

I wrote that Twenty Twenty-One is not a block theme because you pinged the _block theme group._

Ideally for us as developers, we would only need to support the latest or two latest versions of WordPress. Then all the bundled themes could be refactored to new standards and a large portion of the CSS could be removed.

• Including CSS that may only have been needed before blocks had specific settings.

I am only putting weight to that decision while explaining why it is not enough to only support one version.

I wrote that Twenty Twenty-One is not a block theme because you pinged the _block theme group._

Sorry, I mistakenly thought this group included all the default theme developers.

Ideally for us as developers, we would only need to support the latest or two latest versions of WordPress.

I strongly agree with this.

I know this is not something we should discuss here, but for example, Twenty Twenty-One supports WP version 5.6 and up. Twenty Ten, surprisingly, supports WordPress version 3.0 and up.

When trying to fix a bug in bundled themes, it seems nearly impossible to test and ensure that it works on all supported WordPress versions.

I have little experience updating bundled themes, but I'm interested to see how you deal with issues like this.

It is difficult, especially when trying to test on versions that require PHP 5.x 😰

[poena]

I wanted to fully review and test the PR, but I am only able to reproduce the reported issue if the striped variation is enabled.

Theme version: 2.4
WP: 6.7.1
Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/8058

If the CSS variable var(--table--has-background-text-color); is removed, it will be a breaking change for sites where the value of the variable has been changed for example by a child theme.
The default color must not be changed, the original design must be kept unless there is no other option.

The primary color and dark grey only has the same value if the dark mode is off and if the body background color has not been changed to a dark color.

But when the body background color is dark enough, the primary color is #fff.
And this color does not have enough color contrast against the light background on the stripe variation.

TLDR: The text color that the user selects must work, but the default must not be changed.

When a color is added, the class name has-text-color is added to the table.
This can be used as part of the selector of a new style to set the correct text color conditionallty.