#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)
Change History (28)
#2
@
10 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:
↓ 4
@
10 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.
#5
follow-up:
↓ 6
@
10 years ago
- Milestone changed from Awaiting Review to 4.2
I could reproduce it. Let's fix that.
#8
in reply to:
↑ 7
@
10 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
@
10 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:
- Check if get_theme_mod( 'background_color' ) is set
- Check if its value is not equal to theme default
- (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
@
10 years ago
In other words, this should be two issues:
- Remove .custom-background class when not needed, fixed in 28687-3.diff
- Remove theme mods, if this even needs to be done in first place
#12
@
10 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.
#13
@
10 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.
10 years ago
#15
@
10 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? :-)
#17
in reply to:
↑ 16
@
10 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
@
10 years ago
- Keywords commit added
- Owner changed from obenland to ocean90
- Status changed from reviewing to accepted
#19
follow-up:
↓ 20
@
10 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
@
10 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?
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:
background_image
theme mod to empty string, to make sure missing fallback to theme default image doesn't happenColor 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.