Inline JavaScript is XHTML invalid
thekeyper - July 13, 2007 - 03:35
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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";
#1
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>
#2
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.
#3
version of patch for HEAD
#4
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.
#5
Patch looks good, applies cleanly.
Javascript still works after applying on firefox2/win.
#6
+1
However, based on my findings, the most backwards-compatible way is:
<script type="text/javascript"><!--
<![CDATA[
...
]]>
//-->
</script>
#7
#8
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.
#9
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
#10
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).
#11
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?
#12
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!)
#13
The approach in #10 is well known an well tested, and works for both
text/htmlandapplication/xhtml+xml. Could someone make a patch, so that you could get this in?#14
@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.
#15
#16
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 $foosort of comment.
#17
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 JavaScriptSuffix:
//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#18
@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?
#19
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.
#20
The patch in #10 looks subtle enough. I wish it could land for versions prior 7.x as well..
Subscribing.
#21
I think you've meant #15. Let's get this sucker in (and backport it later on).
#22
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 ...
#23
Patch is not in unified patch form and does not apply to 6.x.
$patch -p0 < inline-javascript.patch.txtpatching 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
#24
#25
Thanks, committed to Drupal 6!
temp
Automatically closed -- issue fixed for two weeks with no activity.
#26
Automatically closed -- issue fixed for two weeks with no activity.