The JavaScript for drupal is generated by drupal_get_js. Not one does it do the preprocessing magic it renders the xhtml output. Since this output is not sent through a theme function it cannot be overriden. The current output is valid xhtml. But, as html5 becomes more popular some will want to change the JavaScript output to be valid html5.

A simple example of the problem....

Currently we have something like:

<script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery.extend(Drupal.settings, { "basePath": "/" });
//--><!]]>
</script>

This is valid xhtml. We actually need the CDATA for the xhtml to validate. This is not the case with html5. Valid html5 markup would look like:

<script>
jQuery.extend(Drupal.settings, { "basePath": "/" });
</script>

The default type for script is text/javascript so we don't even need to specify that (though we could).

The inability to change the markup is a bug we need to fix. Patch to come soon.

CommentFileSizeAuthor
#4 drupal-js-themable-1.patch7.52 KBmfer
#2 drupal-js-themable.patch7.51 KBmfer

Comments

robloach’s picture

robloach
he/him
commented

Oh dear god yes.

mfer’s picture

mfer commented
Status: Active » Needs review
StatusFileSize
new drupal-js-themable.patch7.51 KB

This is a companion issue to #576940: links tags which reference css are not themable. There is, also, #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag which I am not opposed to. But, with so little time until D7 is actually frozen for API changes I don't see that one going in.

robloach’s picture

robloach
he/him
commented
Status: Needs review » Needs work

Very nicely done! So close, a couple small changes and it's good.

+++ includes/common.inc	2009-10-07 15:06:28 +0000
@@ -3417,12 +3417,6 @@ function drupal_get_js($scope = 'header'
-  // For inline Javascript to validate as XHTML, all Javascript containing
-  // XHTML needs to be wrapped in CDATA. To make that backwards compatible
-  // with HTML 4, we need to comment out the CDATA-tag.
-  $embed_prefix = "\n<!--//--><![CDATA[//><!--\n";
-  $embed_suffix = "\n//--><!]]>\n";

I like this change a lot!

+++ includes/common.inc	2009-10-07 15:06:28 +0000
@@ -3459,7 +3456,7 @@ function drupal_get_js($scope = 'header'
     // starting with "ad*".
     $filename = 'js_' . md5(serialize($files) . $query_string) . '.js';
     $preprocess_file = file_create_url(drupal_build_js_cache($files, $filename));
-    $preprocessed .= '<script type="text/javascript" src="' . $preprocess_file . '"></script>' . "\n";
+    $preprocessed .= theme('script', NULL, array('src' => $preprocess_file)) . "\n";
   }

Having a NULL argument first looks kind of weird, but it makes sense, since its the contents of the script, and this script tag isn't meant to have anything in it.

+++ includes/theme.inc	2009-10-07 14:57:31 +0000
@@ -2019,6 +2019,33 @@ function theme_indentation($size = 1) {
+
+  //set a default value for type
+  if(!isset($attributes['type'])){
+    $attributes['type'] = 'text/javascript';
+  }

"// Set a default value for the type." ;-)

+++ modules/system/system.module	2009-10-07 15:02:38 +0000
@@ -1664,7 +1664,7 @@ function system_admin_menu_block($item) 
     SELECT m.load_functions, m.to_arg_functions, m.access_callback, m.access_arguments, m.page_callback, m.page_arguments, m.title, m.title_callback, m.title_arguments, m.theme_callback, m.theme_arguments, m.type, m.description, m.path, m.weight as router_weight, ml.*
-    FROM {menu_router} m 
+    FROM {menu_router} m
     LEFT JOIN {menu_links} ml ON m.path = ml.router_path
     WHERE (ml.plid = :plid AND ml.menu_name = :name AND hidden = 0) OR (m.tab_parent = :path AND m.type IN (:local_task, :default_task))", array(':plid' => $item['mlid'], ':name' => $item['menu_name'], ':path' => $item['path'], ':local_task' => MENU_LOCAL_TASK, ':default_task' => MENU_DEFAULT_LOCAL_TASK), array('fetch' => PDO::FETCH_ASSOC));
   foreach ($result as $link) {
@@ -1698,7 +1698,7 @@ function system_admin_menu_block($item) 
@@ -1698,7 +1698,7 @@ function system_admin_menu_block($item) 
     }
   }
   if ($has_subitems) {
-    // If we've had at least one non-tab subitem, remove the link for the 
+    // If we've had at least one non-tab subitem, remove the link for the

These whitespace changes probably shouldn't be in this patch.

I'm on crack. Are you, too?

mfer’s picture

mfer commented
Status: Needs work » Needs review
StatusFileSize
new drupal-js-themable-1.patch7.52 KB

Here is an update based on the feedback from Rob Loach.

robloach’s picture

robloach
he/him
commented
Status: Needs review » Reviewed & tested by the community

The whitespace changes are minor, and they have to happen anyway. RTBCing!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

wow, erm, now you're trying to sneak in that theme function for links in here?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Sure, that's a great API design. theme_css(), theme_script(), theme_link(), theme_........

So much time wasted instead of working on the proper solution.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

FYI: I'm referring to #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag (again).

mfer’s picture

mfer commented

@sun If I thought that issue would get into D7 I would pursue it. But, I find this one to be much more likely. I would rather have this and the css counterpart in core rather than putting all my eggs in the basket of #552478 and have it not go in. This is a simple bug fix which creates a minor change. That is proposing an API change that isn't ready to go yet.

mfer’s picture

mfer commented

Should the interface to the theme function be a $element array? Like those used in to many other parts of drupal?

The function definition would looke like:

  function theme_script(array $element = array()) {

And the input array would look like:

  $element = array(
    '#value' => 'The inline js goes here',
    '#attributes' => array('defer' => 'defer'),
  );

This change would mean the script tag can be used by drupal_render as well.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

I prefer #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag if we can get it ready.

This should be blowing up massively because all theme functions take a single array now.

mfer’s picture

mfer commented
Status: Needs work » Postponed

Postponing in light of #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag. If that goes in we don't need this one.

aptereket’s picture

aptereket commented

Thanks for the patch!

robloach’s picture

robloach
he/him
commented
Status: Postponed » Fixed

#552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag was committed and added theme_html_tag(), which is now used for both the JavaScript and CSS script tags. This is now considered fixed :-) .

Status: Fixed » Closed (fixed)

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