Duplicate ID 'new' in forums breaks XHTML validation (patch attached, just needs reroll)
Damien Tournoud - March 17, 2009 - 20:03
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forum.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | brianV |
| Status: | reviewed & tested by the community |
| Issue tags: | Novice, XHTML, XHTML validation |
Description
Reported by #402254: How do we do on XHTML validation?, something is outputting duplicate 'new' IDs in forum listing:
GET http://dev.local/Drupal/Core7/forum/2 returned 200 (9.22 KB).
DOMDocument::loadHTML(): ID new already defined in Entity, line: 96
DOMDocument::loadHTML(): ID new already defined in Entity, line: 108
DOMDocument::loadHTML(): ID new already defined in Entity, line: 120
DOMDocument::loadHTML(): ID new already defined in Entity, line: 132
#1
Let's tag all these with 'XHTML validation'
#2
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; ?>
#3
#4
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.
#5
#6
I'm wondering: do we need the first of those links to be tagged with
id="new", in order to be a target for#newURL arguments?#7
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!
#8
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.
#9
This new patch adds
<a id="new"></a>to only the first new topic in the topic list.#10
Or instead of generating id="new", and jumping to that with "#new", we could just link to the proper comment ID?
#11
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.#12
Nice!
Some remarks:
1.
$is_first_new_topicshould be a boolean. I know that$stickyis not, but this one comes from the DB2.
<?php+ // make sure only one topic is indicated as the first new topic
+ $topic->is_first_new_topic = 0;
+ if (($topic->new != 0) && ($num_new_topics == 0)) {
+ $topic->is_first_new_topic = 1;
+ $num_new_topics++;
+ }
?>
Apparently, we don't need to count the new topics, but only to know if we are on the first new topic.
#13
Ok, I will make another pass at it with these comments in mind.
#14
New version rolled.
$is_new_topicis now a boolean.$num_new_topicsrenamed to$was_first_new_foundand made into boolean to better reflect usage.Relevant code section:
<?php// 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;
}
?>
#15
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 topicThis should start with a capital letter and end with a full stop.
Otherwise looks great.
#16
Oops!
New patch, with coding style fixes recommended above.
#17
The last submitted patch failed testing.
#18
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.
#19
How about
<?php$topic->first_new_topic
?>
#20
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.
#21
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.
#22
The last submitted patch failed testing.
#23
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.
#24
$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)
#25
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.
#26
After chatting with catch in IRC, I present....
!!!!!REVISION 8!!!!!
Certified 100% bug, BPA and MSG free. This product was not tested on animals.
#27
Looks good. I was going to suggest we write a test for this, but really that should be done when #355185: Add XHTML validation to simpletest gets in. Waiting on the test bot.
#28
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.
#29
Nice, more step on the road to generating valid XHTML ;)
Let's get this in.
#30
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'
#32
The last submitted patch failed testing.
#33
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.
#34
Re-roll of patch from #28.
#35
Tests pass, looks good
#36
The last submitted patch failed testing.
#37
Just bumping again. Needs a reroll, but I don't have time to tacke it.
#38
Reroll
#39
+++ 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.
#40
Looks good to me. Personally, I don't think it's worth a reroll to compress it down to
<a id="new" />#41
added my own suggestion from #39
#42
#43
The last submitted patch failed testing.
#44
Rerolled once again. Setting to CNR so the testbot will look at it, but this has really been RTBC since early June.
#45
The last submitted patch failed testing.
#46
oops.
#47
Since this is just a reroll of a patch that has been RTBC since June, I am putting back to RTBC.
#48
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).
#49
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?
#50
@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.#51
Thank you, well done!