WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 16 months 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: Customize 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 3 years ago.
22030.1.2.diff (815 bytes) - added by obenland 3 years ago.
22030.2.diff (1.9 KB) - added by obenland 2 years ago.
22030.3.diff (567 bytes) - added by kovshenin 19 months ago.
22030.4.diff (1.9 KB) - added by obenland 18 months ago.
22030.5.diff (1.6 KB) - added by obenland 18 months ago.
22030.6.diff (1.1 KB) - added by obenland 18 months ago.

Download all attachments as: .zip

Change History (41)

comment:1 @SergeyBiryukov3 years ago

  • Component changed from General to Appearance

comment:2 @DrewAPicture3 years 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 @lancewillett3 years 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 @DrewAPicture3 years ago

  • Keywords close removed

Opened #22038 to discuss UX of default background colors.

comment:5 @Ov3rfly3 years 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 3 years ago by Ov3rfly (previous) (diff)

comment:6 @DrewAPicture3 years ago

Related: #22038 – Bad UX with default background color

comment:7 @SergeyBiryukov3 years ago

  • Milestone set to Awaiting Review

comment:8 @toscho3 years ago

  • Cc info@… added

@obenland3 years ago

comment:9 @obenland3 years ago

  • Keywords has-patch added

@obenland3 years ago

comment:10 @Ov3rfly3 years ago

Tested 22030.1.2, works exactly as expected. Thanks.

Version 0, edited 3 years ago by Ov3rfly (next)

comment:11 @SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.6

comment:12 @lancewillett2 years ago

  • Keywords commit added

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

comment:13 follow-up: @nacin2 years 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: @ocean902 years 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 @DrewAPicture2 years 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 @obenland2 years 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 @ocean902 years 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: @aaroncampbell2 years 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 @ocean902 years 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 @ocean902 years ago

  • Keywords commit removed

comment:21 @obenland2 years 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 @nacin2 years 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 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

@obenland2 years ago

comment:24 @nacin22 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 @samuelsidler20 months ago

  • Priority changed from normal to low

comment:26 @nacin20 months ago

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

comment:27 @obenland19 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.

@kovshenin19 months ago

comment:28 @kovshenin19 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 @obenland18 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.

@obenland18 months ago

@obenland18 months ago

comment:30 @obenland18 months ago

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

comment:31 @ircbot18 months ago

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

@obenland18 months ago

comment:32 @DH-Shredder17 months ago

  • Keywords commit added

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

comment:33 @nacin17 months ago

In 27704:

Custom backgrounds: Always cause a preview in the customizer.

Omitted from [27703].

props obenland.
see #22030.

comment:34 @nacin16 months ago

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