WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 3 weeks ago

#22030 closed defect (bug) (fixed)

After "clear background-color" in design options, still css is added to html-output

Reported by: Ov3rfly Owned by:
Milestone: 3.9 Priority: low
Severity: normal Version: 3.4
Component: Appearance Keywords: has-patch commit
Focuses: Cc:

Description

After you select "clear background-color" in design options, the default color still is added as inline css to html-output of frontend.

Tested with 3.4.2 and Twenty Twelve 1.0 theme

Steps to reproduce:

  1. Look at default html-output, no inline css for background-color
  2. Design -> Background -> Color, select e.g. color #123456, save
  3. Look at html-output, this is added:
<style type="text/css" id="custom-background-css">
body.custom-background { background-color: #123456; }
</style>
  1. Design -> Background -> Color -> Click "clear" option, save
  2. Look at html-output, this is added with default value:
<style type="text/css" id="custom-background-css">
body.custom-background { background-color: #e6e6e6; }
</style>

Expected behaviour: After "clear" option is used and default value #e6e6e6 (or only a #) is shown in backend, no inline css should be included in html output of frontend.

Attachments (7)

22030.diff (734 bytes) - added by obenland 19 months ago.
22030.1.2.diff (815 bytes) - added by obenland 19 months ago.
22030.2.diff (1.9 KB) - added by obenland 8 months ago.
22030.3.diff (567 bytes) - added by kovshenin 3 months ago.
22030.4.diff (1.9 KB) - added by obenland 3 months ago.
22030.5.diff (1.6 KB) - added by obenland 2 months ago.
22030.6.diff (1.1 KB) - added by obenland 2 months ago.

Download all attachments as: .zip

Change History (41)

comment:1 SergeyBiryukov19 months ago

  • Component changed from General to Appearance

comment:2 DrewAPicture19 months ago

  • Cc xoodrew@… added
  • Keywords close added

I believe this is intentional as #e6e6e6 is the default background color for Twenty Twelve. See #21226.

Suggest close.

comment:3 lancewillett19 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Yes, this is intentional with Twenty Twelve. WP now allows you to set a default background color in the theme functions file. When you "clear the color" in Appearance > Background it should revert back to the default color value.

comment:4 DrewAPicture19 months ago

  • Keywords close removed

Opened #22038 to discuss UX of default background colors.

comment:5 Ov3rfly19 months ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

In a new wordpress install, where no value has been set for custom background-color via backend yet, no inline css is added to front end html, as expected.

After a value for custom background-color has been saved, the inline css appears, as expected.

But (and that's the bug here) it does not disappear again (as it should), if the value is "cleared" via the backend "clear" option.

The currently only way to remove that inline css again, is to delete the entry theme_mods_twentytwelve in database wp_options table.

Expected behaviour: No/empty/cleared setting = no extra inline css.

Note: The default background color comes from style.css, no inline code required. And about the theme functions.php file, if there is no add_theme_support() for custom-background, again no inline code required.

Last edited 19 months ago by Ov3rfly (previous) (diff)

comment:6 DrewAPicture19 months ago

Related: #22038 – Bad UX with default background color

comment:7 SergeyBiryukov19 months ago

  • Milestone set to Awaiting Review

comment:8 toscho19 months ago

  • Cc info@… added

obenland19 months ago

comment:9 obenland19 months ago

  • Keywords has-patch added

obenland19 months ago

comment:10 Ov3rfly19 months ago

Tested 22030.1.2, works exactly as expected. Thanks.

Version 0, edited 19 months ago by Ov3rfly (next)

comment:11 SergeyBiryukov14 months ago

  • Milestone changed from Awaiting Review to 3.6

comment:12 lancewillett13 months ago

  • Keywords commit added

22030.1.2 patch looks good, let's get this in.

comment:13 follow-up: nacin12 months ago

Related code here has a lot of history. See [21054] and all of the tickets it links to. This does not regress any of those issues, correct?

comment:14 follow-up: ocean9010 months ago

Can someone point me to this clear option? I can see only the "default color" button and the "remove background image" button. Not sure which is the "clear" one.

comment:15 in reply to: ↑ 14 DrewAPicture10 months ago

Replying to ocean90:

Can someone point me to this clear option? I can see only the "default color" button and the "remove background image" button. Not sure which is the "clear" one.

I think it refers to the action of "clearing" the background color. See comment:3.

comment:16 in reply to: ↑ 13 obenland10 months ago

Replying to nacin:

This does not regress any of those issues, correct?

It shouldn't and it hasn't in my testing. It removes the theme mod, so we just go back to the status after the initial theme activation.

comment:17 ocean9010 months ago

I mentioned on IRC that we would need the same for the Customizer.

My idea: Save the value, but don't print the inline CSS if it's the default color. That's the way like we do it in twentythirteen_header_style(), twentytwelve_header_style() and twentyeleven_header_style().

comment:18 follow-up: aaroncampbell10 months ago

The patch works good, the only thing is that it seems like specifying a color (even a default one) should result in the CSS output. If you CLEAR the color out and save, then I would expect the CSS to go away. I'd recommend changing the set_theme_mod('background_color', ''); to remove_theme_mod( 'background_color' ); but not checking for the default color first (just stick to the length checks).

comment:19 in reply to: ↑ 18 ocean9010 months ago

Replying to aaroncampbell:

The patch works good.

Yes, but not when you change it via the Customizer. See the linked IRC chat.

comment:20 ocean9010 months ago

  • Keywords commit removed

comment:21 obenland10 months ago

I tried the following approach in _custom_background_cb() to cover both the customizer and the background admin:

<?php

$color = get_theme_mod( 'background_color' );

if ( get_theme_support( 'custom-background', 'default-color' ) == $color )
        $color = false;

if ( ! $background && ! $color )
        return;

For some reason the customizer chokes on that and does not display the default color though. It also breaks themes that trust that the value in the theme mod gets used.

22030.1.2.diff fixes it for the background admin, but I'm really not sure (how) we can fix it for the customizer.

comment:22 nacin9 months ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release
  • Version changed from 3.4.2 to 3.4

The customizer has some JS that mirrors the PHP equivalent. http://core.trac.wordpress.org/browser/trunk/wp-includes/js/customize-preview.js?rev=22798#L105

So the issue here is that we save the background color even when it is the default, rather than removing the mod?

Looks like get_background_color() would work just fine with this. Per the comment in _custom_background_cb() I added in [21002], the desired behavior does appear to be that we don't save the background color when it equals the default. // A default has to be specified in style.css. It will not be printed here. We simply missed that in #20448.

I'm fine with this change, but let's do it early in a cycle because it could potentially trip up themes. And, let's figure out what needs to be done regarding the customizer.

comment:23 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

obenland8 months ago

comment:24 nacin7 months ago

  • Keywords commit added; 3.7-early removed
  • Milestone changed from 3.7 to 3.8

Marking for commit for 3.8.

comment:25 samuelsidler5 months ago

  • Priority changed from normal to low

comment:26 nacin5 months ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

comment:27 obenland3 months ago

  • Keywords 3.9-early removed
  • Milestone changed from Future Release to 3.9

I'm fine with this change, but let's do it early in a cycle because it could potentially trip up themes.

kovshenin3 months ago

comment:28 kovshenin3 months ago

  • Keywords commit removed

With remove_theme_mod() in .2.diff the two scenarios are now inconsistent. The legacy background screen removes the theme mod, while the Customizer saves an empty string.

22030.3.diff is an alternative approach which simply copies what we already do in the Twenties with the header text color.

comment:29 obenland3 months ago

I tried something similar earlier, but the Customizer needs to be able to override the empty value (false) from the theme_mod, with the the theme's default value, for the preview to work. Once we bail on the default color and don't output the CSS, the customizer has no chance to update that color.

I'll add a patch where it removes the theme_mod in the customizer too.

obenland3 months ago

obenland2 months ago

comment:30 obenland2 months ago

22030.5.diff uses the check in _custom_background_cb() and adds Customizer support for this approach.

comment:31 ircbot2 months ago

This ticket was mentioned in IRC in #wordpress-dev by obenland. View the logs.

obenland2 months ago

comment:32 DH-Shredder4 weeks ago

  • Keywords commit added

22030.6.diff looks good to me, and seems to function as intended. Love the simplicity.

comment:33 nacin3 weeks ago

In 27704:

Custom backgrounds: Always cause a preview in the customizer.

Omitted from [27703].

props obenland.
see #22030.

comment:34 nacin3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.