110711 - Checkboxes in new query.cgi should use LABEL and other new query.cgi problems

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Matthew T (active 1999-2002)]

To reproduce:
1.  Go to <http://bugzilla.mozilla.org/attachment.cgi?id=50517&action=view>.
2.  Click on the word `commenter'.
3.  Note that the checkbox next to `commenter' is toggled.
4.  Go to <http://landfill.tequilarista.org/bbaetz/query.cgi>.
5.  Click on the word `commenter'.

What should happen:
*   The checkbox next to `commenter' is toggled.

What actually happens:
*   It's not.

Comment 1

[Stephen Rasku]

I am not seeing the desired behaviour at either URL.  I am using Netscape 4.72
on Sparc Solaris.  Is this something that is only supported on newer browsers?

Comment 2

[Matthew T (active 1999-2002)]

Yes. And the answer to your next question is yes, it will still work fine in 4.x.

Comment 3

[Gervase Markham [:gerv]]

Remind me how this works? Like this?

<label><input type="checkbox"...>Click this box</label>

?

Gerv

Comment 4

[Matthew T (active 1999-2002)]

Yes, as demonstrated in the HTML source of the original design. :-P

Comment 5

[Tim Powell]

Actually, the best way to do this is with label for and id. For example, <input 
type="checkbox" id="1"><label for="1">Click this box</label>. If you do it this 
way, IE4 and 5 will also support clicking the checkbox label. Yeah, it's a 
little bit uglier because you have to create potentially extra ids, but the 
usability improvement is worth it.

Comment 6

[Gervase Markham [:gerv]]

I seem to remember I tried that method and it didn't work in Mozilla...

Gerv

Comment 7

[Tim Powell]

It works fine in Mozilla. I use it all the time.

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This patch fixes bug 110708, bug 110709, bug 110710 and bug 110711 (mostly.) It
also fixes another issue that jband spotted - incorrect default value for email
matching - which doesn't have a bug number.

Is that yer lot? :-)

Review wanted.

Gerv

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.A

Oh - and keywords is broken. Well spotted Peter; sorry I didn't believe you
first time. <looks embarrassed>

Gerv

Comment 10

[Gervase Markham [:gerv]]

CCing Peter to show him we're working on some of the problems he found :-)

This is a 2.16 blocker.

Gerv

Comment 11

[Matthew T (active 1999-2002)]

I did so file a bug on incorrect default value for e-mail matching, bug 110712.
I'm not as think as you drunk I am, y'know. Marking dependencies.

Comment 12

[Christian Reis]

Comment on attachment 62833 [details] [diff] [review]
Patch v.A


r=kiko

No second review needed IMO

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 61795 [details] [diff] [review]
Patch v.1


>+      <label>
>+        <input type="checkbox" name="emailassigned_to[% n %]" value="1"
>+          [% " checked" IF default.emailassigned_to.$n %] />bug owner
>+      </label>

>+      <label>
>+        <input type="checkbox" name="emailreporter[% n %]" value="1"
>+          [% " checked" IF default.emailreporter.$n %] />reporter
>+      </label>

>+      <label>
>+        <input type="checkbox" name="emailqa_contact[% n %]" value="1"
>+          [% " checked" IF default.emailqa_contact.$n %] />QA Contact
>+      </label>

>+      <label>
>+        <input type="checkbox" name="emailcc[% n %]" value="1"
>+          [% " checked" IF default.emailcc.$n %] />CC list member
>+      </label>

>+      <label>
>+        <input type="checkbox" name="emaillongdesc[% n %]" value="1"
>+          [% " checked" IF default.emaillongdesc.$n %] />commenter
>+      </label>

Please use <label for="foo"><input id="foo"> on the above as suggested in
comment 5

> <dl>
>   <dt>Only bugs changed in the last </dt>
>   <dd><input name=changedin size=3 value="[% default.changedin.0 FILTER html %]" /> days</dd>

What about a label on "Only bugs changed in the last" ?  There are a few other
places where label 
could be used as well, but if we want to keep this only for the checkboxes (as
the summary says)
than fix the labels to use the explicit label as noted above, and r=caillon

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Gerv,

You could go ahead and check in Attachment 62833 [details] [diff], since kiko gave it first
review with no second review needed, but how about rolling it up into the next
version of your overall patch for query.cgi problems (i.e. the successor to
attachment 61795 [details] [diff] [review]).  That'll reduce confusion for newcomers to this bug.

Comment 15

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Here's v.2. It incorporates Patch v.A. I've fixed the labels as caillon
suggested, although I'm confused about what he says re: "Only bugs changed in
the last", because it's not a checkbox...

Gerv

Comment 16

[Tim Powell]

Gerv, label tags are designed to improve accessibility for disabled users. In 
general, clicking a label gives focus to the field (works in IE but not 
Mozilla, bug 28657). A nice side-effect of this is that it toggles 
radio/checkboxes. When a screen reader reads a page that uses labels it is able 
to read the associated field label with the field. This is most useful when 
tabbing through fields to complete a form. For example, "text input field - 
Only bugs changed in the last" would be the result with the proposed label. 
Obviously not ideal. I'd suggest having a different bug to deal with 
accessibility of bugzilla and keep this specific to checkboxes/radioboxes.

