wp_add_inline_style behaviour changes with SCRIPT_DEBUG

[stephenharris]

More specifically the behaviour changes on whether styles are concatenated: Added CSS is (or isn't) wrapped in <style> tags depending on whether scripts are (or are not) concatenated.

wp_add_inline_style() allows for printing additional CSS after the specified stylesheet has been loaded, which it does in ​WP_Styles::print_inline_style.

The second argument in print_inline_style() allows you to print or return these additional style rules. If set to print, it also prints <style> tags around the CSS. If the second argument is false, it just returns the attached data, with no <style> tags added.

The actual bug is ​WP_Styles::do_item. Depending on the the value $this->do_concat it uses print_inline_style argument with the second argument set to true or false.

This means if you use wp_add_inline_style() you must enclose the style rules yourself in <style> tags - unless, for example, SCRIPT_DEBUG is set to true.

[stephenharris]

Use print_inline_style with second argument set to false

[SergeyBiryukov]

Confirmed. The fix seems consistent with how WP_Scripts works: tags/3.5.2/wp-includes/class.wp-scripts.php#L122​.

Related: [18464], [24047].

[nacin]

Replying to stephenharris:

This means if you use wp_add_inline_style() you must enclose the style rules yourself in <style> tags - unless, for example, SCRIPT_DEBUG is set to true.

This should be fun to break. What happens when <style> tags are nested? Do things still work OK?

[stephenharris]

Replying to nacin:

This should be fun to break. What happens when <style> tags are nested? Do things still work OK?

If the <style> tags are nested then the styling is not applied (Firefox/Chrome). Although the trailing tag is not displayed, older browsers might fare worse.

But nested <style> tags are not a problem here - any bugs associated with this fix would be because the user did not wrap their rules in <style> tags. But if they were doing that it would already be broken with SCRIPT_DEBUG set to false.

The above patch only changes behaviour when SCRIPT_DEBUG is set to true.

[nacin]

Replying to stephenharris:

The above patch only changes behaviour when SCRIPT_DEBUG is set to true.

Got it, thanks. I read it as the reverse. Is this (from the ticket description) wrong?

This means if you use wp_add_inline_style() you must enclose the style rules yourself in <style> tags - unless, for example, SCRIPT_DEBUG is set to true.

[stephenharris]

Replying to nacin:

Got it, thanks. I read it as the reverse. Is this (from the ticket description) wrong?

I've had to double check this every time I've looked at this :). Nope the description is correct. Standard behaviour is that the user is responsible for adding the <style> tags. But in SCRIPT_DEBUG, wp_add_inline_style() adds its own <style> tags. The patch stops it from doing that.

[SergeyBiryukov]

In 25202:

Prevent WP_Styles::do_item() from adding its own style tags when concatenation is disabled. props stephenharris. fixes #24813.

[georgestephanis]

I'm not sure if this is really the best resolution here. It's breaking things that had worked previously.

The documentation on wp_add_inline_style ​in the codex and ​in the phpdoc states explicitly to pass the CSS in as the second parameter -- nowhere is it indicating that it should include tags, and the example in the Codex explicitly doesn't include the tags.

If we're changing how the function is used, okay I guess, but can we add something to wp_add_inline_style that will wrap the string in <style> tags there if it's just clean CSS, for backcompat?

[georgestephanis]

And for that matter, if the expectation is that they are passing in something already wrapped in <style> tags, then why do we have ​this bit that wraps it in the output function?

echo "<style type='text/css'>\n";
echo "$output\n";
echo "</style>\n";

[stephenharris]

The patch was intended to modify behaviour when SCRIPT_DEBUG == true (as this sets $wp_scripts->do_concat to false). That way production sites would be unaffected, and the function would behave consistently.

What I've only just realised on revisiting this is that !is_admin() also $wp_scripts->do_concat set to false.

