title attribute should be inside control tag on admin interface.

I think that's going to be tied to this issue - #504962: Provide a compound form element with accessible labels

I've looked briefly at introducing it in modules/user/user.admin.inc with functions user_admin_permissions() & theme_user_admin_permissions().

Interestingly this was one of the first things I noticed about Drupal when I started using it. Funny that a bug never was filed.

The trick with using a fieldset is that a fieldset cannot be used across cells of a table.

Ok, so we can actually add this in as a label fairly easily now.

This just needs to be added to the checkbox array before it is rendered:

        $form['checkboxes'][$rid][$key]['#title'] = $roles[$rid] . ' : ' . t($key);
        $form['checkboxes'][$rid][$key]['#title_display'] = 'invisible';

I can't see any reason to have the title in the td tag in which case the next row in the array should be modified so that the title isn't output there:

          $row[] = array('data' => drupal_render($form['checkboxes'][$rid][$key]), 'class' => array('checkbox'));

Any thoughts on this?

Oh yes, not only does this need review, but I wanted to check if there are other places where this type of table of checkboxes might produce this type of inaccessible form.

In review of Patch #3:

• Pass 1: coder? | Passes.
• Pass 2: bottom up? | The code appears concise and understandable.
• Pass 3: comments? | Patch does not offer any additional comments. Perhaps they are not needed. Perhaps an explanation would help future coders adopt similar practices and preserve the patch should this hunk be reviewed again - especially if this is to be a new practice and not yet consistent through out the code base.
• Pass 4: topical? | Patch appears to address the stated issue, but there is the question of #4 - where else does this occur?
• Pass 5: does it work? | Probably, but I failed recreating the problem. For example, on D7 CVS-head @ admin/people/permissions, I did not find any instance of administer-news-feeds in the rendered source.

Can you provide more info on recreating?
Could a grep of the code base identify other sources of the title attribute outside the control tag?

Thank you and I apologize - I did enable the aggregator module, but it must have been on a different patch Drupal instance. I see both of your examples now.

I am board with the patch not fixing everything - especially if the other issues are unknown and it is reasonable to think they might not exist.

Pre-patch I have:

<td class="checkbox" title="authenticated user : administer news feeds"><div class="form-item form-type-checkbox form-item-2-administer-news-feeds">
 <input type="checkbox" name="2[administer news feeds]" id="edit-2-administer-news-feeds" value="administer news feeds"  class="form-checkbox" />
</div>
</td>

Post-patch I have:

<td class="checkbox" title="authenticated user : administer news feeds"><div class="form-item form-type-checkbox form-item-2-administer-news-feeds">
 <input type="checkbox" name="2[administer news feeds]" id="edit-2-administer-news-feeds" value="administer news feeds"  class="form-checkbox" />  <label class="element-invisible" for="edit-2-administer-news-feeds">authenticated user : administer news feeds </label>

</div>
</td>

So the invisible label is being rendered as described. What is the new line break above

?

@YaxBalamAhaw - I'm pretty sure that either a title with the checkbox or a label would work as well for accessibility. This the review with the label:

<td class="checkbox"><div class="form-item form-type-checkbox form-item-3-access-news-feeds">
 <input type="checkbox" name="3[access news feeds]" id="edit-3-access-news-feeds" value="access news feeds" checked="checked"  class="form-checkbox" />  <label class="element-invisible" for="edit-3-access-news-feeds">administrator : access news feeds </label>
</div>
</td>

I've got it up in my sandbox - http://drupal7.dev.openconcept.ca/admin/people/permissions

@tomdavidson - thanks for this review

Pass 3: comments? I don't think it needs it. It hasn't elsewhere.

Pass 4: topical? I don't know of any places where this occurs. Not sure if there's an easy way to grep through it though either.. Open to suggestions though. I use grep a lot.

Pass 5: does it work? it's up in my sandbox. It's an issue for all permission checkboxes on the permissions page.

I added a new patch to remove the title from the <td>

@mgifford - Hope I didnt give the wrong idea, those 5 questions were the ones I had asked myself and then tried to answer as part of the review. I will be more clear on future reviews.

Patch #8 is verified to work as described. There is still the extra empty line above the

in the rendered HTML. Looking at the rest of the page source I see similar empty lines, so under the assumption that is not significant or outside the scope of the patch, changed to reviewed & tested.

<td class="checkbox"><div class="form-item form-type-checkbox form-item-2-administer-news-feeds">
 <input type="checkbox" name="2[administer news feeds]" id="edit-2-administer-news-feeds" value="administer news feeds" class="form-checkbox">  <label class="element-invisible" for="edit-2-administer-news-feeds">authenticated user : administer news feeds </label>

</div>
</td>

@tomdavidson - I liked your 5 point process. I was trying to respond to the points & just wanted to echo them.

Thanks for marking this RTBC!

ahh ok. well, in that case I better give the source of what i was trying to do: http://webchick.net/6-pass-patch-reviews

ahh, very useful, thanks!

In case of English text, we don't need to use ' : ' but can instead use ': ' (no leading space). I've fixed that in the patch, and committed it to CVS HEAD. Thanks.