I'm not sure whether the nesting of input in the label will cause problems. In 
your patch you have:

+      <label for="emaillongdesc[% n %]">
+        <input type="checkbox" name="emaillongdesc[% n %]" 
+               id="emaillongdesc[% n %]" value="1"
+               [% " checked" IF default.emaillongdesc.$n %] />commenter
+      </label>

I would have thought that would be:
+        <input type="checkbox" name="emaillongdesc[% n %]" 
+               id="emaillongdesc[% n %]" value="1"
+               [% " checked" IF default.emaillongdesc.$n %] />
+      <label for="emaillongdesc[% n %]">commenter</label>

The way you have it, I'd think there'd be some blank space around the input 
field that is also part of the label. If it works in IE and Moz, it's no big 
deal.

Comment 17

[Gervase Markham [:gerv]]

It works in Moz; I can't easily try it in IE. Can someone else, please?

Gerv

Comment 18

[Gervase Markham [:gerv]]

This patch is still suitable for review. The preceding comments have not led me
to change it :-)

Gerv

Comment 19

[Bradley Baetz (:bbaetz)]

Comment on attachment 64373 [details] [diff] [review]
Patch v.2

>Index: defparams.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v
>retrieving revision 1.63
>diff -u -r1.63 defparams.pl
>--- defparams.pl	30 Oct 2001 23:42:22 -0000	1.63
>+++ defparams.pl	10 Jan 2002 22:09:00 -0000
>@@ -458,7 +458,7 @@
> DefParam("defaultquery",
>           "This is the default query that initially comes up when you submit a bug.  It's in URL parameter format, which makes it hard to read.  Sorry!",
>          "t",
>-         "bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&order=%22Importance%22");
>+         "bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailqa_contact2=1&order=%22Importance%22");

I don't like this. Firstly, its not going to affect existing installations.
Secondly, its a hack ;)

You should rather just default to this if there is nothing else given as a
param. Or would that stuff up
the "edit this query" stuff?


>     [% END %]
>-      <p>
>+      <div align="right">

I don't like this. I read the bug it depends on, and I don't really find the
argument convincing.

Besides, this puts it in at the right of the _table_, and so roughly in the
center of the screen anyway.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

>I don't like this. Firstly, its not going to affect existing installations.

True, but that's part of life with Bugzilla.  File a bug on me to fix it on
b.m.o. once the patch goes in.


>Secondly, its a hack ;)

No, it's the right way of doing things, since the "defaultquery" parameter is
how selections on the query page get determined in Bugzilla (unless a specific
query is being edited or the user has their own default).


>You should rather just default to this if there is nothing else given as a
>param. Or would that stuff up the "edit this query" stuff?

That's exactly what happens.  First query.cgi checks for a query being edited
(i.e. presence of form/URL parameters), then it checks to see if the user has a
default query, then it uses the global default.

Comment 21

[Gervase Markham [:gerv]]

Myk's answered your first point. As for your second, it's in the _logical_
bottom right corner this way. In the actual bottom-right corner would look silly
on wide monitors. I like it where it is, aesthetically.

Someone remind me of the bug number about this? :-)

Gerv

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 64373 [details] [diff] [review]
Patch v.2

>Index: template/default/query/query.atml
  ...
>-    <td><input type="checkbox" name="emailassigned_to[% n %]" value="1"
>-        [% " checked" IF default.emailassigned_to.$n %] />bug owner</td>
>+    <td>
>+      <label for="emailassigned_to[% n %]">
>+        <input type="checkbox" name="emailassigned_to[% n %]" 
>+               id="emailassigned_to[% n %]" value="1"
>+               [% " checked" IF default.emailassigned_to.$n %] />bug owner
>+      </label>
>+    </td>

Note that this usage of label contains redundant assocations between labels
and controls since the HTML specification defines both explicit ("for" 
attribute) and implicit (containment of control within contents of label) 
associations between labels and controls and requires only one or the other.

The spec doesn't say you can't use both, but all its examples use just one,
and a single method is more maintainable in the long run, so we should use
one or the other.  My preference is explicit association because it is more
flexible, permits more consistency, is more fault-tolerant, and is more 
similar to the XUL way of doing things.


>-      <p>
>+      <div align="right">

This is bug 110710 which like bbaetz I disagree with for usability reasons.
Let's separate this issue from the rest and tackle it separately in that bug.

Comment 23

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Version 3 addresses Myk's comments, removed the right-alignment and also turns
<br> into <br /> throughout. Ready for re-review.

Gerv

Comment 24

[Matthew Tuck [:CodeMachine]]

Gerv I think we should avoid all these mega patches.  For the original
templatisation it might have been necessary, but for bug it is a lot harder to
review than it would have been separately.

Comment 25

[Jeff Lawson]

*** Bug 118774 has been marked as a duplicate of this bug. ***

Comment 26

[Gervase Markham [:gerv]]

