XHTML validation

A simple no-brainer, and so this is RTBC.

Thanks, committed.

The problem with auto-generated form id's (as well as JavaScript compatibility) was discussed at http://drupal.org/node/111719 - including a patch to review.

I agree.

Why are we still using the .clear class? That was part of drupal.css in 4.7. All core themes should be using .clear-block to clear containers.

for patch #3, the .clear class inside phptemplate_preprocess_page() is obviously wrong since .clear isn't inside defaults.css or system.css.

The original patch that went in is causing Garland to make node links 1px in height. Not good!

This patch is to fix what originally went in. It also moves around some of the markup so it's more exposed. i.e. primary/secondary local tasks.

The span clearing tag within the secondary task was not needed at all. There's a rule already set for "ul.secondary" to clear itself.

The clear-block declaration inside the rtl style is not needed so it's been removed. The other instances of the "clear" class has been removed and replaced with "clear-block".

@dvessel: That's out of my scope - making pages XHTML valid is one thing, altering themes is another and I do not have any coding experience with Drupal core themes. As a themer I know that even such small changes can break the layout in certain browsers. So, that would have to be done by someone else. Probably best in another issue.

sun, this is exactly what your doing. Altering a core theme to make it valid. I'm fully behind you with this and lets do it right. :)

The patch looks great.

Just this one part:

RCS file: /cvs/drupal/drupal/modules/trigger/trigger.module,v
retrieving revision 1.2
diff -u -p -r1.2 trigger.module
--- modules/trigger/trigger.module	26 Sep 2007 18:19:22 -0000	1.2
+++ modules/trigger/trigger.module	5 Oct 2007 20:45:15 -0000
@@ -333,7 +333,12 @@ function theme_trigger_display($element)
     }
   }
 
-  $output = theme('table', $header, $rows) . drupal_render($element);
+  if (count($rows)) {
+    $output = theme('table', $header, $rows) . drupal_render($element);
+  }
+  else {
+    $output = drupal_render($element);
+  }
   return $output;
 }

Do we really want to omit the table? A patch recently went in to prevent validation errors even when there's no content. Have the table present all the time presents a cue to the user that they need to fill it in for it to do anything. The rest of core does this.

Okay, I actually looked at that page where the tables are affected and indeed it needs to go. Sorry bout that.

All tested and it's looking very good.

Why did you move the list logic to the theme instead of the preprocess functions? It does not look logical.

Because it already does an if condition inside .tpl due to the tabs wrapper and it exposes the markup where it should be, the template file. Thought it would benefit themers in a small way if anything.

I can switch it back so the logic goes back into the preprocess function but that condition inside the template has to stay in effect checking twice. Not a big deal, I can change it back.

OK, understood now. Let me think about this a bit more though.

Looked through the patch again and committed, thanks.