Make WordPress Core

Opened 12 years ago

Last modified 3 years ago

#21627 assigned enhancement

Filter for custom-background CSS selector

Reported by: horttcore's profile Horttcore Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: Future Release Priority: low
Severity: minor Version: 3.4.1
Component: Customize Keywords: needs-testing needs-unit-tests needs-patch needs-refresh
Focuses: Cc:

Description

There should be an easier way for changing the css selector from body to html or any other then making your own callback.

Attachments (7)

theme.diff (426 bytes) - added by Horttcore 12 years ago.
Filter applied to css selector
21627.diff (848 bytes) - added by westonruter 10 years ago.
Use theme feature attribute instead of filter for selector
21627.3.patch (4.1 KB) - added by peterwilsoncc 9 years ago.
21627.4.diff (3.6 KB) - added by peterwilsoncc 9 years ago.
21627.5.diff (4.6 KB) - added by peterwilsoncc 9 years ago.
Pass settings using WP_Customize_Manager::customize_preview_settings
21627.6.diff (4.2 KB) - added by peterwilsoncc 9 years ago.
21627.2.diff (4.3 KB) - added by peterwilsoncc 7 years ago.

Download all attachments as: .zip

Change History (39)

@Horttcore
12 years ago

Filter applied to css selector

#1 @nacin
12 years ago

Why might modifying this selector be necessary?

#2 @Horttcore
12 years ago

With the filter you could set it to any other element i.e. html which could be better for the theme.

Here is a sample code:

body {
   margin: auto;
   width: 965px;
}

With the current selector just a part of the site will have the custom background.
The background should be applied to the highest element in the DOM by default and that would be html.

We could add this filter or just change the selector to the html element.

What do you prefer?

#3 @cais
12 years ago

  • Cc edward.caissie@… added

#4 @rulatir
11 years ago

Very much seconded.

#5 @ocean90
10 years ago

#25379 was marked as a duplicate.

#6 @ocean90
10 years ago

Having a filter in a custom callback doesn't sound like a good plan, since we cannot ensure that plugin/theme authors are including the filter in a custom callback too.

#7 @dreamwhisper
10 years ago

  • Cc dreamwhisper added

#8 @nacin
10 years ago

  • Component changed from Themes to Appearance

@westonruter
10 years ago

Use theme feature attribute instead of filter for selector

#9 @westonruter
10 years ago

  • Milestone changed from Awaiting Review to Future Release

Replying to ocean90:

Having a filter in a custom callback doesn't sound like a good plan, since we cannot ensure that plugin/theme authors are including the filter in a custom callback too.

Right, when the custom-background theme support is added, the theme may supply a customwp-head-callback callback which would not be required to the proposed filter. However, if there was a filter, then themes would have much less cause to have to override wp-head-callback's default callback of _custom_background_cb to begin with. I would imagine that providing a custom callback for wp-head-callback is usually due to a different selector being needed.

As an alternative to providing a filter, there could be a new default element-selector property of the custom-background theme support. A custom theme callback would still need to honor element-selector, but they also have to do this already for background repeats, positions, and attachments. See 21627.diff.

#10 @celloexpressions
9 years ago

  • Milestone changed from Future Release to 4.1

+1 for 21627.diff.

#11 follow-up: @SergeyBiryukov
9 years ago

The selector also appears in customize-preview.js. Should it be updated there as well?

#12 in reply to: ↑ 11 @celloexpressions
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Priority changed from normal to low
  • Severity changed from trivial to minor

Replying to SergeyBiryukov:

The selector also appears in customize-preview.js. Should it be updated there as well?

Yes.

#13 @ocean90
9 years ago

  • Milestone changed from 4.1 to Future Release

#14 follow-up: @peterwilsoncc
9 years ago

Sorry, ignore .3

21627.4.diff builds on @westonruter's patch to include:

  • customize-preview.js
  • take theme authors preference in get_body_class()

customize-preview.js requires the element be selectable without the class, so I've split the element and the selector into two options applied to the theme-support array.

#15 in reply to: ↑ 14 @westonruter
9 years ago

Replying to peterwilsoncc:

Sorry, ignore .3

21627.4.diff builds on @westonruter's patch to include:

  • customize-preview.js
  • take theme authors preference in get_body_class()

customize-preview.js requires the element be selectable without the class, so I've split the element and the selector into two options applied to the theme-support array.

