WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#28687 closed defect (bug) (fixed)

Removing background image and/or reverting background color to default doesn't remove custom-background body class and remove theme mods

Reported by: slobodanmanic Owned by: ocean90
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.9
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

Scenario:

  • Set custom background color, then revert do default or
  • Set custom background image, then remove it

Problem:

  • None of the theme mods are removed
  • Because of this .custom-background is still being added

This is what I have in theme_mods_twentyfourteen after setting background color and image, then removing them both:

s:16:"background_color";s:6:"f5f5f5";
s:16:"background_image";s:0:"";
s:22:"background_image_thumb";s:0:"";
s:17:"background_repeat";s:6:"repeat";
s:21:"background_position_x";s:4:"left";
s:21:"background_attachment";s:6:"scroll";

Sorry if this has been filed before. I checked for tickets that could relate to this, but no luck.

I'll work on a patch today, but it would be great if someone could say if there's a reason for this behaviour.

Attachments (5)

28687.diff (681 bytes) - added by slobodanmanic 5 years ago.
28687-2.diff (513 bytes) - added by slobodanmanic 5 years ago.
28687-3.diff (592 bytes) - added by slobodanmanic 5 years ago.
28687-4.diff (537 bytes) - added by nitkr 5 years ago.
28687.5.diff (558 bytes) - added by valendesigns 5 years ago.

Download all attachments as: .zip

Change History (28)

@slobodanmanic
5 years ago

#1 @slobodanmanic
5 years ago

28687-2.diff checks if background color theme mod value is the same as theme default and if yes, doesn't add .custom-background to body classes.

Another problem remains, it's related, but not the same, and it has to do with all background related theme mods being kept in the database.

With background image:

  • when you remove it, it sets background_image theme mod to empty string, to make sure missing fallback to theme default image doesn't happen
  • when you reset it, theme mod is deleted and fallback to theme default happen

Color should work the same way, resetting to default should delete background_color theme mod, so it can fallback to theme default background color. 28687.diff does this.

Posted as two separate patches because these are two different, although closely related problems. I can post unified patch if that makes more sense.

#2 @slobodanmanic
5 years ago

There's also Customizer problem, it also doesn't remove any background theme mods when set to default. Is that possible in Customizer?

#3 follow-up: @helen
5 years ago

  • Version changed from trunk to 3.9

Related: #22030

I imagine this has happened since before 3.9, but please do let me know if this is only happening in trunk.

#4 in reply to: ↑ 3 @slobodanmanic
5 years ago

Replying to helen:

Related: #22030

I imagine this has happened since before 3.9, but please do let me know if this is only happening in trunk.

It's not only trunk and looking at #22030 it seems like it's been like that for a while.

#5 follow-up: @obenland
5 years ago

  • Milestone changed from Awaiting Review to 4.2

I could reproduce it. Let's fix that.

#6 in reply to: ↑ 5 @slobodanmanic
5 years ago

Replying to obenland:

I could reproduce it. Let's fix that.

I'll post a patch over the weekend.

#7 follow-ups: @obenland
5 years ago

slobodanmanic, have you had a chance to take another look?

#8 in reply to: ↑ 7 @slobodanmanic
5 years ago

Replying to obenland:

slobodanmanic, have you had a chance to take another look?

Still working on it, sorry. Will post an update soon.

#9 in reply to: ↑ 7 @slobodanmanic
5 years ago

Replying to obenland:

slobodanmanic, have you had a chance to take another look?

Can I get your opinion?

28687-3.diff is similar to 28687-2.diff, with one extra check and, if you agree with the implementation is the only file that needs to be changed.

Compared to current version of the file in trunk, it will now:

  1. Check if get_theme_mod( 'background_color' ) is set
  2. Check if its value is not equal to theme default
  3. (And check if there's a background image, which is all it currently does)

Theme mods are still not deleted and set to empty string instead, but .custom-background class will not be applied if both color and image are not set, or if color is set to theme default value.

Deleting theme mods when saving them from Custom Background page is not a problem, but I just can't figure it out when doing it from Customizer. I'd love to figure it out, but after looking at this for a while, I think that either:

  • Removing theme these two theme mods is necessary at all, since empty values do the trick OR
  • Removing background_image theme mod should also automatically remove background_repeat, background_position_x and background_attachment, and not leave them as they are, since they are tied to the image that was used

Let me know what you think, and if you have an idea how to remove background_image theme mod when Customizer saves after you removed background image, please let me know.

I tried adding remove_theme_mod to update in WP_Customize_Background_Image_Setting class, but that didn't work.

#10 @slobodanmanic
5 years ago

In other words, this should be two issues:

  1. Remove .custom-background class when not needed, fixed in 28687-3.diff
  2. Remove theme mods, if this even needs to be done in first place

#11 @obenland
5 years ago

I'll take a look soon, thanks!

@nitkr
5 years ago

#12 @nitkr
5 years ago

  • Keywords has-patch added

The condition check in this 28687-4.diff seems enough to fix this bug, theme mods doesn't seem like an issue.

Last edited 5 years ago by nitkr (previous) (diff)

@valendesigns
5 years ago

#13 @valendesigns
5 years ago

The previous patch didn't apply to the root, you had to cd into src. I created a new patch 28687.5.diff to fix that. I also tested the proposed change and it does work as expected. Cheers!

This ticket was mentioned in Slack in #core by drew. View the logs.


4 years ago

#15 @DrewAPicture
4 years ago

  • Owner set to obenland
  • Status changed from new to reviewing

@obenland: Can you pretty please test 28687.5.diff and make a recommendation? :-)

#16 follow-up: @ocean90
4 years ago

The patch needs to be checked against [21001].

#17 in reply to: ↑ 16 @obenland
4 years ago

Replying to ocean90:

The patch needs to be checked against [21001].

r21054 reverted parts of r21001, and 28687.5.diff is coherent with the last sentence in that commit message:

[…] the custom-background class should not be shown when only a default color is in use.

The patch allows .custom-background only to be added if background color or image divert from their defaults.

The question whether a theme mod should be removed if reset to its default value, could be material for another discussion or ticket. @ocean90 and I only started talking about it.

#18 @ocean90
4 years ago

  • Keywords commit added
  • Owner changed from obenland to ocean90
  • Status changed from reviewing to accepted

#19 follow-up: @ocean90
4 years ago

  • Keywords commit removed

Thanks @obenland. 28687.5.diff makes sense and would be consistent with the behaviour of _custom_background_cb().

But we need to update the callback for the customize preview too: trunk/src/wp-includes/js/customize-preview.js@31697#L139

#20 in reply to: ↑ 19 @obenland
4 years ago

Replying to ocean90:

But we need to update the callback for the customize preview too: trunk/src/wp-includes/js/customize-preview.js@31697#L139

Is there ever a situation where color() doesn't return a color?

Also: With wp.customize holding the currently saved value, do we have defaults available anywhere?

This ticket was mentioned in Slack in #core-customize by ocean90. View the logs.


4 years ago

#22 @ocean90
4 years ago

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

In 32081:

Don't print the custom-background class in body_class() when a default color is in use.

.custom-background should only be added if a background color or an image divert from their defaults. This behavior exists already in _custom_background_cb().

props slobodanmanic, nitkr, valendesigns, obenland.
see [21054], [21001].
fixes #28687.

#23 @arenddeboer
4 years ago

This might be a new bug, but when changing the wordpress url scheme to https from http, the database entry with the absolute url to the background image css is not updated, resulting in a mixed content warning.

Note: See TracTickets for help on using tickets.