Make WordPress Core

Opened 12 years ago

Closed 10 years ago

#22030 closed defect (bug) (fixed)

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

Reported by: ov3rfly's profile 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 11 years ago.
22030.1.2.diff (815 bytes) - added by obenland 11 years ago.
22030.2.diff (1.9 KB) - added by obenland 11 years ago.
22030.3.diff (567 bytes) - added by kovshenin 10 years ago.
22030.4.diff (1.9 KB) - added by obenland 10 years ago.
22030.5.diff (1.6 KB) - added by obenland 10 years ago.
22030.6.diff (1.1 KB) - added by obenland 10 years ago.

Download all attachments as: .zip

Change History (41)

#1 @SergeyBiryukov
12 years ago

  • Component changed from General to Appearance

#2 @DrewAPicture
12 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.

#3 @lancewillett
12 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.

#4 @DrewAPicture
12 years ago

  • Keywords close removed

Opened #22038 to discuss UX of default background colors.

#5 @Ov3rfly
12 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 12 years ago by Ov3rfly (previous) (diff)

#6 @DrewAPicture
12 years ago

Related: #22038 – Bad UX with default background color

#7 @SergeyBiryukov
12 years ago

  • Milestone set to Awaiting Review

#8 @toscho
12 years ago

  • Cc info@… added

@obenland
11 years ago

#9 @obenland
11 years ago

  • Keywords has-patch added

@obenland
11 years ago

#10 @Ov3rfly
11 years ago

Tested 22030.1.2 with 3.4.2, works exactly as expected. Thanks.

Last edited 11 years ago by Ov3rfly (previous) (diff)

#11 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.6

#12 @lancewillett
11 years ago

  • Keywords commit added

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

#13 follow-up: @nacin
11 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?

#14 follow-up: @ocean90
11 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.

#15 in reply to: ↑ 14 @DrewAPicture
11 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.

#16 in reply to: ↑ 13 @obenland
11 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.

#17 @ocean90
11 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().

#18 follow-up: @aaroncampbell
11 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).

#19 in reply to: ↑ 18 @ocean90
11 years ago

Replying to aaroncampbell:

The patch works good.

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

#20 @ocean90
11 years ago

  • Keywords commit removed

#21 @obenland
11 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.

#22 @nacin
11 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.

#23 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

@obenland
11 years ago

#24 @nacin
10 years ago

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

Marking for commit for 3.8.

#25 @samuelsidler
10 years ago

  • Priority changed from normal to low

#26 @nacin
10 years ago

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

#27 @obenland
10 years 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.

@kovshenin
10 years ago

#28 @kovshenin
10 years 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.

#29 @obenland
10 years 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.

@obenland
10 years ago

@obenland
10 years ago

#30 @obenland
10 years ago

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

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


10 years ago

@obenland
10 years ago

#32 @kirasong
10 years ago

  • Keywords commit added

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

#33 @nacin
10 years ago

In 27704:

Custom backgrounds: Always cause a preview in the customizer.

Omitted from [27703].

props obenland.
see #22030.

#34 @nacin
10 years ago

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