314195 - show_bug.cgi template does not have labels for most inputs

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[apla]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

see summary and patch

Reproducible: Always




bugzilla default theme, english language

Comment 1

[apla]

Attachment: patch against current cvs for fix bug issue

Comment 2

[A. Karl Kornel]

Comment on attachment 201131 [details] [diff] [review]
patch against current cvs for fix bug issue

Reporter, you need to ask someone to review this patch.  You can find the list of reviewers at <http://www.bugzilla.org/docs/reviewer-list.html>.

Pick somebody appropriate, set the review flag on the attachment to ?, and fill in the email address of the person you have chosen.  Thanks!

Comment 3

[Frédéric Buclin]

Comment on attachment 201131 [details] [diff] [review]
patch against current cvs for fix bug issue

>-                         size="20" maxlength="20">
>+                         size="20" maxlength="20" />

Is this valid HTML 4.01 Transitional? We are not writing XHTML.

Comment 4

[apla]

> Is this valid HTML 4.01 Transitional? We are not writing XHTML.

most browsers (in my opinion) show correct closed tags if closing slash prepended by space. i can edit my patch by using non-closed inputs

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 201131 [details] [diff] [review]
patch against current cvs for fix bug issue

http://validator.w3.org/ accepts the closing-slash notation for HTML 4.01 Transitional, so the patch is all right in this regard.
apla, this bug is about labels, so if you're doing unrelated stuff like the trailing-slash-thing, please say so :)

Your patch uses tabulator characters, which makes runtests.pl complain. Please convert them to spaces.

In the time tracking part of the template, why don't you add label tags to the table headers labeling the imput fields below them?

>           <tr>
>             <td align="right" valign="top">
>-              <b>CC:</b>
>+              <label for="cc"><b>CC:</b></label>

This produces illegal HTML if the bug's cc list is empty: in this case, there is no entity with a "cc" id the label may point at. So you need to make the labeling conditional.

>-              <input type="checkbox" name="removecc">Remove selected CCs
>+              <label for="removecc">
>+			    <input type="checkbox" name="removecc" id="removecc" />Remove selected CCs
>+			  </label>

Indentation is incorrect here, and in other places, too. See http://www.bugzilla.org/docs/developer.html#templates.

>@@ -399,7 +401,7 @@
>                 </b>
>               <td colspan="5">
>                 <input name="keywords" accesskey="k"
>-                       value="[% bug.keywords.join(', ') FILTER html %]" size="60">
>+                       value="[% bug.keywords.join(', ') FILTER html %]" size="60"/>

Please add a blank before the closing-slash to make the notation compatible with HTML 4.01 Transitional (see http://www.w3.org/TR/xhtml1/#C_2).

There are several other places in your patch affected by this, too.

>@@ -479,9 +481,11 @@
>           [% PROCESS formattimeunit time_unit=bug.estimated_time - (bug.actual_time + bug.remaining_time) %]
>         </td>
>          <td align="center">
>-           <input name="deadline" value="[% bug.deadline %]"
>-                  size="10" maxlength="10"><br />
>-           <small>(YYYY-MM-DD)</small>
>+           <label for="deadline">
>+		     <input name="deadline" id="deadline" value="[% bug.deadline %]"
>+                  size="10" maxlength="10"/><br />
>+             <small>(YYYY-MM-DD)</small>
>+		   </label>

Letting the label include the object it labels leads to strange results sometimes. Please don't do that. There are other places in your patch with the same issue, too.

>+		  <input type="checkbox" name="reporter_accessible"
>+		    id="reporter_accessible" value="1"
>+            [% " checked" IF bug.reporter_accessible %]>Reporter

You've started adding closing-slashes to some input fields. Please cover all, or none :)

Comment 6

[Wayne Mery (:wsmwk)]

will label also make clicking "Remove selected CCs" toggle the check box?

Comment 7

[Marc Schumann [:Wurblzap]]

Bug 281691 is going to fix most of this.

(In reply to comment #6)
> will label also make clicking "Remove selected CCs" toggle the check box?

If a label is set appropriately, then yes. The current patch plus comment 5 does this.

Comment 8

[Marc Schumann [:Wurblzap]]

Fixed by bug 281691.