($wp_scripts->do_concat is set by global $concatenate_scripts, itself set here ​https://github.com/WordPress/WordPress/blob/master/wp-includes/script-loader.php#L902.)

In light of that the above issue still stands admin-side, but the attached patch isn't recommendable.

@georgestephanis' suggestion of checking for <style> tags is probably the best way forward.

[stephenharris]

Replying to georgestephanis:

And for that matter, if the expectation is that they are passing in something already wrapped in <style> tags, then why do we have ​this bit that wraps it in the output function?

Indeed, but there is a return before its wrapped in <style> tags, and when that second argument of print_inline_style() is set to false, as is (sometimes) done by $wp_styles->do_item() you get an inconsistent behaviour.

On a production site (SCRIPT_DEBUG == false it therefore behaves differently depending on whether your admin-side or front-end ).

[georgestephanis]

Replying to stephenharris:

On a production site (SCRIPT_DEBUG == false it therefore behaves differently depending on whether your admin-side or front-end ).

I don't think you can rely on script debug never being on in production.

I'll have a patch up in a bit if nobody beats me to it.

[georgestephanis]

New diff attached, this seems to fix all the issues and is fully backward compatible in my testing. It probably could use some unit testing, but as this is fixing a regression, getting it in quickly would probably be good.

I also added phpdoc, and removed the return true -- this way it will just return the style, which will evaluate to true. It simplifies the code a bit, and more readable code is always a win.

The only downside is that it will no longer concatenate multiple added inline styles into a single <style> element, but if we can't be sure about whether they've already been wrapped in <style> tags, that's no longer feasible anyways.

[georgestephanis]

New patch, this one reverts r25202 and resolves the initial conflict back to how it should have been from the beginning -- there should not be <script> tags in the provided css to wp_add_inline_style

[georgestephanis]

​24813.6.diff just added -- adds the s (PCRE_DOTALL) flag to the preg_replace so that it will work with multi-line CSS being passed in. Otherwise, this would fail:

$data = "<style type='text/css'>
.class {
	property: value;
}
</style>";

echo preg_replace( '#<style[^>]*>(.*)</style>#i', '$1', $data );

as the dot-wildcard doesn't natively include line breaks.

Also added a trim around the preg_replace as if the developer is doing it multi-line, it would leave a trailing line break, that we can safely dispose of.

I also tweaked the tests from the previous patch to more accurately reflect line breaks, and use actual selector { property: value; } syntax, as well as specifying the initial 'handle' stylesheet as http://example.com rather than just a protocol-less example.com

I've not tested the tests, so if someone could, that'd be super -- but I think I've got it all correct.

I also corrected the spelling of surpress to suppress (pedant that I am)

[georgestephanis]

Adding a unit test to make sure there aren't stray <style> tags when there aren't any inline styles to put in them.

[nacin]

In 25785:

Test runner: Add @expectedIncorrectUsage to trap _doing_it_wrong() calls.

see #24813, #25282.

[nacin]

In 25786:

Revert [25202] and enforce that wp_add_inline_style() does not want <style> tags.

Prior to 3.7, these tags were not printed (and thus needed to be provided), but only in the admin and when concatenation was enabled. They should never be required. Strip them when we find them and issue a notice for incorrect usage.

props atimmer, georgestephanis.
fixes #24813.

[celloexpressions]

Chrome is interpreting the <style> tag in the doing_it_wrong message as a style tag and doing weird things in Twenty Fourteen:

Probably want to avoid this happening...

[georgestephanis]

Should the output of _doing_it_wrong maybe be esc_html'd? Or are there often strong tags and the like that would need to persist?

[azaozz]

The output from _doing_it_wrong() could be displayed in the browser, or could be added to a log file. esc_html() would be pretty bad for the log. Another trick/hack is to leave a space between the < and the first char of a tag name. Here it would be < script ... >...< /script > and browsers won't render the tag.

[nacin]

Replying to celloexpressions:

Chrome is interpreting the <style> tag in the doing_it_wrong message as a style tag

I caught this earlier this week and completely forgot. Fixing.

[nacin]

In 25809:

Avoid printing a possible HTML element. fixes #24813.