WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 5 days ago

#39461 new defect (bug)

default-preset option for custom background cannot be set by default

Reported by: cristian-ungureanu Owned by:
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

I am trying to set a custom background by default for this theme https://wordpress.org/themes/islemag/ . This is my code:

		add_theme_support( 'custom-background', array(
			'default-image'          => get_template_directory_uri() . '/img/islemag-background.jpg',
			'default-preset'         => 'fill',
			'default-repeat'         => 'no-repeat',
			'default-position-x'     => 'center',
			'default-attachment'     => 'fixed',

		) );

When I open the customizer, the default preset option is set on "fill screen" but the background image is still small. Everything works great if I change the default preset and then change it back on "fill screen"

I talked with developers in core channel on slack and @celloexpressions told me this:
"Currently you need to define the default for each individual property (size, repeat, etc.) in addition to defining the preset. It would probably be better for consistency if each property is set based on the preset if the theme sets a preset."

Tried to set each property for custom background but with no luck.

Attachments (2)

39561.diff (11.5 KB) - added by cdog 8 months ago.
39461.diff (11.9 KB) - added by cdog 4 months ago.

Download all attachments as: .zip

Change History (24)

#1 follow-up: @celloexpressions
9 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7.1

Moving to 4.7.1 for investigation since this is introduced in 4.7. Could move to a 4.7.2 if there isn't a solution ready in time. CC @cdog since I have a feeling you may know the best way to fix this.

Ideally, themes should only need to set the default-preset in the add theme support and the other properties would be based on that where applicable.

#2 in reply to: ↑ 1 @cdog
9 months ago

Thanks for reporting! Looking into it.

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


9 months ago

#4 @celloexpressions
9 months ago

  • Milestone changed from 4.7.1 to 4.7.2

Let's aim for 4.7.2; this is a fairly major bug with the feature introduced in 4.7 in terms of themes implementing it, but it'll be better to give a bit more time to work out the correct fix. If there's a minimal patch ready by tomorrow we could reconsider getting it in sooner, though.

#5 @cdog
9 months ago

OK, this should be better documented. It works as designed.

This should get you working (you've omitted the background size setting):

add_theme_support( 'custom-background', array(
	'default-image'      => get_template_directory_uri() . '/img/islemag-background.jpg',
	'default-size'       => 'cover',
	'default-repeat'     => 'no-repeat',
	'default-attachment' => 'fixed',
) );

Anytime the Default preset is selected it will revert to your theme's default settings.

Also make sure to remove any theme mods if already set (using wp-cli):

$ wp theme mod remove background_size background_position_x background_position_y background_repeat background_attachment

The Default preset is intended to be altered by themes. A theme can define its own defaults by overriding core defaults. See wp-includes/theme.php:

$defaults = array(
	'default-image'          => '',
	'default-preset'         => 'default',
	'default-position-x'     => 'left',
	'default-position-y'     => 'top',
	'default-size'           => 'auto',
	'default-repeat'         => 'repeat',
	'default-attachment'     => 'scroll',
	'default-color'          => '',
	…
);

The other presets can't be altered (and shouldn't; doing so can cause unexpected results). The 'default-preset' should be set other than 'default' only when you want to use an already defined preset exactly as it is. Doing so requires you to set all options according to the preset (maybe we can improve this). See wp-admin/js/customize-controls.js:

values = { // position_x, position_y, size, repeat, attachment
	'default': defaultValues,
	'fill': [ 'left', 'top', 'cover', 'no-repeat', 'fixed' ],
	'fit': [ 'left', 'top', 'contain', 'no-repeat', 'fixed' ],
	'repeat': [ 'left', 'top', 'auto', 'repeat', 'scroll' ]
};

Any thoughts on this are welcome. @celloexpressions, @cristian-ungureanu what do you think?

Last edited 9 months ago by cdog (previous) (diff)

#6 follow-up: @celloexpressions
9 months ago

Thanks for looking into this @cdog. Do you think it would be possible for the the various properties to default to the defaults associated with the default preset that the theme sets? For example, if a theme does this:

add_theme_support( 'custom-background', array(
	'default-image'  => get_template_directory_uri() . '/img/islemag-background.jpg',
	'default-preset' => 'fill',
));

