In theme_comment_view(), there's a check to see if a comment is new or not. If new, a "new" ID is output for the comment. If there are multiple new comments, they will get duplicate IDs, which is forbidden in (X)HTML. We should use classes instead of IDs so themes can style new comments. The first new comment should also have an ID. This would require theme changes, as well as comment.module changes.

CommentFileSizeAuthor
#9 comment_new_id.patch930 bytesZen
#5 comment_32.patch887 bytesZen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Title: Node view pages use duplicate ID for all new comments » Comment module outputs duplicate IDs for all *new* comments

verified on drupal.org. we should not be doing this.

Junyor’s picture

Junyor CreditAttribution: Junyor commented

I've been looking into this a bit today. The comment that should have the "new" ID is dependent on the order of the comment page. I think the first new message chronologically should have the "new" ID. We can do a check similar to the one in comment_render to see what order the page is in. If the page is reverse chronological, we'll have to use a counter to find the oldest new comment.

I'll give this some more thought and see if I can come up with a good solution.

Uwe Hermann’s picture

Uwe Hermann CreditAttribution: Uwe Hermann commented

Any updates on this?

Junyor’s picture

Junyor CreditAttribution: Junyor commented

I haven't come up with a better way of doing it, but it's still happening (AFAICT; I tested on drupal.org).

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
comment_32.patch887 bytes

I disagree. It should be the *first* new comment on a page. This way, I just have to keep scrolling down the page to read other new comments and avoid missing other new comments above my current page position.

Moreover, this keeps the patch simple. Please review =)

Thanks,
-K

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Active » Needs review
Dries’s picture

Dries CreditAttribution: Dries commented

I support the last patch. However, maybe we should explore a different solution too; rather than generating a 'new' id, maybe we can just use the actual $comment->cid? Looks like that might be less code and slightly more elegant (we are already querying the number of new comments) ... Not sure it will work but maybe someone could explore that path?

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Needs work

Looks good and it is good to see someone working on the default implementation of themeable functions.

However, I think the wording could be a bit better. Maybe $new_id_used or $first_new (with TRUE/FALSE flipped).

Zen’s picture

Zen CreditAttribution: Zen commented
Version: x.y.z » 4.7.4
Status: Needs work » Needs review
FileSize
comment_new_id.patch930 bytes

Patch updated as per Drumm's comments against 4-7-4.

-K

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Version: 4.7.4 » 5.x-dev

moving

ChrisKennedy’s picture

ChrisKennedy CreditAttribution: ChrisKennedy commented

+1

It is terrible usability for the link to new comments to go to the last one. On drupal.org I constantly have to search backwards through pages to find the oldest unread comment. It is completely annoying.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Fixed

Committed to HEAD.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Fixed » Patch (to be ported)
m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Version: 5.x-dev » 4.7.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community

Should be applied to 4.7 too. Should still apply to 4.7...

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

applied

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)