Improper escaping of CDATA for inline css

According to http://en.wikipedia.org/wiki/CDATA#Use_of_CDATA_in_program_output and http://dorward.me.uk/www/comments-cdata/#mandatory-xhtml, this is the correct method for escaping CDATA for css blocks.

The test for adding inline css has to be fixed too.

#3: 1021622-cdata_escaping_in_xhtml-3.patch queued for re-testing.

The patch in #3 should also change the comments, as they no longer match the proposed code.

See #672954-12: Wrap inline CSS with explicit CDATA for the backstory on these lines of code.

Aha, sorry, I missed the /* and */ wrapping the <![CDATA[ tags. Looks good to me, but this is not my area of expertise.

Patch in 3 solves the validation error and the display problems in opera, but this needs to be fixed in Drupal 8 first.

Moving to major since it breaks output on certain browsers.

Referencing #1541438: Please lets remove the embed prefix. If we decide to do that, then this would only be applicable to 7.x.

Tagging.

Can we confirm whether this is still an issue in current D8?

This does still appear to be an issue in D8. I tested with recent versions of browsers and all rendered the pages ok with no breakage or console errors that I could tell. I imagine that some supported browsers could still break. The last submitted patch no longer applies and needs a reroll.

Sample output:

Opera 12.14:

<!--[if lt IE 7]>
<style media="all">
<!--/*-->
<!--[CDATA[/*-->
<!--*/
body {color: red;}
/*]]>*/-->
<!--[endif]---->
<!--[if lte IE 8]>
<script src="http://d8/core/misc/html5.js?v=11"></script>
<![endif]-->

Firefox 20.0-beta:

-[if lt IE 7]>
<style media="all">
<!--/*-->
    
      <!--[CDATA[/*-->
    
      <!--*/
body {color: red;}
/*]]>*/-->
    
      <!--[endif]---->
    
      <!--[if lte IE 8]>
<script src="http://d8/core/misc/html5.js?v=11"></script>
<![endif]-->

Chrome 25.0.1364.172:

<!--[if lt IE 7]>
<style media="all">
<!--/*-->
<!--[CDATA[/*-->
<!--*/
body {color: red;}
/*]]>*/-->

Here's a reroll with a test.

Sample output with patch applied:

Firefox 20.0-beta

<!--[if lt IE 7]>
<style media="all">
/* <![CDATA[ */
body {color: red;}
/* ]]> */
</style>
<![endif]-->

Remember to put the test-only patch first so the bot doesn't mess with the status. :)

Downgrading from major since the problem only occurs with certain custom CSS. @dags confirmed that there are no rendering errors with or without the patch in HEAD and the latest browsers above, but per #1, this is still a markup bug.

#15: 1021622-cdata_escaping_in_xhtml-withtest.patch queued for re-testing.

I was about to mark this needs work for the same thing bfroehle did in #6, but #7 confirms that it's still correct even though it's kinda hard to tell by the diff.

Committed and pushed to 8.x. Thanks! Moving down to 7.x.

Backported #15 to D7.

Oops. >.>

Wow, this is one patch that is still literally identical in D8 and D7. Looks good!

I read through the issues linked to above and it seems that extra commenting is not superflous; it's there to support older HTML user agents (e.g., #672954: Wrap inline CSS with explicit CDATA links to http://www.webdevout.net/articles/escaping-style-and-script-data).

I am not sure what we're supporting in that regard but there needs to be some discussion of that before committing this to Drupal 7... For example, does this cause issues with the HTML seen by relatively primitive user agents (cURL or drupal_http_request())?

It would be easy enough to test a rerolled patch with cURL & drupal_http_request().

What other discussion should happen and how would we make it happen more quickly?

Rerolled #22.

We still need to address David_Rothstein's concerns in #25.

1) "extra commenting is not superflous"

2) "does this cause issues with the HTML seen by relatively primitive user agents (cURL or drupal_http_request())?"

This change will not cause an issue with drupal_http_request() as it doesn't parse the html, only the headers; my guess is the same for cURL. Things further down the line do the parsing http://stackoverflow.com/questions/3409264/how-to-parse-actual-html-from...

Searching D8 code I don't really see this being used though; it is used in here https://api.drupal.org/api/drupal/core!tests!Drupal!Tests!Core!Asset!Css... but the $wrap_in_cdata is never true so it never runs.