Node taxonomy pre-processing produces invalid XHTML

Here's my patch for this. Please take a look and tell me what you think.

I've re-jigged things so that commas are produced inside the <li> elements rather than between them, like this:

<div class="node-terms">
  <ul class="taxonomy" role="navigation">
    <li class="vocab-term">
      <a title="Posts about Costumes" rel="tag" href="/category/costumes">Costumes</a>
      ,
    </li>
    <li class="vocab-term"></li>
  </ul>
</div>

I'm not sure how good my approach is - I tried looking for other themes which delimit terms with commas, but couldn't find any code to steal :-(

Hope you like it.

Nuts. I keep forgetting to update the status when I submit a patch :-)

Tested it out, and it validates with the W3C now. Forgot to mention that crucial bit.

Thanks Jeff! I'll check out the new HEAD soon.

About those additions you mention...

1) May I suggest using h3 for the vocab headers, or even h4? My reason is that node.tlp.php uses h2 for the node title when the node isn't a standalone page. If taxonomy terms are set to always display, then the vocab headers will be on the same level as the node title, which would be confusing in the typical river-of-news front-page scenario. It probably wouldn't be good for accessibility either. The vocab-headers should be at a level beneath the node title, since the vocabulary terms are a property of the node.

2) A delimiter setting is something I was going to suggest doing (you read my mind there), but I'm still getting my head around theme settings api, so I thought it best to fix the markup straight away.

Just read the latest HEAD...

$heading_level = $vars['page'] ? 'h2' : 'h3';

Looks good. I'm really starting to like the ternary operator after today. (Used to avoid it, thought it was hard to read...)

This issue is turning out well. The h2/h3 scenario is a good example of what the XHTML2 effort was trying to support with the <h> element.

This is in the new 6.x-1.6 release of adaptivetheme, so fixed.

Thanks Jeff!

Hi Jeff,
I'd like to re-open this issue, 'cos I've realized the problem is only half-fixed.

Case $taxonomy_format == 'vocab' works fine; however when $taxonomy_format == 'list' there is still a problem. The delimiter is properly nested inside the <li> when it is within a taxonomy, but lies outside the <li> when it is between two vocabularies - invalid XHTML.

i.e.,

<div class="node-terms">
  <ul class="taxonomy" role="navigation">
    <li class="vocab-term"> 
      <a>Automobile</a>   <!-- vehicle terms form vocab 1 -->
      ,                  <!-- this comma is valid xhtml -->
    </li>
    <li class="vocab-term">
      <a>Bicycle</a>
    </li>
    ,                            <!-- this comma is invalid xhtml -->
    <li class="vocab-term">   <!-- animal terms form vocab 2 -->
      <a>Cat</a>
      ,
    </li>
    <li class="vocab-term"> 
     <a>Dog</a>
    </li>
  </ul>
</div>

I've fixed this in a new patch (against current HEAD). Let me know what you think.

I inadvertently introduced a new bug with my last patch; the logic for deciding when to show the delimiter is flawed.

It leads to a hanging delimiter being displayed after the last term, when there are no terms assigned from the final vocabulary in the foreach ($vocabularies as $vocabulary) loop.

$use_delimiter would only be false when the final vocabulary was set to display and had terms assigned to the node.

Sorry for messing this up! I'll have a go at fixing it asap.

Okay, looks like I've fixed it, fingers crossed.

I tried a few different ways to get it to work, but the code was starting to get really messy. In the end I refactored it into two separate routines for handling one-list and per-vocab display settings. There's a little bit of repeated code, but it's much more readable to me, and more importantly, it works.

I'm going to keep testing this over the next few days, with lots of different taxonomy settings (core and theme) to check there isn't any more unexpected behaviour. I'll let you know how that goes.

Updated patches for 1.x and 2.x follow. I'm mainly working with 2.x now.

Ah, $output was empty when there were no terms applied to a node. Thanks Jeff!

template.preprocess-node.inc rev 1.1.2.9 introduced a new bug :-(

The if ($output != '') check is nested at the wrong level, inside foreach ($vocabularies)...

case "vocab":
  ...
  foreach ($vocabularies as $vocabulary) {
    ... 
    if ($terms) {
      ...
    } // end: if ($terms)
    if ($output != '') {
      $output = '<ul class="taxonomy">'. $output .'</ul>';
    }
  } // end: foreach ($vocabularies as $vocabulary)
  break;

The <ul class="taxonomy"> wrapper is applied per-vocabulary, leading to invalid output like this...

<ul class="taxonomy">
  <ul class="taxonomy">
    <ul class="taxonomy">
      <li class="vocab"/>
    </ul>
    <li class="vocab"/>
  </ul>
  <li class="vocab"/>
</ul>

So the if ($output != '') needs to move outside foreach ($vocabularies).

I didn't notice this until I started playing with the margins on li.vocab, but it's quite obvious on the base theme. Patch enclosed...