I tried to add inline CSS styles to <head> section using following PHP code:

$dynamic_style = ".wrapper  { width: 90%;} /* This is sample comment */"
drupal_add_css($data = $dynamic_styles, $options['type'] = 'inline', $options['preprocess'] = FALSE);

Unfortunately Drupal ignores anything between /* and */. This might look like a minor issue (comments for inline styles are not very useful), but real problems start when I wrap styles with CDATA section:

$dynamic_styles = "<!--/*--><![CDATA[/*><!--*/  .wrapper { width: 100%; }  /*]]>*/-->"
drupal_add_css($data = $dynamic_styles, $options['type'] = 'inline', $options['preprocess'] = FALSE);

In this case, the output will be:

<style>
<!--  .wrapper { width: 100%; }  -->  
</style>

instead of:

<style>
<!--/*--><![CDATA[/*><!--*/  .wrapper { width: 100%; }  /*]]>*/-->
</style>

which will result in missing styles and invalid markup.

Documentation says that if 'preprocess' option is set to TRUE, then inline styles "will be compressed when being output on the page". But it appears to me that styles are being compressed no matter whether $options['preprocess'] is set to TRUE or FALSE.

Comments

jarek foksa’s picture

Priority: Critical » Normal
damien tournoud’s picture

Status: Active » Closed (works as designed)

Outputting the CDATA part is not your job, it's the job of the page template.

damien tournoud’s picture

Category: bug » support
jarek foksa’s picture

Could you precise how to do this in template file? All styles added via drupal_add_css() are outputted by <?php print $styles; ?> in html.tpl.php. Note that CDATA must be put inside <styles></styles> tags, not outside them.

damien tournoud’s picture

You don't have to use CDATA at all. The parameter to drupal_add_css() is a plain CSS snippet. Why would you want to add CDATA in there?

damien tournoud’s picture

Title: Drupal_add_css() strips everything between /* and */ signs » Wrap inline CSS with explicit CDATA
Category: support » bug
Status: Closed (works as designed) » Active

Actually, we probably want to wrap those inline CSS (new in Drupal 7) in explicit CDATA, for consistency with inline javascript. Requalifying as a bug.

effulgentsia’s picture

Status: Active » Needs review
StatusFileSize
new2.75 KB

This patch includes the prefix and suffix used in the issue overview, but I have no idea if it's correct. Someone who understands that gibberish, please comment as to whether that is the correct way to comment out the CDATA for HTML4.

effulgentsia’s picture

Component: theme system » markup

"markup" component maintainers are the best candidates to review whether the way in which the CDATA wrapping and commenting of CDATA tags for HTML4 is correct.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Unfortunately, I've lost the original source we used for the JS equivalent. However, according to http://www.webdevout.net/articles/escaping-style-and-script-data, this seems to be correct.

realityloop’s picture

Issue tags: -drupal_add_css, -cdata

#7: css_cdata-672954-7.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +drupal_add_css, +cdata

The last submitted patch, css_cdata-672954-7.patch, failed testing.

jacine’s picture

Here's another good source on this: http://hixie.ch/advocacy/xhtml

<script> and <style> elements in XHTML sent as text/html have to be
escaped using ridiculously complicated strings.

This is because in XHTML, <script> and <style> elements are #PCDATA
blocks, not #CDATA blocks, and therefore <!-- and --> really _are_
comments tags, and are not ignored by the XHTML parser. To escape
script in an XHTML document which may be handled as either HTML4 or
XHTML, you have to use:

      <script type="text/javascript"><!--//--><![CDATA[//><!--
        ...
      //--><!]]></script>

To embed CSS in an XHTML document which may be handled as either
HTML4 or XHTML, you have to use:

      <style type="text/css"><!--/*--><![CDATA[/*><!--*/
        ...
      /*]]>*/--></style>

Yes, it's pretty ridiculous. If documents _aren't_ escaped like
this, then the contents of <script> and <style> elements get
dropped on the floor when parsed as true XHTML.

(This is all assuming you want your pages to work with older
browsers as well as XHTML browsers. If you only care about XHTML
and HTML4 browsers, you can make it a bit simpler.)

jacine’s picture

Status: Needs work » Needs review
StatusFileSize
new2.79 KB

Here's a re-roll of @effulgentsia's patch.

Jeff Burnz’s picture

Status: Needs review » Reviewed & tested by the community

This appears to work as expected.

sun’s picture

Issue tags: -drupal_add_css, -cdata

#13: css_cdata-672954-13.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, css_cdata-672954-13.patch, failed testing.

jacine’s picture

Status: Needs work » Needs review
Issue tags: +drupal_add_css, +cdata

#13: css_cdata-672954-13.patch queued for re-testing.

jacine’s picture

Status: Needs review » Reviewed & tested by the community

Something must have been up with the bot when @sun retested. This is still green, so setting back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think this makes sense, since it brings CSS inline with the way JS behaves in D7.

But there are tests in at least filter.test that are checking for this behaviour with scripts. We need ones that check it for CSS as well.

sun’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

The patch already contains a change for the actually affected functionality that's covered by tests. User input and/or filter.test has nothing to do with this functionality.

tom_o_t’s picture

Issue tags: -drupal_add_css, -cdata

#13: css_cdata-672954-13.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +drupal_add_css, +cdata

The last submitted patch, css_cdata-672954-13.patch, failed testing.

jacine’s picture

Status: Needs work » Needs review
StatusFileSize
new2.77 KB

Here's a straight re-roll since #13 doesn't apply anymore.

jacine’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Weird. Not sure how I missed that .test hunk before.

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -drupal_add_css, -cdata

Automatically closed -- issue fixed for 2 weeks with no activity.