Duplicate ID 'new' in forums breaks XHTML validation (patch attached, just needs reroll)

Let's tag all these with 'XHTML validation'

tagging this for the novice queue. It looks like the problem is in forum-icon.tpl.php.

<?php if ($new_posts): ?>
  <a id="new">
<?php endif; ?>

Patch attached.

Changed

<a id="new">

to:

<a class="new-posts">

The class name was changed to 'new-posts' since the 'new' class is already used in Garland for the 'new' indicators on comments not yet seen by the users.

.new {
  color: #ffae00;
  font-size: 0.92em;
  font-weight: bold;
  float: right; /* LTR */
}

From Garland's comment.tpl.php:

<?php if ($comment->new) : ?>
    <span class="new"><?php print drupal_ucfirst($new) ?></span>
<?php endif; ?>

I don't think this class name change affects any other theming in core.

I hadn't thought of that. The module uses the first 'id="new"' as the anchor the browser jumps to.

Since the 'name' attribute is deprecated in XHTML, we need to use an 'id' to jump to with that link.

This means we will need to write code to determine which thread is the first unread thread, and create a link before it to act as the target.

Looking into it further!

Just assigning to me, and changing to 'needs work'. I have a pretty good patch on the go that should fix this issue properly. It's just a little more involved than I expected.

This new patch adds <a id="new"></a> to only the first new topic in the topic list.

Or instead of generating id="new", and jumping to that with "#new", we could just link to the proper comment ID?

Dries,

I suspect you may be thinking of the page displaying a given forum topic along with it's replies. In that context, your suggestion makes sense, but it is an issue for comment.module.

(off-topic)
Just to point out a small problem: with the existing approach, the comment.module only has to calculate the most recent comment when the user visits a node containing new comments. In the usage you suggest, the most recent comment would have to be calculated for every node displayed in forum or node listings in order to generate the correct 'new' link containing the comment id to the latest comment, regardless of whether or not the user actually visits the node. This would be somewhat less efficient from a performance standpoint.
(/offtopic)

That aside, this issue is looking at the forum topic list, which shows a table or forum topics, the number of replies, who created it, and who made the last post.

N.B. The handling in the patch I've written is consistent with that used by comment.module in the same situation. comment.module also places <a id="new" /> in the first unread comment in the list of comments.

Ok, I will make another pass at it with these comments in mind.

New version rolled.

1. $is_new_topic is now a boolean.
2. $num_new_topics renamed to $was_first_new_found and made into boolean to better reflect usage.

Relevant code section:

// make sure only one topic is indicated as the first new topic
$topic->is_first_new_topic = false;
if (($topic->new != 0) && (!$was_first_new_found)) {
  $topic->is_first_new_topic = true;
  $was_first_new_found = true;
}

Dries, being able to link to the correct comment ID instead of #new is the subject of #26966: Fix comment links when paging is used. - there aren't any nice ways to do this with the current state of comment module.

Couple of code style issues with this patch:

+  $was_first_new_found = false;
 

All the booleans need to be upper case - TRUE/FALSE.

+    // make sure only one topic is indicated as the first new topic

This should start with a capital letter and end with a full stop.

Otherwise looks great.

Oops!

New patch, with coding style fixes recommended above.

Re-upping patch with a new name to be retested. Patch applied cleanly against head here, so I have no idea why the testbot failed.

How about $topic->first_new_topic without the is_ just like sticky doesnt have is_ and similarly remove was_

dmitri,

it's a pretty common convention to prefix a boolean with is_ to hint at the type by the name.

If I had to judge by the name alone, I would assume 'first_new_topic' contained the nid of the first new topic, or something similar. 'is_first_new_topic' automatically implies that we are dealing with a boolean.

On the other hand, this convention isn't followed to strictly within Drupal core.

I agree 'was_first_new_found' is pretty nasty.

New revision.

I renamed $was_first_new_found to $first_new_found. However, I've left $is_first_new_topic alone for the reasons stated above.

I accidentally removed a line I didn't intend to in the last patch, causing it to fail some tests.

New patch attached, hopefully the final one for this issue.

$is_first_new_topic should probably just be $first_new_topic or $first_new

This bit reads a bit awkwardly to me:

+    $topic->is_first_new_topic = FALSE;
+    if (($topic->new != 0) && (!$first_new_found)) {
+      $topic->is_first_new_topic = TRUE;
+      $first_new_found = TRUE;
+    }

Could we do something like

+    if (($topic->new != 0)) {
+      $topic->first_new = !$first_new_found;
+      $first_new_found = TRUE;
+    }

(not tested)

New version posted.

I renamed $is_first_new_topic to $new_topic.

I've changed the one hunk to closer match what is posted in #6:

+    // Make sure only one topic is indicated as the first new topic.
+    if (($topic->new != 0) && (!$first_new_found)) {
+      $topic->first_new = !$first_new_found;
+      $first_new_found = TRUE;
+    }