Thanks, but accessing settings from the parent frame via window.parent._wpCustomizeSettings isn't going to make it into Core. So either you need to use postMessage passing, or export the settings directly into the preview instead via WP_Customize_Manager::customize_preview_settings().

@peterwilsoncc
9 years ago

Pass settings using WP_Customize_Manager::customize_preview_settings

#16 @peterwilsoncc
9 years ago

Replying to westonruter:

Replying to peterwilsoncc:
So either you need to use postMessage passing, or export the settings directly into the preview instead via WP_Customize_Manager::customize_preview_settings().

Thanks for the feedback, I've updated in 21627.5.diff to use the correct method.

Whilst this code allows theme authors to customise the element and the class, it still enforces the overloaded element.classname selector, is it worth including a third setting and defaulting to the overloaded selector?

#17 @wonderboymusic
9 years ago

  • Keywords needs-refresh added

Patch doesn't apply

#18 @peterwilsoncc
9 years ago

  • Keywords has-patch added; needs-patch needs-refresh removed

Refreshed against trunk in 21627.6.diff

Renamed the theme support options and added the ability to customise the selector in the CSS, default values are:

add_theme_support( 'custom-background', array(
        'background-element'     => 'body',
        'background-class'       => 'custom-background',
        'background-selector'    => '%1$s.%2$s', // background-element.background-class
) );

An example customisation in a theme's functions.php might be:

add_theme_support( 'custom-background', array(
        'background-element'     => 'html',
        'background-class'       => 'has-custom-background',
        'background-selector'    => '.has-custom-background',
) );
Last edited 9 years ago by peterwilsoncc (previous) (diff)

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


8 years ago

#20 @celloexpressions
8 years ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 4.7

Quickly testing 21627.6.diff, I was not able to get it to work with Twenty Fifteen, adding 'background-selector' => '#main'. However, that was likely an error on my end and I think we're close to being ready with this improvement. Moving for 4.7 for consideration.

#21 @peterwilsoncc
8 years ago

  • Keywords needs-unit-tests added

Tests required:

  • various selectors
    • element
    • class
    • id
    • descendant
    • child
    • pseudo classes & elements

I'll need to go back and check the reason behind background-class to consider whether theme authors ought to be able to set an ID instead.

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


7 years ago

#23 @celloexpressions
7 years ago

  • Owner changed from Horttcore to peterwilsoncc
  • Status changed from new to assigned

Feel free to punt if there isn't time to finish this for 4.7; seems like you're the best person to own this for now @peterwilsoncc.

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


7 years ago

#25 @peterwilsoncc
7 years ago

  • Milestone changed from 4.7 to Future Release

I will bump this for a future release, the old patch is missing a bunch of escaping. The timeline for getting that in and unit tests seems a little tight.

21627.2.diff includes some escaping but I'm after some advice on escaping the selector.

#26 @westonruter
7 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Future Release to 4.8

@peterwilsoncc the background settings being added in WP_Customize_Manager should not be escaped because they are part of an array that will be safely serialized to JSON, and escaping should only be done at printing time.

In terms of escaping, you can look at wp_custom_css_cb() which escapes just by doing strip_tags. The key need there is to ensure that a user doesn't enter </style> or any script tags. It's important to not use esc_html() because that will corrupt descendant selectors, like html &gt; body.

That being said, I don't think escaping is entirely appropriate because the selector is being defined statically in code. It's not coming from user data.

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


7 years ago

#28 @westonruter
7 years ago

Another point where 21627.2.diff will need revising: window.parent._wpCustomizeSettings.theme.bgElement won't work reliably since the preview can be on another domain from the parent window, thus violating cross-domain security policy. So those theme settings need to be exported into the preview's settings instead of the pane's settings, as they're not needed in the pane anyway.

#29 @peterwilsoncc
7 years ago

  • Keywords needs-refresh added

Thanks @westonruter, I'll take a look at this in the course of the next week.

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


7 years ago

#31 @peterwilsoncc
7 years ago

  • Milestone changed from 4.8 to Future Release

Bumping this as there is no time to do properly before 4.8 & pending the outcome of #39128.

#32 @celloexpressions
3 years ago

It seems like implementing this filter requires more work than it may be worth. Themes can use their own image controls instead of the custom background theme-support feature set. And block based themes will use entirely different mechanisms for background images.

Unless anyone wants to pick this back up, I suggest closing.

Note: See TracTickets for help on using tickets.