WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 6 weeks ago

Last modified 5 weeks ago

#51927 closed defect (bug) (fixed)

Twenty Twenty-One: Buttons lose contrast consistency in dark-mode.

Reported by: allancole Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.6
Component: Bundled Theme Keywords: 2nd-opinion has-patch
Focuses: css Cc:

Description

While digging into another issue I discovered a possible flaw that suggests we may need to change the theme’s default palette colors when dark-mode is turned on. The color system in TT1 gets confused in dark-mode when it tries to deal with pastel background-colors and nested blocks that need special contrast to retain a11y.

This CodePen lays out whats happening and some options for how to correct it:
https://codepen.io/allancole/pen/wvzKYWq

I’m pretty sure TT1 has some fall backs that catch some this already. But we’re doing it with block-by-block overrides which make it harder to comprehend and difficult to unravel and fix.

To test this,
1) Create a group block using the pastel-yellow palette colors as the background.
2) Nest a paragraph block and a button block inside of that group.
3) Publish.
4) Set the theme to dark mode and visit the frontend.

See screenshots to se expected and actual results.

I have two possible suggestions to solve this.

Solution 1: Reverse the brightness for palette colors in dark-mode.

This requires adding the theme’s pastel and dark color variables to our style-dark-mode.css and including some new dark-mode specific pastel colors that are effectively reversed. the downside here is that if we change the theme’s palette colors, we might also might need to update the palette colors in the customizer and the block editor which is especially difficult and quite a big change.

Solution 2: Revise our utility classes to account for the dark-mode class.

This would require us to revisit the color-palette.scss utility classes and include a new rule for dark-mode to make the text appear the right color.

Example:

.has-green-background-color[class] {
	background-color: var(--global--color-green);
}

.is-darkmode .has-black-background-color[class]:not(.has-text-color) {
	color: var(--global--color-dark-gray); /* Make text dark when in dark mode */
}

I’m still exploring the two options, but so far the second one seems the easiest to do with the least amount of refactoring and friction.

I’m curious to hear other’s thoughts on this. Maybe there’s some other way to solve this?

Attachments (2)

expected-result.png (59.6 KB) - added by allancole 3 months ago.
Expected result.
actual-result.png (56.3 KB) - added by allancole 3 months ago.
Actual result

Download all attachments as: .zip

Change History (29)

@allancole
3 months ago

Expected result.

@allancole
3 months ago

Actual result

#1 @poena
3 months ago

These are not new but well known and thorougly tested issues that we were not able to solve in time.

The second solution has been tested but the pull request was not accepted.

-It does not only need to check for custom text colors for buttons but also for custom background, and the colors must work for both hover and focus styles for both filled and outlined buttons.

The complexity of this is why I explored alternative button designs.

Last edited 3 months ago by poena (previous) (diff)

#2 @allancole
3 months ago

The second solution has been tested but the pull request was not accepted.

Ahh, didn’t realize that. I’ll try to dig up the old PR to see what problems arose with that approach.

-It does not only need to check for custom text colors but also for custom background, and the > colors must work for both hover and focus styles for both filled and outlined buttons.

Gotcha. I may spend some time trying to build those additional factors into a fork of the CodePen above. It’s definitely a complexity issue, but if I can get it to work on a smaller scale, I’m certain we can get it to work in the theme itself :-)

This ticket was mentioned in PR #806 on WordPress/wordpress-develop by allancole.


3 months ago

  • Keywords has-patch added; needs-patch removed

This change restructures the Button element styles for better a11y and expected color behaviors. Here what has changed:

  • The button-style() mixin is now the _single source or truth_ for how Button should look site-wide.
    • Button Block, File Block, and Search form Blocks all rely on this one mixin.
    • The same styles is also applied to the <button> element which appears in widgets, the 404 search form and comments form.
  • Improves expected button styles for various conditions and contexts as follows:
    • User color palette selections for Buttons are retained regardless of a parent block’s color settings or dark-mode.
    • Supports both Default/Filled styles and Outline styles.
    • More consistent :hover and :active styles.
    • :focus styles are now always visible and legible.
  • Adds a --local-color color variable to scope color relationships to nested blocks.
  • Reduces selectors in style.css output.
  • 1:1 experience between editor and front end button styles.
  • Properly supports Dark-mode.

Trac ticket: https://core.trac.wordpress.org/ticket/51927

