WordPress.org
Get WordPress

Make WordPress Core

Opened 5 years ago

Closed 3 months ago

Last modified 3 months ago

#45916 closed enhancement (fixed)

Twenty Nineteen: Consider setting background color and foreground color together

Reported by: allancole's profile allancole Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: good-first-issue good-first-bug has-patch needs-testing
Focuses: css Cc:

Description

Originally reported by ​@joyously in the Twenty Nineteen GitHub repo:

On the <body> tag, as a best practice the background-color and text color should always be set together in the main stylesheet. This will make it easier to quickly understand the underlying color scheme.

See here: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentynineteen/style.css#L765

Original ticket here: ​https://github.com/WordPress/twentynineteen/issues/64

Attachments (2)

45916.diff​ (2.1 KB) - added by samful 3 years ago.
45916.2.diff​ (2.2 KB) - added by ronakganatra 3 months ago.

Download all attachments as: .zip

Change History (26)

#1 @samful
3 years ago

  • Focuses css added
  • Keywords good-first-bug has-patch added; needs-patch removed

This ticket is from 2018 on github, I submitted a patch to push this along and changed keywords. If this patch is accepted, then the 2017 theme also has the same issues and needs to be patched in the same way. (The 2020 theme does set these together btw)

@samful
3 years ago

#2 @sabernhardt
5 months ago

  • Milestone changed from Awaiting Review to 6.3

The patch still applies. We could also consider moving the background-color to _typography.scss so the two body colors are defined earlier in the stylesheet and before the text color of form elements (button, input, select, optgroup, textarea).

#3 @oglekler
4 months ago

  • Keywords changes-requested added

Hi @samful,
can you please make a bit of an improvement, which is mentioned in #comment:2? If you do not have the opportunity, please write if for another developer can make it further.

Thank you 🙏

#4 @ronakganatra
3 months ago

  • Keywords changes-requested removed

Hello @oglekler , I have applied the #comment:2 suggestion in below patch, let me know your thoughts.

Thanks

@ronakganatra
3 months ago

#5 @oglekler
3 months ago

  • Keywords needs-testing added

#6 @tb1909
3 months ago

Test Report

This report validates that the indicated patch addresses the issue.

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

Environment

  • OS: macOS 13.4
  • Web Server: Nginx
  • PHP: 8.1.13
  • WordPress: 6.3-alpha-55505-src
  • Browser: Google Chrome 114.0.5735.133
  • Theme: Twenty Nineteen
  • Active Plugins:

none

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

  • Tested the patch. Result as described in #comment:2
  • Declaration of color and background property together with same styling result
  • Working on subpages / homepage / single posts as expected
Last edited 3 months ago by tb1909 (previous) (diff)

#7 @ugyensupport
3 months ago

Description

Twenty-Nineteen: Consider setting the background color and foreground color together

Environment

  • WordPress: 6.2.2
  • PHP: 8.0.0
  • Server: Apache/2.4.10 (Debian)
  • Database: mysqli (Server: 5.5.59-MariaDB-1~wheezy / Client: 5.5.62)
  • Browser: Chrome 114.0.0.0 (macOS)
  • Theme: Twenty-Nineteen
  • Plugins:
    • WordPress Beta Tester 3.4.1

Steps to Reproduce

  1. Assurance with @ronakganatra Patch https://core.trac.wordpress.org/attachment/ticket/45916/45916.2.diff

Expected Results

  1. ✅ Issue resolved with a patch.

Actual Results

  1. Tested the patch. Result as described in #comment:2
  2. Declaration of color and background property together with the same styling result
  3. Working on subpages/homepage / single posts as expected
Last edited 3 months ago by ugyensupport (previous) (diff)

#8 @ronakganatra
3 months ago

Thank you @ugyensupport , @tb1909 for testing.

This ticket was mentioned in ​Slack in #core-test by mai21. ​View the logs.
3 months ago

#10 @costdev
3 months ago

  • Keywords commit added; needs-testing removed

We have two test reports verifying the patch with no regressions found.

Adding for commit consideration.

#11 @audrasjb
3 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

Self assigning for commit.

#12 @audrasjb
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55960:

Twenty Nineteen: Always set background color and foreground color together.

On the <body> element, as a best practice background color and text color should always be set together. This makes it easier to quickly understand the
underlying color scheme.

Props allancole, joyously, samful, sabernhardt, oglekler, ronakganatra, tb1909, ugyensupport.
Fixes #45916.

#13 @audrasjb
3 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as it looks like this changeset broke the Test Default Themes & Create ZIPs workflow.

#14 @audrasjb
3 months ago

In 55964:

Twenty Nineteen: Revert [55960].

This reverts [55960] pending further investigation, since this changeset caused failures on the Test Default Themes & Create ZIPs workflow.

Unprops audrasjb.
See #45916.

#15 @audrasjb
3 months ago

  • Keywords changes-requested needs-patch added; has-patch removed

I reverted [55960], pending a new patch (and I strongly recommend using a PR next time, to make sure the Test Default Themes & Create ZIPs workflow is passing.

#16 @kebbet
3 months ago

Should the *.css files be left untouched when adding changes? Will they be built during zip-creation process?

This ticket was mentioned in ​PR #4705 on ​WordPress/wordpress-develop by ​@audrasjb.
3 months ago #17

  • Keywords has-patch added; needs-patch removed

https://core.trac.wordpress.org/ticket/45916

This ticket was mentioned in ​Slack in #core by audrasjb. ​View the logs.
3 months ago

This ticket was mentioned in ​PR #4714 on ​WordPress/wordpress-develop by ​@sabernhardt.
3 months ago #19

The background color is set earlier, making the second body selector unnecessary.

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

#20 @sabernhardt
3 months ago

  • Keywords needs-testing added; changes-requested removed

The SCSS from [55960] set both color and background color for the body element in two places, but the compiled CSS files did not match that. I made another PR that groups the color settings with other CSS properties, and a new test report would help.

This ticket was mentioned in ​Slack in #core by mukeshpanchal27. ​View the logs.
3 months ago

#22 @audrasjb
3 months ago

Thanks @sabernhardt!

#23 @audrasjb
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 56062:

Twenty Nineteen: Always set background color and foreground color together.

On the <body> element, as a best practice background color and text color should always be set together. This makes it easier to quickly understand the
underlying color scheme.

Follow-up to [55960], [55964].

Props sabernhardt, audrasjb, kebbet.
Fixes #45916.

Note: See TracTickets for help on using tickets.