281691 - Misused <label>'s cause screen reader problems

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Aaron Leventhal]

We should use a <label> to surrond the actual label for a control, but not the
control itself. The label for the below combo box should be surrounding only
"<u>H</u>ardware:" but not the entire <select>. The current code causes screen
readers to read the entire list of children for each combo box as you tab to it,
instead of the correct label.

    <td align="right">

        <b><u>H</u>ardware:</b>
      </td><td>
    <label for="rep_platform" accesskey="h">
      <select name="rep_platform" id="rep_platform">
          <option value="All">All
          </option>
          <option value="DEC">DEC
          </option>
          <option value="HP">HP
          </option>

          <option value="Macintosh">Macintosh
          </option>
          <option value="PC" selected>PC
          </option>
          <option value="SGI">SGI
          </option>
          <option value="Sun">Sun
          </option>
          <option value="Other">Other
          </option>
      </select>

    </label>
  </td>

Comment 1

[Aaron Leventhal]

Note that this occurs in more than one of our controls -- we need to look
through the code for this.

Comment 2

[Frédéric Buclin]

Taking! Very easy to fix. Patch coming in a few minutes.

Comment 3

[timeless]

STOP.

Comment 4

[GavinS]

(In reply to comment #3)
> STOP.

Please explain...

Why shouldn't the <label> be around the 'Hardware' text?

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> Why shouldn't the <label> be around the 'Hardware' text?

Gavin, I have discussed with timeless in IRC. I convinced (?) him that
everything was fine doing this.

I'm doing a bit more than what Aaron requested, which explains why my patch is
not already uploaded (I have actually modified 3 of the 4 files concerned by
Aaron's comment). Unfortunately, I haven't time today to finish my patch and I'm
away this week-end so I will submit it on monday. Gavin, I will ask you to review.

Comment 6

[Anne (:annevk)]

What we are doing is correct per HTML 4.01. I think we should report the bug to
the screen reader in question. Of course, we still might want to fix this, but
this is not really a bug.

Comment 7

[Aaron Leventhal]

(In reply to comment #6)
> What we are doing is correct per HTML 4.01. I think we should report the bug to
> the screen reader in question. Of course, we still might want to fix this, but
> this is not really a bug.

This is not a problem with the screen reader. That's like saying that missing
alt text on an image is a problem with a given screen reader :)  The MSAA/ATK
support in Mozilla simply cannot make up for a lack of information in the markup.

In this label case, we might be using technically correct HTML, but we're not
using it as it's intended or in any useful way. The <label> element is supposed
to signify where the label for the control is. In this case the label is
"Hardware", but we're wrapping the control itself. That's like saying <label
for="inp1"><input id="inp1" type="text"/></label>. Why would we do that? Let's
use the label element to indicate where the label is.

Comment 8

[Frédéric Buclin]

Attachment: work in progress

This patch is not ready for review yet and is incomplete, but is only here to
help anne (if needed).

Files concerned by this <label> stuff are :

bug/edit.html.tmpl
reports/create-chart.html.tmpl
reports/series-common.html.tmpl
search/form.html.tmpl

as well as template/en/default/filterexceptions.pl depending on how much you
alter these files.

Comment 9

[Frédéric Buclin]

I have more important bugs to fix first. Reassigning to anne, per IRC discussion.

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Anne didn't reply all year, so I'm picking up Frédéric's work.

Comment 11

[Marc Schumann [:Wurblzap]]

This fixes bug 313728, too.

Comment 12

[Anne (:annevk)]

Thanks. Aaron, it is per design that controls can be inside <label> elements.

Comment 13

[Aaron Leventhal]

(In reply to comment #12)
> Thanks. Aaron, it is per design that controls can be inside <label> elements.

You're probably right -- we should handle that case better, but in that case isn't the actual label's text also supposed to be inside the label?

If the only thing inside the <label> is the control, that makes no sense. We should use <label> for HTML selects, but in a way that contributes to the semantics of the UI and is helpful to screen reader users and clickers.

    <label for="[% sel.name %]" accesskey="[% sel.accesskey %]">
      <select name="[% sel.name %]" id="[% sel.name %]"
              multiple="multiple" size="[% sel.size %]">

For example, in the case of 
Version: [ unspecified (V) ] 

The text "Version:" should be inside the <label>

Comment 14

[Marc Schumann [:Wurblzap]]

(In reply to comment #13)
> The text "Version:" should be inside the <label>

...which is what the current patch does.

Comment 15

[Anne (:annevk)]

(In reply to comment #13)
> You're probably right -- we should handle that case better, but in that case
> isn't the actual label's text also supposed to be inside the label?

Yeah, both the label text and the control.

Comment 16

[Aaron Leventhal]

(In reply to comment #14)
> (In reply to comment #13)
> > The text "Version:" should be inside the <label>
> 
> ...which is what the current patch does.
> 

I think you have to specify the name of a specific reviewer in order to get a response usually. I'm not sure who reviews bugzilla code changes.

Comment 17

[Frédéric Buclin]

Comment on attachment 205823 [details] [diff] [review]
Patch

>Index: template/en/default/bug/edit.html.tmpl

2 out of 12 hunks FAILED -- saving rejects to file template/en/default/bug/edit.html.tmpl.rej

There are several places where the code changed. I will let you find where exactly. ;)


> [% BLOCK select %]
>   <td>
>+    <select id="[% selname %]" name="[% selname %]" id="[% selname %]">

The ID attribute is set twice.

I didn't look further, due to bitrot (I couldn't apply the patch).

Comment 18

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Unrotted, and comment fixed.

Comment 19

[Frédéric Buclin]

Comment on attachment 211842 [details] [diff] [review]
Patch

>+++ ./template/en/default/bug/edit.html.tmpl	2006-02-14 09:55:18.916622400 +0100

misplaced or missing <label> for:
- alias
- CC
- Remove selected CCs
- flags in flag/list.html.tmpl
- fields related to the timetracking group
- Private (for both new and existing comments)
- group descriptions
- Reporter accessible
- CC list accessible



>+++ ./template/en/default/bug/summarize-time.html.tmpl	2006-01-14 00:08:46.794563200 +0100

- 'Period starting' should be completely between <label> </label> tags
- same remark for 'and ending'
- missing label for 'Format'



>+++ ./template/en/default/reports/create-chart.html.tmpl	2005-12-14 12:40:02.339708500 +0100

missing <label> for:

- Cumulate
- Date range
- to



>+++ ./template/en/default/search/form.html.tmpl	2005-12-30 16:10:06.754771200 +0100

You forgot to remove "accesskey => 'o'" for the OS (it appears twice: in the <label> and in the <select>)

missing <label> for:

- Only bugs with at least
- bugs numbered
- Sort results by
(- maybe fields in the Bug Changes frameset)

Comment 20

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.1

Addressed comments.

Comment 21

[Frédéric Buclin]

This patch is too invasive for 2.22.

Comment 22

[Frédéric Buclin]

Comment on attachment 216782 [details] [diff] [review]
Patch 1.1

>Index: template/en/default/bug/edit.html.tmpl

>           <tr>
>             [% IF bug.cc %]
>+              <td align="right" valign="top">
>+                <label for="cc"><b>CC</b></label>:
>+              </td>
>+              <td valign="top">
>+              <select id="cc" name="cc" multiple="multiple" size="5">
>               [% FOREACH c = bug.cc %]
>                 <option value="[% c FILTER html %]">[% c FILTER html %]</option>
>               [% END %]
>               </select>
>               <br>
>+              <input type="checkbox" id="removecc" name="removecc">
>+              [%%]<label for="removecc">Remove selected CCs</label>
>               <br>
>             [% ELSE %]
>+              <td colspan="2"><input type="hidden" name="cc" value=""></td>
>             [% END %]
>             </td>
>           </tr>

This last </td> is misplaced. it must be moved into the IF block.


>+             <label for="qa_contact" accesskey="q"><b>
>+               <u>Q</u>A Contact
>+             </b></label>:

Having ":" not appended to "QA Contact" makes it to appear on its own line when viewing a bug (with my screen resolution and font size). This problem appears in several other places too. That's the main reason of my r-.


>+              <label for="bug_file_loc" accesskey="u"><b>
>                 [% IF bug.bug_file_loc 
>                    AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
>                   <a href="[% bug.bug_file_loc FILTER html %]"><u>U</u>RL</a>:

Remove ":" after URL, you added it after </label> already.




>Index: template/en/default/reports/create-chart.html.tmpl

>+          <label for="datefrom"><b>Date Range</b></label>:
>+          <input type="text" size="12" name="datefrom"
>             value="[% time2str("%Y-%m-%d", chart.datefrom) IF chart.datefrom%]">

Missing id="datefrom" in <input>.


>+          <label for="dateto"><b>to</b></label>
>+          <input type="text" size="12" name="dateto"
>             value="[% time2str("%Y-%m-%d", chart.dateto) IF chart.dateto %]">

Missing id="dateto" in <input>.

Comment 23

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Addressing review comments.

Fixing the colon-line-break thing yields rather long lines, but it was either this or using [%%] extensively. I don't like either solution very much.

While fixing the misplaced </td>, I corrected indentation, too.

Comment 24

[Frédéric Buclin]

Comment on attachment 218340 [details] [diff] [review]
Patch 1.2

>Index: template/en/default/bug/edit.html.tmpl

>-                <b>Alias:</b>
>+                <label for="alias" title="a name for the [% terms.bug %] that can be used in place of its ID number, f.e. when adding it to a list of dependencies"><b>Alias</b></label>:


Looks good. You know, instead of having these long lines, you could as well include ":" in labels. This would be a lot cleaner. I don't understand why you didn't do it, btw.

If you want to fix it to avoid these long lines, please do. In all cases, r=LpSolit (I didn't check that you caught all misused <label>'s)

Comment 25

[Marc Schumann [:Wurblzap]]

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.68; previous revision: 1.67
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v <--  comments.html.tmpl
new revision: 1.26; previous revision: 1.25
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.74; previous revision: 1.73
done
Checking in template/en/default/bug/summarize-time.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/summarize-time.html.tmpl,v  <--  summarize-time.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/flag/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/flag/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.20; previous revision: 1.19
done
Checking in template/en/default/reports/create-chart.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v  <--  create-chart.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/reports/series-common.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series-common.html.tmpl,v  <--  series-common.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v <--  form.html.tmpl
new revision: 1.37; previous revision: 1.36
done
Checking in template/en/default/search/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/knob.html.tmpl,v <--  knob.html.tmpl
new revision: 1.17; previous revision: 1.16
done

(In reply to comment #24)
> Looks good. You know, instead of having these long lines, you could as well
> include ":" in labels. This would be a lot cleaner. I don't understand why you
> didn't do it, btw.

If the label were just an extended area for the mouse to click upon, I wouldn't care. As I understand it, though, screen readers use label text as a kind of title for the control. As such, the colon is no part of the label from my interpretation of things... I may be wrong.

Comment 26

[timeless]

for reference. this is **** me off.