This is hardly a mega-patch - it's bloated a touch by some reformatting, but the
actual code changes are minimal. For a person who reviewed query.cgi/tmpl in the
first place, I think this is ideal.

Gerv

Comment 27

[Christian Reis]

Comment on attachment 67132 [details] [diff] [review]
Patch v.3

Okay, only a few nits you can take into account or not, except for the bottom
items in the form, which are really missing label.

>-$vars->{'have_keywords'} = scalar(%::legal_keywords);
>+$vars->{'have_keywords'} = scalar(@::legal_keywords);

I guess this has been broken for a while? :-)

>Index: template/default/query/query.atml
[...]
>     </td>
>     <td>
>-      <input name="short_desc" size="30" value="[% default.short_desc.0 FILTER html %]" />
>+      <input name="short_desc" size="40" value="[% default.short_desc.0 FILTER html %]" />
>     </td>
>     <td>
>       <input type="submit" value="Search" />
>     </td>
>   </tr>  
>-</table>
> 
> [%# *** Product Component Version Target *** %]
> 
>-<table>

Why are you joining the tables here? We don't gain a lot with this and it
makes the first cell spread out to the right. I don't care too much, but I do
think this is a change for the worse. I'm not saying right-alignment of the
label is wrong,I'm saying in this case there is nothing to be gained by 
spreading out the cell to the right; just makes controls further apart.

*** Oh. I see it now. Oh, okay, we are aligning all of the cells, and before we
were ignoring the top one. That's fine then. I'll leave that bit in just 
if anybody else has the same question.

>+  [% FOREACH field = [ 
>+    { name => "long_desc", description => "A&nbsp;comment" },
>+    { name => "bug_file_loc", description => "The&nbsp;URL" },
>+    { name => "status_whiteboard", description => "Whiteboard" } ] %]
>+
>+    [% UNLESS field.name == 'status_whiteboard' AND NOT Param('usestatuswhiteboard') %]

I find this unless clause really hard to understand, but I guess that's me
not knowing perl :) It can't be rewritten as a simpler statement by making the
check before the interation and adding it to the list or not?

>+      <td><input name="[% field.name %]" size="40" value="
>+        [% default.${field.name}.0 FILTER html %]" />
>+      </td>
>+      <td></td>
>+    </tr>  

Any reason why we have an empty td here (yes, I know you are taking into
account the search button up there)? You could add an &nbsp; into it
to avoid ns4 not rendering the cell, perhaps.


>+      <input type="checkbox" name="emailassigned_to[% n %]" 
>+             id="emailassigned_to[% n %]" value="1"

Question: is having name == id ever a problem? Just wondering.

Ok. Now for the real problem:

<input type="radio" name="cmdtype" value="doit" checked /> Run this query
<input type="radio" name="cmdtype" value="editnamed" />
<input type="radio" name="cmdtype" value="runnamed" />
<input type="radio" name="cmdtype" value="forgetnamed" />
<input type="radio" name="cmdtype" value="asdefault" />
<input type="radio" name="cmdtype" value="asnamed" />
<input type="checkbox" name="tofooter" value="1" />

These still lack <label>! boo!

Comment 28

[Gervase Markham [:gerv]]

> I guess this has been broken for a while? :-)

As you say. Kiko - lots of your comments are reasonably tiny things, and this
patch is a bug fix (some Bugzillas have been running with no keyword searching
for a while) and needs to go in ASAP. Which of your review comments really need
addressing? The <label> stuff is definitely an RFE.

Gerv

Comment 29

[Gervase Markham [:gerv]]

Comment on attachment 67132 [details] [diff] [review]
Patch v.3

This is silly. There is nothing in this _bug_fixing_ patch which requires work.
It's perfectly acceptable as-is. I am not going to succumb to feature creep.

Gerv

Comment 30

[Gervase Markham [:gerv]]

Attachment: Patch v.4

Refreshed patch. _Still_ looking for review :-)

Gerv

Comment 31

[Bradley Baetz (:bbaetz)]

Comment on attachment 70416 [details] [diff] [review]
Patch v.4

Tested in moz and ns4. It looks ugly in ns4, but your patch didn't change that

r=bbaetz. Note to second reviewer - a cvs diff -w is much easier to see all the
changes.

Comment 32

[Matthew Tuck [:CodeMachine]]

It might not be a mega-patch in the sense of size, but in the sense of dealing
with multiple issues it is.  This would have been in quicker if you had have
stuck to one bug per patch, and I don't see any reason you couldn't have here.

It is not always easy to cross reference what is going on on the patch with what
you're trying to fix when you do it like that.  The first time I tried to review
this I gave up, because I didn't have lots of time.  If it was split, I could
have done reviews on bits.

Comment 33

[Matthew Tuck [:CodeMachine]]

Comment on attachment 70416 [details] [diff] [review]
Patch v.4

Good enough.

Comment 34

[Gervase Markham [:gerv]]

Checked in. Thanks, people :-)

Checking in template/default/query/query.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/query/query.atml,v  <-- 
query.atml
new revision: 1.6; previous revision: 1.5
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.66; previous revision: 1.65
done

Gerv