Please note that the code above is only executed once, due to checking $first_new_found. If that is omitted (as in #6), the loop is executed for every 'new' comment in the listing, which could be as many as 50 times.

Hopefully, this is the last patch for this issue.

After chatting with catch in IRC, I present....

!!!!!REVISION 8!!!!!

Certified 100% bug, BPA and MSG free. This product was not tested on animals.

Looks good. I was going to suggest we write a test for this, but really that should be done when #355185: Add HTML validation to functional tests gets in. Waiting on the test bot.

Spotted one more thing - we use if / endif in template files rather than braces, sorry all these came one by one. Re-rolled with just that one change, this should be ready to go.

Re: if / endif in template files..

I wasn't aware of that. Marking down in my book of 'odd little things about Drupal module coding'

Just bumping this issue - needs to be rerolled. If someone gets to it before I do, good. Otherwise, I will get to it sooner or later.

Re-roll of patch from #28.

Tests pass, looks good

Just bumping again. Needs a reroll, but I don't have time to tacke it.

Reroll

+++ modules/forum/forum-icon.tpl.php	19 Sep 2009 13:32:36 -0000
@@ -9,17 +9,16 @@
+  <a id="new"></a>

not that happy about this, shoudn't it be <a id="new" />

This review is powered by Dreditor.

Looks good to me. Personally, I don't think it's worth a reroll to compress it down to <a id="new" />

added my own suggestion from #39

Rerolled once again. Setting to CNR so the testbot will look at it, but this has really been RTBC since early June.

oops.

Since this is just a reroll of a patch that has been RTBC since June, I am putting back to RTBC.

It would be great to see a screenshot of how this changes the behavior. I'm trying to understand how important this feature is (vs removing it).

This is absolutely required for #653940: Clean-up: Tests do not report all errors.

+++ modules/forum/forum-icon.tpl.php	4 Dec 2009 16:17:58 -0000
@@ -9,17 +9,15 @@
+ * - $first_new: Indicates whether this is the first topic with
+ *   new posts.

Wraps too early.

+++ modules/forum/forum.module	4 Dec 2009 16:17:58 -0000
@@ -855,6 +856,13 @@ function forum_get_topics($tid, $sortby,
+    

Trailing white-space here.

+++ modules/forum/forum.module	4 Dec 2009 16:17:58 -0000
@@ -855,6 +856,13 @@ function forum_get_topics($tid, $sortby,
+    if (($topic->new != 0) && (!$first_new_found)) {

Plenty of unnecessary parenthesis.

+++ modules/forum/forum.module	4 Dec 2009 16:17:58 -0000
@@ -1017,7 +1025,7 @@ function template_preprocess_forum_topic
-      $variables['topics'][$id]->icon = theme('forum_icon', array('new_posts' => $topic->new, 'num_posts' => $topic->comment_count, 'comment_mode' => $topic->comment_mode, 'sticky' => $topic->sticky));
+      $variables['topics'][$id]->icon = theme('forum_icon', array('new_posts' => $topic->new, 'num_posts' => $topic->comment_count, 'comment_mode' => $topic->comment_mode, 'sticky' => $topic->sticky, $topic->first_new));

This is either not required or it is missing an array key.

I'm on crack. Are you, too?

@sun

Turns out I rerolled that badly. Re-rolled with your suggestions changed, the theme() call fixed, and tested thoroughly this time.

@Dries

As requested, I have also attached a screenshot which illustrates the change. Basically, there is no graphical change. The only change is that the first new icon (green) gets the <a id="new" /> (green codeblock) while the remaining icons (red) don't get it. (red codeblock).

This is, of course, used in links to the first new topic in the forum (eg., http://example.com/forum/1#new).

Previous behaviour was that each new forum icon would receive the <a id="new" />. This breaks XHTML validation, as each id element is required to have a unique name.

Thank you, well done!

+++ modules/forum/forum-icon.tpl.php	8 Dec 2009 06:21:54 -0000
@@ -9,17 +9,14 @@
+  <a id="new" />

This is technically valid XHTML, but not supported by even good browsers (including Firefox 3.5, Opera 10 and Google Chrome.)

Specifically, to get perfect support for this we would probably need to serve pages as application/xhtml+xml, creating a host of other problems.

From http://dusan.fora.si/blog/self-closing-tags

Do self-close tags with no closing tag, such as br, hr, img, link, base or meta. For example, <br></br> is not valid. But don’t do it to tags that do not expect that. It may be valid, but you won’t like the result. Just do <a name="anchor"></a> or such.

See also this test: http://dusan.fora.si/blog/wp-content/uploads/2007/07/tags.html

To ensure cross-browser support, please make the above a <a id="new"></a> instead.

This review is powered by Dreditor.

If this is still considered for D7, here's a reroll for current HEAD containg the fix suggested by @Arancaytar.

Glad to see the reroll. I kind of ran out of steam rolling and re-rolling this patch (12 times!).

Looks good, was RTBC for a very long time, back to RTBC.

#53: 405238-53.patch queued for re-testing.

#53: 405238-53.patch queued for re-testing.

While I am incredibly loathe to change anything to do with template files at this point, this does fix a legitimate bug, and I can't envision another way to do it. The good news is it seems to do so in a way that will not break BC in a theme that's already overridden this (they'll get the same XHTML validation errors as ever), and far better to get this fix in now than after RC.

Committed to HEAD. Thanks!