Currently, the "attachments"-table always uses "attachments" as ID for the generated table.

if (count($rows)) {
  return theme('table', $header, $rows, array('id' => 'attachments'));
}

There are a lot of situations in which more than one "attachments"-table is displayed on a page. Issue comments are an example: a random example shows 6 different places where "<table id='attachments' ..." is used.

The W3C validation flags this as invalid markup:

#  Error  Line 109, Column 11: ID "attachments" already defined.

<table id="attachments">

An "id" is a unique identifier. Each time this attribute is used in a document it must have a different value. If you are using this attribute as a hook for style sheets it may be more appropriate to use classes (which group elements) than id (which are used to identify exactly one element).

Attached is a patch against HEAD which replaces the use of "ID" by the use of "class".

CommentFileSizeAuthor
#11 upload-d6.patch633 bytesandypost
#2 366610_2_upload_attachments_table.patch560 bytesmr.baileys
upload_attachments_table.patch562 bytesmr.baileys

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs work » Needs review
StatusFileSize
new 366610_2_upload_attachments_table.patch560 bytes

re-rolled

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

Additional reasoning as to why the current usage of the ID attribute in theme_upload_attachments is a bug: Drupal Markup Style Guide

As a general rule; don't use the id attribute.

Unless you are absolutely 100% sure that your template file or theme function will only be called once per user-request, then your ID might cause the markup not to validate (the value must be unique to every other DOM element's id in the whole DOM tree).

That means, don't use the id attribute on templates of nodes and users and even common forms like search and login forms. Even if you include the object ID in the id attribute. You, as a developer, can not guarantee that particular node, form or user will only be on the page once. Remember that views and panels can pull lots of stuff into blocks and pages.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Issue tags: +Quick fix

I think this qualifies as a quick fix.

puradata’s picture

puradata commented
Status: Needs review » Needs work

Patch needs work. It adds two classes "attachment" and "sticky-enable". I think the intention is to have one class "attachment"

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs work » Needs review

Thanks for reviewing this patch.

The "sticky-enable" & "sticky-table" classes are added automatically when a table has a header. This patch doesn't address nor change this behavior. Below are two excerpts, the first one of an attachments-table rendered without this patch applied, the second one with this patch applied.

before

<table class="sticky-enabled sticky-table" id="attachments">
 <thead class="tableHeader-processed"><tr><th>Attachment</th><th>Size</th> </tr></thead>
<tbody>
 <tr class="odd"><td><a href="http://d7-patch/sites/default/files/satellite.jpg">satellite.jpg</a></td><td>52.26 KB</td> </tr>
</tbody>
</table>

after

<table class="attachments sticky-enabled sticky-table">
 <thead class="tableHeader-processed"><tr><th>Attachment</th><th>Size</th> </tr></thead>
<tbody>
 <tr class="odd"><td><a href="http://d7-patch/sites/default/files/satellite.jpg">satellite.jpg</a></td><td>52.26 KB</td> </tr>
</tbody>
</table>
I think the intention is to have one class "attachment"

No, the intention is to add an extra class (attachments), while removing the id (since we cannot guarantee that it will be unique on a rendered page)

puradata’s picture

puradata commented

my bad. The patch works correctly.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Issue tags: +XHTML validation

Tagging as an XHTML validation problem.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks great. Could probably do with a small note in the theme upgrade documentation after commit.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review
Issue tags: +Drupal 6 porting
StatusFileSize
new upload-d6.patch633 bytes

Please, backport this to 6 branch

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

Not sure if it's wise to backport this to D6: existing D6 themes might rely on the ID, and they might break if this gets backported...

andypost’s picture

andypost
he/him
Primary language Russian
commented

Existing D6 themes can be patched to track this change
But new D6 themes should have valid markup

catch’s picture

catch
he/him
Primary language English
commented

Can you do a grep of the themes repository for #attachments ?

andypost’s picture

andypost
he/him
Primary language Russian
commented

At this moment I can't

andypost’s picture

andypost
he/him
Primary language Russian
commented

Now results:

cvs checkout -r DRUPAL-6--1 -d themes6 contributions/themes (161)
7 themes (Aeon5 Alina blommor01 BlueSquare marinelli moleskine slash)

cvs checkout -r DRUPAL-6--2 -d themes6-2 contributions/themes (41)
only 1 theme (Aeon5)

So it not hard to notify them with issues

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -Drupal 6 porting +Needs backport to D6

fix tags

so why d6 can allow invalid markup?

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

Marked #529556: Attachments not assigning a unique ID as duplicate of this issue.

edit: fixed link.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Closed (won't fix)

I'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.

Anonymous’s picture

Anonymous (not verified) commented
Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Closed (fixed)

Changing issue status to reflect that it was fixed in 7.x.