Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51927 closed defect (bug) (fixed)

Twenty Twenty-One: Buttons lose contrast consistency in Dark Mode

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

Description (last modified by desrosj)

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 4 years ago.
Expected result.
actual-result.png (56.3 KB) - added by allancole 4 years ago.
Actual result

Download all attachments as: .zip

Change History (30)

@allancole
4 years ago

Expected result.

@allancole
4 years ago

Actual result

#1 @poena
4 years 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 4 years ago by poena (previous) (diff)

#2 @allancole
4 years 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.


4 years ago
#3

  • 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

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


4 years ago
#4

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
4 years 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
4 years 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

carolinan commented on PR #806:


4 years ago
#7

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.

carolinan commented on PR #806:


4 years ago
#8

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.

carolinan commented on PR #806:


4 years ago
#9

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.


4 years ago

allancole commented on PR #806:


4 years ago
#11

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 :-)

allancole commented on PR #806:


4 years ago
#12

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
4 years 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.

carolinan commented on PR #806:


4 years ago
#14

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

allancole commented on PR #806:


4 years ago
#15

@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
4 years ago

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

Last edited 4 years ago by poena (previous) (diff)

carolinan commented on PR #806:


4 years ago
#17

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.

carolinan commented on PR #806:


4 years ago
#18

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

carolinan commented on PR #806:


4 years ago
#19

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

allancole commented on PR #806:


4 years ago
#20

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.

allancole commented on PR #806:


4 years ago
#21

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

#22 @poena
4 years ago

#52166 was marked as a duplicate.

#23 @poena
4 years ago

  • Milestone changed from Awaiting Review to 5.7

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


4 years ago

#25 @SergeyBiryukov
4 years 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
4 years 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.

#28 @desrosj
4 years ago

  • Description modified (diff)
  • Summary changed from Twenty Twenty-One: Buttons lose contrast consistency in dark-mode. to Twenty Twenty-One: Buttons lose contrast consistency in Dark Mode
Note: See TracTickets for help on using tickets.