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.

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?

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

Remind me how this works? Like this? <label><input type="checkbox"...>Click this box</label> ? Gerv

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

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.

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

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

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

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

CCing Peter to show him we're working on some of the problems he found :-) This is a 2.16 blocker. Gerv

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 on attachment 62833 [details] [diff] [review] Patch v.A r=kiko No second review needed IMO

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

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.

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

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.

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

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

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.

>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.

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 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.

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

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.

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

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 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!

> 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 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

Refreshed patch. _Still_ looking for review :-) Gerv

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.

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 on attachment 70416 [details] [diff] [review] Patch v.4 Good enough.

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