Could the default background-size, etc. be set automatically?

#7 in reply to: ↑ 6 @cdog
9 months ago

Thanks for the input @celloexpressions. I'll think of a solution and propose a patch. When should this be ready?

#8 @celloexpressions
8 months ago

It can go in whenever it's ready :) I think 4.7.2 is at least a few weeks out if we want to target that release.

@cdog
8 months ago

#9 @cdog
8 months ago

  • Keywords has-patch added; needs-patch removed

Sorry for delay, got busy lately. The good news is that I just got 2 tickets to WordCamp Europe! See you there :)

Now on the patch. 39561.diff (misspelled it, can someone rename it?) provides a solution for what we discussed and addresses a display issue when loading the customizer. Specifically some controls can toggle the display of other controls. Choosing a background image was either showing or hiding all background settings overriding the selected preset related controls (for example, the Default preset shouldn't display any controls; choosing an image was showing them).

Next, I'll provide some working examples for a better understanding of the changes and how this can be used. The workflow from the interface remains the same.

Example 1. Choosing a default preset (this could be default, fill, fit, repeat or custom)

add_theme_support( 'custom-background', array(
	'default-image'  => get_template_directory_uri() . '/img/autumn.jpg',
	'default-preset' => 'fill'
));

Specifying a value for default-preset will set the active preset and inherit all properties of the preset.

Example 2. Overriding default settings for presets

add_theme_support( 'custom-background', array(
	'default-image'      => get_template_directory_uri() . '/img/autumn.jpg',
	'default-position-x' => 'center',
	'default-position-y' => 'center'
));

This will apply for all presets. Prior to the patch this was affecting the Default preset only. Switching a preset from one to another will revert to these values instead of core defaults.

add_theme_support( 'custom-background', array(
	'default-image'  => get_template_directory_uri() . '/img/autumn.jpg',
	'default-preset' => 'fit'
	'default-repeat' => 'no-repeat'
));

Because the Fit to Screen preset allows you to toggle the Repeat Background Image option from the interface now you can do the same from your theme.

add_theme_support( 'custom-background', array(
	'default-image'      => get_template_directory_uri() . '/img/autumn.jpg',
	'default-preset'     => 'fit'
	'default-attachment' => 'fixed'
));

The default-attachment property will be ignored in this case. It will affect only the presets that allow this change (like Default, Repeat and Custom). All values are sanitized to drop any invalid settings.

Example 3. Owning the Default preset from a theme (revert to your own defaults instead of core defaults)

add_theme_support( 'custom-background', array(
	'default-image'      => get_template_directory_uri() . '/img/autumn.jpg',
	'default-preset'     => 'default',
	'default-position-x' => 'center',
	'default-position-y' => 'center',
	'default-size'       => 'cover',
	'default-repeat'     => 'no-repeat',
	'default-attachment' => 'fixed'
));

Doing this you will have your own Default preset.

@celloexpressions hope this helps. I aimed for greater flexibility and to fix existing issues. Any feedback is welcome. Thanks!

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


7 months ago

#11 @westonruter
7 months ago

  • Milestone changed from 4.7.3 to 4.7.4

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


6 months ago

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


6 months ago

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


6 months ago

#15 @swissspidy
6 months ago

  • Milestone changed from 4.7.4 to 4.8

Moving to the 4.8 milestone for now as decided during today's bug scrub.

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


4 months ago

#17 @obenland
4 months ago

  • Milestone changed from 4.8 to Future Release

#18 follow-up: @celloexpressions
4 months ago

9 and 39561.diff needs a review still; it's been long enough since I've dug into this that it would probably be most effective if a committer can do it. It looks pretty good to me at a glance.

@cdog
4 months ago

#19 in reply to: ↑ 18 @cdog
4 months ago

Refreshed the patch and added a missing docblock. See: 39461.diff. This should fix the op issue and a few others (minor changes) described in my previous comments without changing the existing functionality in core. I'm also here if there's something to be discussed/updated prior to commiting the current patch or any feedback on a better approach.

#20 @westonruter
4 months ago

  • Milestone changed from Future Release to 4.8.1

#21 @westonruter
4 months ago

  • Milestone changed from 4.8.1 to 4.9

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


5 days ago

Note: See TracTickets for help on using tickets.