#4 @prbot
3 months ago

github-actions[bot] commented on PR #806:

Hi @allancole! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

#5 @poena
3 months ago

@allancole I believe I have applied the PR correctly for testing but I am not getting an identical result in the editor and the front:

Editor
https://imgur.com/X7Ra2DU

Front:
https://imgur.com/7roCb2w

#6 @kjellr
3 months ago

Yeah, I'm seeing some mismatches too. Using the test data from the original GitHub issue, these are the inconsistencies I see against the default background:

Editor (Hover states)
https://cldup.com/BPb0pmwUqt-3000x3000.png
https://cldup.com/Yv0WC5aJkr-2000x2000.png
https://cldup.com/vl0O8kVyJS-3000x3000.png
https://cldup.com/JCzIWsWJQD-2000x2000.png
https://cldup.com/B_jjmTcScQ-2000x2000.png

Frontend (Hover states)
https://cldup.com/T43X3mhSoD-3000x3000.png
https://cldup.com/cLZfbHfj7z-1200x1200.png
https://cldup.com/ZHK4meQOzP-1200x1200.png
https://cldup.com/Gs6YK-JdFp-3000x3000.png
https://cldup.com/lAk5qvr5nS-3000x3000.png

#7 @prbot
3 months ago

carolinan commented on PR #806:

Filled buttons with custom text color
Front: The custom text color is applied to the buttons border and text on hover.
Editor: The custom text color is not applied to the buttons border and text on hover. The primary color is showing.

The result is the same both with and without dark mode enabled.

Filled buttons with custom palette color
The result is the same both with and without dark mode enabled, in the editor and front.
The custom text color is not displaying on hover.
This is not consistent with buttons where a custom text color is used.

First, we need to decide what the expected and wanted result is, then we can adjust it.

#8 @prbot
3 months ago

carolinan commented on PR #806:

Outlined buttons with custom text color
The custom text color is showing on hover both in the editor and front. This is as expected, but inconsistent with the result for the filled buttons.

Outlined buttons with palette text color
Front: The palette color is not applied to the text on hover.
Editor: The palette color is applied to the text on hover.

#9 @prbot
2 months ago

carolinan commented on PR #806:

As discussed on Slack, lets revert to showing the default colors for hover and focus states even if a color is selected (custom or palette).

Is it possible to have this ready for a 1.1 release on tuesday?

This ticket was mentioned in Slack in #core-themes by poena. View the logs.


2 months ago

#11 @prbot
2 months ago

allancole commented on PR #806:

As discussed on Slack, lets revert to showing the default colors for hover and focus states even if a color is selected (custom or palette).

Will do.

Is it possible to have this ready for a 1.1 release on tuesday?

Yup, I can have all of that sorted out today :-)

#12 @prbot
2 months ago

allancole commented on PR #806:

I think this PR should be good to go now @carolinan :-) . The PHPCS error is coming from something in the _upstream_ branch, not related to this PR.

#13 @poena
2 months ago

  • Focuses css added
  • Summary changed from Twenty Twenty One: Theme color palettes lose contrast consistency in dark-mode. to Twenty Twenty-One: Buttons lose contrast consistency in dark-mode.

#14 @prbot
2 months ago

carolinan commented on PR #806:

Tested on Chrome on Windows 10. Editor and front, without Gutenberg active.
Default body background, all palette colors as body background, dark mode.


Filled and outlined button block:
Default colors
Custom text color,
Custom background color,
Custom background color and custom text color
Palette text color,
Palette background color,
Palette background color and palette text color
Palette background color and custom text color
Custom background color and palette text color

Notes:
Confirmed that buttons are readable when placed inside group blocks that has palette background colors.
The focus style color is different depending on if the button has a border radius or not.
Confirmed that the button text is visible when the button is selected to be edited.


Password form button
Comment form button
Search form button

No issues.


Search block, all settings tested.

Under the following conditions:

  1. Dark Mode is enabled
  2. The search block has the "button inside" setting
  3. The block is placed inside a group block with a light palette background color

The border for the div around the input field wp-block-search__inside-wrapper has the wrong color on the front.
On the front, the color is light grey, but in the editor it is the dark grey color var(--form--color-text);

Front:
https://i0.wp.com/user-images.githubusercontent.com/7422055/102604804-05b0cb80-4125-11eb-9876-c26f7c19e6ea.png

