WordPress.org
Get WordPress

Make WordPress Core

Opened 6 years ago

Closed 18 months ago

Last modified 18 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 5 years ago.
45916.2.diff​ (2.2 KB) - added by ronakganatra 18 months ago.

Download all attachments as: .zip

Change History (26)

#1 @samful
5 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
5 years ago

#2 @sabernhardt
19 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
18 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
18 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
18 months ago

#5 @oglekler
18 months ago

  • Keywords needs-testing added

#6 @tb1909
18 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 18 months ago by tb1909 (previous) (diff)

#7 @ugyensupport
18 months ago

Assurance with

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
Version 1, edited 18 months ago by ugyensupport (previous) (next) (diff)

#8 @ronakganatra
18 months ago

Thank you @ugyensupport , @tb1909 for testing.

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

#10 @costdev
18 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
18 months ago

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

Self assigning for commit.

#12 @audrasjb
18 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
18 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
18 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
18 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
18 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.
18 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.
18 months ago

This ticket was mentioned in ​PR #4714 on ​WordPress/wordpress-develop by ​@sabernhardt.
18 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
18 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.
18 months ago

#22 @audrasjb
18 months ago

Thanks @sabernhardt!

#23 @audrasjb
18 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.