Line 1671 of common.inc says:

$output .= '<script type="text/javascript"'. ($info['defer'] ? ' defer="defer"' : '') .'>'. $info['code'] ."</script>\n";

Should this not be changed to:

$output .= '<script type="text/javascript"'. ($info['defer'] ? ' defer="defer"' : '') .'>
//<![CDATA[
'. $info['code'] ."
//]]>
</script>\n";
CommentFileSizeAuthor
#24 drupal6.js-inline.patch1.68 KBsun
#22 inline-javascript.patch1.23 KBDries
#15 drupal.js-inline.patch1.46 KBsun
#7 drupal.js-inline.patch799 bytessun
#3 js_CDATA_HEAD.patch823 byteszeta ζ
#2 js-cdata-wrapper.patch795 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Version: 5.1 » 5.7

Yup. My XHTML pages don't validate unless I can wrap my javascript in CDATA. This appears to be a problem only when my javascript also includes XHTML. For example:

<script type="text/javascript">
$(document).ready(
  function(){
    $('.breadcrumb a[href="/"]').prepend('<img src="/sites/icirr/themes/frijheid/favicon.ico" alt="✵" />');
  }
);
</script>
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Active » Needs review
FileSize
js-cdata-wrapper.patch795 bytes

I made a patch which makes valid XHTML of all javascript. It shouldn't cause problems for HTML4 themes, but I can't test every browser out there anyway.

zeta ζ’s picture

zeta ζ CreditAttribution: zeta ζ commented
Version: 5.7 » 7.x-dev
FileSize
js_CDATA_HEAD.patch823 bytes

version of patch for HEAD

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I am not a javascript programmer, so excuse me if I don't use the right words for things.

I found a reliable work around for my purposes. The javascript needs to be wrapped in CDATA when it contains javascript strings or string literals with characters that are reserved in HTML: < > = &. Otherwise, validators want to try to validate the HTML in there.

The drupal_to_js function in common.inc contains some code for this, but that also adds the string delimiters ("). To use this exact function in my theme, I had to 'forget' to put the quotes. See:

$script = '$(document).ready(function(){$(".breadcrumb a:eq(0)").prepend(';
$script .= drupal_to_js('<span class="pngfix"><img src="' . check_url(theme_get_setting('favicon')) . '" alt="✵" /></span>');
$script .= ');});';
drupal_add_js($script, 'inline');

So you don't need to add the quotes around the 'stuff' in your javascript where you'd usually do it if you were just concatenating a bunch of strings to put through drupal_add_js.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented

Patch looks good, applies cleanly.

Javascript still works after applying on firefox2/win.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work

+1
However, based on my findings, the most backwards-compatible way is:

<script type="text/javascript">
<!--
<![CDATA[
...
]]>
//-->
</script>
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Wrapping Javascript in //<![CDATA[ in order to validate XHTML » Inline JavaScript is XHTML invalid
Status: Needs work » Needs review
FileSize
drupal.js-inline.patch799 bytes
quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

sun, the patch looks good, but could you reference the links that say this is the best (most compatible) practice? I've personally been using:

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

Which looks like it'd be equivalent to #3. However, browsers are finicky things.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Some links I reference for the approach mentioned in #8.
http://developer.mozilla.org/en/docs/index.php?title=Properly_Using_CSS_...
http://www.alistapart.com/articles/keepelementskidsinlinewithoffspring

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Sorry, it seems I have recalled it wrongly. Actually, it should be:

<script type="text/javascript">
<!--//--><![CDATA[//><!--
function nothing() { }
//--><!]]>
</script>

There are many references, but the most detailed and comprehensive is http://en.wikipedia.org/wiki/XHTML (see section 3 and 4).

zeta ζ’s picture

zeta ζ CreditAttribution: zeta ζ commented

I assume that the method in #10 relies on line endings to finish the comments. I would prefer a version based on that used for css (which doesn’t have // comments), if this possible, and is sufficiently compatible with browsers de antiqua:

<script type="text/javascript">
 <!--/*--><![CDATA[/*><!--*/ function nothing() { } /*]]>*/-->
 </script>

I appreciate the method in #10 has been tested, whereas this hasn’t: Maybe any advantage is not worth it?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

What's wrong with just encoding your javascript string literals as described in #4? If there are browsers that can't handle CDATA blocks correctly or can't handle them without excessive and unnecessary comment blocks, why not just go the way that does work reliably across browsers? (Unless it doesn't. Please inform me!)

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Needs work

The approach in #10 is well known an well tested, and works for both text/html and application/xhtml+xml. Could someone make a patch, so that you could get this in?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

@bangpound: See http://en.wikipedia.org/wiki/XHTML#Common_errors

@zeta: Depends on what we want. If we do not want to support browsers prior 2000 the method in #8 would be sufficient.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal.js-inline.patch1.46 KB
zeta ζ’s picture

zeta ζ CreditAttribution: zeta ζ commented

I wasn’t concerned about supporting even older browsers, just the reliance on line endings to finish the comments.

Wouldnt the /* style of comment */ be better here? The part being commented is not naturally a line by itself or a
$foo = 'bar' // set value of $foo
sort of comment.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

The part being commented is not naturally a line by itself

AFAIK, it is, because this prefix/suffix needs to be on a separate line and does not affect the embedded JavaScript code. Last patch in #15 already ensures this.

As far as I can understand this wrapper:
Prefix:

  • <!-- HTML: Hides the following CDATA construct in HTML4
  • // JavaScript: Comments out the rest of this line in HTML4
  • --> HTML: Ends previous comment in XHTML
  • <![CDATA[ XHTML: Starting CDATA construct in XHTML
  • //> JavaScript: Comments out the rest of this line in XHTML
  • <!-- Hides JavaScript code from browsers that do not support JavaScript

Suffix:

  • // JavaScript: Comments out the rest of this line in HTML4
  • --> HTML: Ends hiding of JavaScript code from browsers that do not support JavaScript
  • <!]]> XHTML: Closing CDATA construct in XHTML
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

@sun i don't understand why you're posting that link. my question is not about which XHTML comment style to use. my question is why not just encode the offending characters that cause validation to fail rather than use obtuse hack-like comment syntax to make the most non-compliant browsers behave?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Escaping characters in the script would be non-trivial and may break some scripts. Also, your proposed method in #4 does not work for HTML4 and older browsers that don't support JavaScript at all, because the script won't be commented out at all.
Like Damien mentioned, the method in #10 resp. #15 (patch) is well known, well tested, and known to work.

Please test.

yhager’s picture

yhager CreditAttribution: yhager commented

The patch in #10 looks subtle enough. I wish it could land for versions prior 7.x as well..
Subscribing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

I think you've meant #15. Let's get this sucker in (and backport it later on).

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-dev » 6.x-dev
FileSize
inline-javascript.patch1.23 KB

I've committed a modified patch to CVS HEAD. I simply added some code comments. Patch attached so it can be reviewed and/or backported to Drupal 6. Lowering version to Drupal 6 ...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

Patch is not in unified patch form and does not apply to 6.x.

$patch -p0 < inline-javascript.patch.txt 
patching file includes/common.inc
Hunk #3 FAILED at 2077.
Hunk #4 FAILED at 2081.
2 out of 4 hunks FAILED -- saving rejects to file includes/common.inc.rej
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
FileSize
drupal6.js-inline.patch1.68 KB
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed to Drupal 6!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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