Editor:
https://i0.wp.com/user-images.githubusercontent.com/7422055/102605147-26792100-4125-11eb-9461-23857dabfb03.png

#15 @prbot
2 months ago

allancole commented on PR #806:

@carolinan, I think this is good to go now. I fixed the issue with the inside button search form border.

The focus style color is different depending on if the button has a border radius or not.

Yeah, i’m seeing this too:

Normal :focus style | Round :focus style


https://i0.wp.com/user-images.githubusercontent.com/709581/102658757-35bc9680-4146-11eb-88d8-918211862f62.pnghttps://i0.wp.com/user-images.githubusercontent.com/709581/102658735-289fa780-4146-11eb-9e7c-b16d4db8b195.png|https://i0.wp.com/user-images.githubusercontent.com/709581/102658702-14f44100-4146-11eb-8a77-c60146911954.pnghttps://i0.wp.com/user-images.githubusercontent.com/709581/102658680-060d8e80-4146-11eb-8468-b694ce031417.png

I’m not sure what we can do about it without also re-designing the focus styles for _all_ the buttons. I’m sure this came up before, but not sure where to find the discussion.

I think only box-shadow: inset or text-decoration: underline are the only other viable :focus styles that would be consistent for all buttons. But I sense it’s probably too late to do a redesign on buttons at this point. With that considered, I think what we have already is fine a11y-wise in my opinion, so Im okay leaving it as it is despite the rounded buttons looking different on :focus.

#16 @poena
2 months ago

Yes we can leave the focus for the rounded buttons as it is.
The search blocks has the correct colors now.

Last edited 2 months ago by poena (previous) (diff)

#17 @prbot
2 months ago

carolinan commented on PR #806:

There is something wrong with the CSS for IE11.

The search block button text and icon color is not visible when the block is placed inside a block with a background color.
The text/icon is only visible on hover, not in the default state or focus.

#18 @prbot
2 months ago

carolinan commented on PR #806:

If postcss-discard-duplicates works, would you prefer to include this in Twenty Twenty-One version 1.1, despite the IE11 problems?

#19 @prbot
2 months ago

carolinan commented on PR #806:

If postcss-discard-duplicates works, would you prefer to include this in Twenty Twenty-One version 1.1, despite the IE11 problems?

#20 @prbot
2 months ago

allancole commented on PR #806:

If postcss-discard-duplicates works, would you prefer to include this in Twenty Twenty-One version 1.1, despite the IE11 problems?

@carolinan Yes. If we run postcss-discard-duplicates on ie.css and ie-editor.css, then this PR will be ready for merge.

Solving the IE11 problems in a separate PR/diff makes sense to me.

#21 @prbot
2 months ago

allancole commented on PR #806:

Just noting that this PR is related and may rely on #835 which tries to solve the duplicate CSS in ie.css.

#22 @poena
6 weeks ago

#52166 was marked as a duplicate.

#23 @poena
6 weeks ago

  • Milestone changed from Awaiting Review to 5.7

This ticket was mentioned in Slack in #core-themes by poena. View the logs.


6 weeks ago

#25 @SergeyBiryukov
6 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 49987:

Twenty Twenty-One: Clean up Button styles.

This change restructures the Button element styles for better a11y and expected color behaviors. Here what has changed:

  • The button-style() mixin is now the single source or truth for how Button should look site-wide.
    • Button Block, File Block, and Search form Blocks all rely on this one mixin.
    • The same styles is also applied to the <button> element which appears in widgets, the 404 search form and comments form.
  • Improves expected button styles for various conditions and contexts as follows:
    • User color palette selections for Buttons are retained regardless of a parent block’s color settings or dark-mode.
    • Supports both Default/Filled styles and Outline styles.
    • More consistent :hover and :active styles.
    • :focus styles are now always visible and legible.
  • Adds a --local-color color variable to scope color relationships to nested blocks.
  • Reduces selectors in style.css output.
  • 1:1 experience between editor and front end button styles.
  • Properly supports Dark-mode.

Props allancole, poena, scruffian, megphillips91.
Fixes #51927.

#27 @SergeyBiryukov
5 weeks ago

In 50026:

Twenty Twenty-One: Correct colors for the Menu button.

As per design, the background should be transparent and the text should be dark grey, not vice versa.

Follow-up to [49987].

Props poena.
Fixes #52374. See #51927.

Note: See TracTickets for help on using tickets.