WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 6 months ago

#24813 closed defect (bug) (fixed)

wp_add_inline_style behaviour changes with SCRIPT_DEBUG

Reported by: stephenharris Owned by: SergeyBiryukov
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.3
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (9)

print-inline-style.diff (445 bytes) - added by stephenharris 9 months ago.
Use print_inline_style with second argument set to false
24813.rev.diff (1.6 KB) - added by georgestephanis 6 months ago.
24813.2.diff (633 bytes) - added by georgestephanis 6 months ago.
24813.diff (1.2 KB) - added by nacin 6 months ago.
24813.4.diff (3.5 KB) - added by atimmer 6 months ago.
24813.5.diff (4.1 KB) - added by atimmer 6 months ago.
24813.6.diff (4.8 KB) - added by georgestephanis 6 months ago.
24813.7.diff (4.8 KB) - added by georgestephanis 6 months ago.
24813.8.diff (5.3 KB) - added by georgestephanis 6 months ago.
Adding a unit test to make sure there aren't stray <style> tags when there aren't any inline styles to put in them.

Download all attachments as: .zip

Change History (33)

stephenharris9 months ago

Use print_inline_style with second argument set to false

comment:1 SergeyBiryukov9 months ago

  • Component changed from Appearance to General
  • Version changed from 3.5.2 to 3.3

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

Related: [18464], [24047].

comment:2 markoheijnen8 months ago

  • Milestone changed from Awaiting Review to 3.7

comment:3 SergeyBiryukov8 months ago

  • Keywords commit added; 2nd-opinion removed

comment:4 in reply to: ↑ description ; follow-up: nacin8 months ago

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?

comment:5 in reply to: ↑ 4 ; follow-up: stephenharris8 months ago

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.

comment:6 in reply to: ↑ 5 ; follow-up: nacin8 months ago

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.

comment:7 in reply to: ↑ 6 stephenharris8 months ago

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.

comment:8 SergeyBiryukov8 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 25202:

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

comment:9 follow-up: georgestephanis6 months ago

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?

comment:10 follow-up: georgestephanis6 months ago

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";

comment:11 in reply to: ↑ 9 stephenharris6 months ago

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.

comment:12 in reply to: ↑ 10 ; follow-up: stephenharris6 months ago

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 ).

comment:13 in reply to: ↑ 12 georgestephanis6 months ago

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.

georgestephanis6 months ago

comment:14 georgestephanis6 months ago

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.

comment:15 georgestephanis6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

georgestephanis6 months ago

comment:16 georgestephanis6 months ago

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

nacin6 months ago

atimmer6 months ago

atimmer6 months ago

georgestephanis6 months ago

comment:17 georgestephanis6 months ago

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)

georgestephanis6 months ago

georgestephanis6 months ago

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

comment:18 nacin6 months ago

In 25785:

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

see #24813, #25282.

comment:19 nacin6 months ago

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

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.

comment:20 follow-up: celloexpressions6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

http://celloexpressions.com/files/2014-25786-issue.png

Probably want to avoid this happening...

comment:21 georgestephanis6 months ago

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?

comment:22 azaozz6 months ago

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.

comment:23 in reply to: ↑ 20 nacin6 months ago

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.

comment:24 nacin6 months ago

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

In 25809:

Avoid printing a possible HTML element. fixes #24813.

Note: See TracTickets for help on using tickets.