450301 - Simplify searching UI

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P1)

Description

[Max Kanat-Alexander]

The survey reports that users are commonly confused with the Advanced Search screen. Some items reported were:

* Interaction between boolean charts and rest of search is confusing
* “contains the string” etc. is confusing to users

We should do some research and mockups to figure out the best way to keep the power of search while improving the UI.

Comment 1

[Reece H. Dunn]

For the "contains the string"-style options, wouldn't it be better to say something like "contains the phrase".

The idea here is that 'string' in this context is understood by people with a programming background but is likely to confuse other users such as the people trying the software and reporting the bugs, whereas 'phrase' is more widely understood and means the same thing.

NOTE: Google use the word "phrase" in place of "string" in their advanced search.

Comment 2

[Max Kanat-Alexander]

The solution that I came up with while talking to pyrzak about it (and I think it may have even been spawned originally from an idea he had) was to basically make the Simple Search UI something like a clone of Thunderbird's "filters" UI, where it displays the fields that you can search on, then limits the "search type" options based on the field you chose, and also modifies the "value" option based on those two items. Then there's a +/- next to each row to add or remove another item, and a chooser for "all of the above" or "any of the above" at the top of the list.

This would all be below a basic search box, which would be a QuickSearch.

Comment 3

[Guy Pyrzak]

Attachment: Concept for collapseable version of advanced search UI

A UI based on the Search page we implemented at work. This UI collapses the "scary" advanced search page fields behind widgets that can be expanded. 

The other main improvement is a 3 column grid UI which make the search page more approachable. 

I've also thought about replacing the multiselects with autcomplete widgets. Not sure about that one yet. feel free to give thoughts etc.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 427030 [details]
Concept for collapseable version of advanced search UI

Okay, this is good. Here's a few thoughts:

* Instead of Summary, some people have suggested that that box simply be a Quicksearch box, which would be fully consistent with the other Search boxes that people see in Bugzilla. It might be a good idea to keep that consistency and make every search box a Quicksearch box. The only confusing bit here would be that Quicksearch also searches products, etc. by default, so you might get more products/components/etc. than you actually selected, which I wouldn't like. One possible solution is to modify the QuickSearch code so that if there are "product" (etc.) options in the search URL already, we don't also include the quicksearch terms when we search for products (etc.). If we did this, we could have the Summary in a lower section, with Comments.

* If we change the Status box to working just like the simple-search status box, and then put the full box lower down, we could eliminate the simple-search page. We could possibly also include "Fixed" as an option in the drop-down, eliminating the need for a Resolution select in some of the most common cases.

* Searching by Bug IDs may not be all that important if we use QuickSearch for the top box.

* We should use our TUI library to remember which sections people expanded.

Comment 5

[Guy Pyrzak]

Attachment: Expanded version of the previous mock

Comment 6

[Max Kanat-Alexander]

Great! A few comments on the expanded mock:

* I'm not sure that the boxes for comments etc. need to be textareas. I think that the UI that we have for them now is actually OK, so we can pretty much keep that, just hide it.

* I think that the text fields should probably be in their own section, and the select fields should be in their own section.

* If we're going to have three People boxes, then they should have different boxes checked by default. Perhaps reporter, cc list, and assignee, respectively.

* "Any of the fields" doesn't look like a select for some reason that I'm not sure of, in the mock.

* The new boolean chart stuff will stay in the separate bug that we have it in now.

Comment 7

[Guy Pyrzak]

(In reply to comment #6)
> Great! A few comments on the expanded mock:
> 
> * I'm not sure that the boxes for comments etc. need to be textareas. I think
> that the UI that we have for them now is actually OK, so we can pretty much
> keep that, just hide it.
> 
> * I think that the text fields should probably be in their own section, and the
> select fields should be in their own section.
> 
> * If we're going to have three People boxes, then they should have different
> boxes checked by default. Perhaps reporter, cc list, and assignee,
> respectively.

I used a textarea b/c it helped re-inforce the grid, i can change them to inputs. 

I'm not sure if it will make sense to users if we group the fields based on selects and text fields, since that seems kinda random versus the current UI that separats the fields by stuff that classifies the bug and stuff that describes where/how the bug happened

> * "Any of the fields" doesn't look like a select for some reason that I'm not
> sure of, in the mock.
> 
blame ScreenGrab (the firefox extension), Any of the fields is a normal multiselect

> * The new boolean chart stuff will stay in the separate bug that we have it in
> now.
Yeah, sorry that's just a copy paste from our current UI.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> I'm not sure if it will make sense to users if we group the fields based on
> selects and text fields, since that seems kinda random versus the current UI
> that separats the fields by stuff that classifies the bug and stuff that
> describes where/how the bug happened

  Okay. I think the "details" and "metadata" categories that exist in the mockup are confusing, actually. For example, Version, OS, Platform, and Target all sound like "metadata" to me. I think that all of the selects actually do logically group together. After that, I think "comments and summary" group together, then "keywords and whiteboard" group together (at least at Mozilla, though every company uses the Whiteboard in different ways), and then "URL" is the odd man out (and is going to become an extension or custom field, anyway, *and* probably also isn't something people want to search on very often).

> blame ScreenGrab (the firefox extension), Any of the fields is a normal
> multiselect

  Ahh, okay. :-)

> > * The new boolean chart stuff will stay in the separate bug that we have it in
> > now.
> Yeah, sorry that's just a copy paste from our current UI.

  Oh, no need to apologize, I knew that. :-) I just wanted to make it clear for everybody. :-)

Comment 9

[Max Kanat-Alexander]

  One other thought--are "was changed after" and "was changed before" actually accurate? I think that those are >= and <= searches, so they also include the exact time you input into them.

Comment 10

[Guy Pyrzak]

Jesse Ruderman suggested the same solution with the selects, but he also suggested just removing the text fields completely and letting people use the Boolean charts. See my blog for more details.

Comment 11

[Max Kanat-Alexander]

  Okay. I think that the text fields should stay--the boolean charts are too overwhelming to the average user. They could be made simpler, but that would be a separate bug that might not be as-high priority.

Comment 12

[Max Kanat-Alexander]

By the way, making quicksearch the top box will involve a bit of re-work that I won't be available to do soon, so we should go ahead with the redesign with Summary at the top, and work on the "quicksearch at the top" or any other improvements later.

Comment 13

[Guy Pyrzak]

comment from my blog post. 

Too much vertical/horizontal space is taken up by whitespace/labels. Advanced search should attempt to maintain a good use of information density as the current UI

Comment 14

[Guy Pyrzak]

Attachment: V1

Here is the first version of the patch. I know we need to move the JS and the CSS into another file but I'm not sure how you want to handle it since the search/form.html.tmpl is included in 4 other templates: 
search-advanced.html.tmpl
search-create-series.html.tmpl
search-report-graph.html.tmpl
search-report-table.html.tmpl

I also need to test the css on Windows browsers, I'm pretty sure there is a bunch of stuff I'll have to do to make it work.

We also will need to combine the TUI that I wrote for this page with the TUI that was written for the create page.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 433868 [details] [diff] [review]
V1

>=== modified file 'template/en/default/search/boolean-charts.html.tmpl'
>+    Advanced Searching Using Boolean Charts <span style="font-size: 0.8em; font-weight: normal">

  Ahhh! style= is the devil! :-D

> Didn't find what you're looking for above? This area will give you more power than Superman</span>

  That's cute. I don't know if people are going to complain about its cuteness. I will complain, at this very moment, that it lacks a period at the end of the sentence, though.

>         [% INCLUDE "search/type-select.html.tmpl"
>            name = "type${chartnum}-${rownum}-${colnum}",
>-           types = types, selected = col.type %]
>-
>-        <input name="[% "value${chartnum}-${rownum}-${colnum}" %]" 
>-               value="[% col.value FILTER html %]"> 
>+           types = types, selected = col.type 
>+           field = { name => "value${chartnum}-${rownum}-${colnum}" , default => col.value }
>+           %]

  Before I forget: Instead of re-using type-select.html.tmpl, you should probably create a search/field.html.tmpl and add just FREETEXT-type fields, for now. Then you could include the labels.

>=== modified file 'template/en/default/search/form.html.tmpl'
>+<style type="text/css" media="screen">

  Yeah, this should go into the header of everywhere that includes this form.

>+  #summary_field {
>+    margin: 0 0 0 0;

  That can just be one 0.

>+  #summary_field label {
>+    margin: 0 2em 0 0;

  That can just be margin-right: 2em;

>+  .arrow {
>+    display: inline-block;
>+    background: url('skins/standard/yui/sprite.png');
>+    background-position:-10px -855px;

  I don't think we can guarantee the sprites staying in one place across multiple versions of YUI, no?

  It's possible you could just use a glyph. There are appropriate glyphs in Unicode, I think. Perhaps one of the triangles here: http://www.alanwood.net/unicode/geometric_shapes.html

>@@ -103,6 +194,94 @@
>+YAHOO.bugzilla.TUI = function(){
> [snip]

  As I mentioned on IM, you can re-use the existing TUI and just do something to modify _TUI_toggle_control_link.

  Also, I wouldn't get too cozy with YAHOO.bugzilla, since it will probably disappear with YUI 3.

>@@ -122,383 +301,154 @@
>-      <input name="short_desc" id="short_desc" size="40"
>-             value="[% default.short_desc.0 FILTER html %]">
>+         types = query_types, selected = default.short_desc_type.0 
>+         field = { name => "short_desc" , default => default.short_desc.0, 
>+                    label=> "Summary" , access=> "s" }
>+         %]

  And when you do make this its own template, you should pass in real field objects, from bug_fields.


>+<div id="detailed_information"class="bz_section_title">

  Nit: Space before class=.

>+  <div class="arrow">&nbsp;</div><a href="#">Detailed [% terms.Bug %] Information</a><span>Narrow results by the following fields: 
>+    Comments, Url, [% ' Whiteboard, ' IF Param('usestatuswhiteboard') %]
>+    [% ' Keywords, ' IF use_keywords%][% ' Deadline, ' IF user.is_timetracker %][% terms.Bug %] Numbers, Version, 
>+    [% ' Target, ' IF Param('usetargetmilestone') %] Severity, Priority, Hardware, OS</span>

  Nit: 80-character lines.

  Also, you should be using field_descs.

>+  <div class="search_field_row" style="vertical-align: top;">

  style=

>+           <div style="font-size:0.75em">(comma-separated list)</div>

  style= (I assume that these are just all here temporarily and will move out for the final version.)

>+     <select name="bug_id_type">
>+       <option value="anyexact"[% " selected" IF default.bug_id_type.0 == "anyexact" %]>only included in</option>
>+       <option value="nowords"[% " selected" IF default.bug_id_type.0 == "nowords" %]>excluded from</option>
>+     </select> the results

  I don't think nowords is an important-enough use case to offer this select.

>+  <div class="bz_section_title" id="people_filter"><div class="arrow">&nbsp;</div><a href="#">People Filter</a><span>Narrow results to a role (ie. assignee, reporter, commenter, etc.) a person has on a [% terms.bug %]</span></div>

  Nit: Very long single line.

  BTW, I prefer your "Search by" to "Filter". I think "Filter" is gibberish.

>+  <div id="people_filter_section" class="bz_search_section">
>+  [% FOREACH n = [1, 2, 3] %]
>+    <div class="search_email_fields">
>+      [% PROCESS role_types field = { count => n, name => "emailassigned_to",
>+                  label=> "the ${terms.bug} assignee" } %]
> [snip]

  We talked about this on IM a while back, but I'd like to see each of these default to having a different thing checked. Is that controlled by the .cgi?


>@@ -506,78 +456,25 @@
>         { name => "notequals", description => "is not" },
>         { name => "regexp", description => "matches regexp" },
>         { name => "notregexp", description => "doesn't match regexp" } ] %]

  Huh, shouldn't we be using type-select there? I guess that's a separate bug, though.

>+    [%# turns out values is a function for hashes so we use vals instead %]
>+    [% IF NOT sel.vals %]
>+      [% sel.vals = sel.name %]
>+    [% END %]

  I think you mean ${sel.name} there.

  You could also just have a "values" variable, you don't have to add it to sel.

>+        [% FOREACH value = ${sel.vals} %]

  You just grabbed the value of an array named something like ARRAY(0x23908d87). I don't think there's a template variable available with that name.... :-)

>+            [% v = value.name OR '---' -%]

  Does that work?

  Also, you're excluding the possibility of "0" being a value, which it could be.

>+              [% " selected" IF lsearch(default.${sel.name}, v) != -1 %]>

  Use the "contains" vmethod instead of the lsearch function.

>+              [% " selected" IF lsearch(default.${sel.name}, value) != -1 %]>

  Same comment there.


  I haven't applied the patch and looked at it yet, but I from looking at the code, I like what you did with the CSS and making the layout into divs--it seems really clever. :-) Does it work in IE6?

Comment 16

[Guy Pyrzak]

Yup I'm going to remove all the inline style stuff. And a lot of the other stuff you commented on was actually part of the old form.html.tmpl. 

So far most of the stuff you commented on is what I'd call "clean up" or was already in the code before i touched it. But it seems like the big take away are:
1. Use a glyph instead of the sprites.
2. Move the field output stuff into its own template
3. Update TUI as it exists to handle use case that I am using.
4. Make sure the 3rd people area has defaults.
5. Fix up this page to use field_descs
6. Use "Search By" instead of "Filter". 

I didn't want to use Search By because this form is also used on other pages like the graphs etc which are not "searches" per se. You'll notice i purposefully avoid using the word search anywhere on the page. But if that doesn't bother you, i'm happy to use the word search.

I'll test it on IE6, IE7, etc and let you know how it works. But it sounds like there are no huge show stoppers here. I'll of course take care of the style stuff and the nits

Comment 17

[Max Kanat-Alexander]

  Yeah, that all sounds right. You don't have to switch to using field_descs for anything you don't touch, although I'm not sure that's anything. :-)

  Yeah, I think I still prefer "Search by", using pretty much the exact text you had in the original mockup. I think it will make sense in the reporting and charting contexts OK.

Comment 18

[Guy Pyrzak]

Attachment: V2

I did everything except the TUI and the glyph stuff. Those two will have to wait but to be fair this was a bunch of work by itself.

I'll try to get the TUI stuff done but wow, this was a lot

Comment 19

[Frédéric Buclin]

Comment on attachment 447072 [details] [diff] [review]
V2

>=== modified file 'Bugzilla/Search.pm'

>-    foreach my $id ("1", "2") {
>+    foreach my $id ("1", "2", "3") {

Nit: quotes are useless.



>=== modified file 'template/en/default/search/boolean-charts.html.tmpl'

> find what you're looking for above? This area will give you more power than Superman</span>

I'm really not a fan of this kind of humor. It may be fun the first time you read it, but then it becomes annoying. Please remove this.



>=== modified file 'template/en/default/search/form.html.tmpl'

>+<style type="text/css" media="screen">

Please move CSS out of templates. We have skins/*.css for that (and not all skins have the same settings).


>+YAHOO.bugzilla.TUI = function(){

Also, move JS scripts in js/*.js. It's a pain for localizers to maintain code having a lot of JS code in it. Also, having it in an external JS script gives us a chance to cache the JS file in the browser.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 447072 [details] [diff] [review]
V2

>=== modified file 'Bugzilla/Field.pm'
>     {name => 'version',      desc => 'Version',    in_new_bugmail => 1,
>-     buglist => 1},
>+     type => FIELD_TYPE_SINGLE_SELECT, buglist => 1},

 You can't just do this without setting a bunch of other stuff. See the other patches where we changed standard fields to FIELD_TYPE_SINGLE_SELECT. (You can figure this out using loggerhead.)

>=== modified file 'template/en/default/search/boolean-charts.html.tmpl'
>-    Advanced Searching Using Boolean Charts
>+    Advanced Searching Using Boolean Charts <span style="font-size: 0.8em; font-weight: normal">Didn't find what you're looking for above? This area will give you more power than Superman</span>

  stttyyyyyyyyllllleeeeeee! :-)

  Also, didn't we decide that we were going to rename this to "Custom Search"?

>@@ -86,10 +86,9 @@
> 
>         [% INCLUDE "search/type-select.html.tmpl"
>            name = "type${chartnum}-${rownum}-${colnum}",
>-           types = types, selected = col.type %]
>-
>-        <input name="[% "value${chartnum}-${rownum}-${colnum}" %]" 
>-               value="[% col.value FILTER html %]"> 
>+           types = types, selected = col.type 
>+           field = { name => "value${chartnum}-${rownum}-${colnum}" , default => col.value }
>+           %]

  I don't understand why you're removing the input here--there's nothing in type-select that replaces it.

>=== added file 'template/en/default/search/field.html.tmpl'
>+  # The Initial Developer of the Original Code is Netscape Communications
>+  # Corporation. Portions created by Netscape are
>+  # Copyright (C) 1998 Netscape Communications Corporation. All
>+  # Rights Reserved.

  Is that true? I mean, I think you're really the original author here.

>+  [% BLOCK field_label %]

  I think you should re-use bug/field-label.html.tmpl (which is new).

>+  [% SWITCH field.type %]
>+    [% CASE [ constants.FIELD_TYPE_FREETEXT,
>+              constants.FIELD_TYPE_TEXTAREA, 
>+              constants.FIELD_TYPE_UNKNOWN ] %]

>+      <select name="[% field.name FILTER html %]">

  I don't think you want field.name as the name of the type select, without _type on it.

>+        [% FOREACH type = types %]
>+          <option value="[% type FILTER html %]"
>+                  [%- ' selected="selected"' IF type == default.${field.name}.0 %]>
>+              [%- search_descs.$type FILTER html %]
>+          </option>
>+        [% END %]
>+      </select>

  Why aren't you using the type-select template?

>+      <input name="[% field.name FILTER html %]" 
>+             id="[% field.name FILTER html %]" size="40"
>+             value="[% default.${field.name}.1 FILTER html %]">              

  Maybe you should just be passing in the value as "value", to the template? Then you don't have to internally know the structure of the "default" hash if you want to use this template elsewhere in Bugzilla. You could pass in the type as "type" or "search_type" or "type_value".

>+    [% CASE constants.FIELD_TYPE_DATETIME %]
>+      [% PROCESS field_label %]
>+      from <input name="[% field.name %]to" id="[% field.name %]" size="10" maxlength="10"
>+                  value="[% default.0 FILTER html %]">

  Don't you mean default.${field.name}.0 there?

>+      to <input name="[% field.name %]from" size="10" maxlength="10"
>+                value="[% default.1 FILTER html %]">

  Same there.

>+    [% CASE constants.FIELD_TYPE_BUG_ID %]
>+      [% PROCESS field_label %]

  But no actual input? Perhaps we just don't need this CASE.

>+    [% CASE [ constants.FIELD_TYPE_SINGLE_SELECT, 
>+              constants.FIELD_TYPE_MULTI_SELECT ] %]
>+        <div id="[% sel.name FILTER html %]_container" class="search_field_grid">
>+          [% PROCESS field_label %]
>+          <select name="[% field.name FILTER html%]" id="[% field.name FILTER html %]"
>+            [% IF onchange %] onchange="[% onchange FILTER html %]"[% END %]
>+            [% ' multiple="multiple" ' IF multiple %] 

  I think it's always "multiple", for a search select, right?

> size="[% size FILTER html %]">

  They're not just always size 7?

>+            [% values = ${field.name} %]
>+            [% IF ${field.name} == "component" %]
>+              [% values = ${"_component"} %]
>+            [% END %]

  Yeah, this is a place where I think that just passing in a "value" argument would be good.

>+            [% FOREACH value = values %]
>+              [% IF value.id %]
>+                <option value="[% v FILTER html %]"

  What's v?

>+                  [% " selected" IF contains(default.${sel.name}, v) != -1 %]>

  That line doesn't make any sense. I think you want IF value == default.${field.name}, with no other != or anything.

>+                  [% display_value(sel.name, value.name) FILTER html %]

  sel.name--I think you want field.name.

>+              [% ELSE %]
>+                <option value="[% value OR '---' FILTER html %]"

  Hmm. Either resolution comes in with an id or it doesn't--which one is it? I think we don't need the OR '---' in both places.

>+                  [% " selected" IF contains(default.${field.name}, value) != -1 %]>

  You shouldn't be using contains there. (Also, contains is a vmethod, not a function.)

>+                  [% display_value(sel.name, value) FILTER html %]

  sel.name.

>=== modified file 'template/en/default/search/form.html.tmpl'
>+<style type="text/css" media="screen">
>+  .search_field_grid {
>+    display: inline-block;
>+    margin-top: 1em;
>+  }
> [snip]

  This of course should all go into a separate .css file.

>+YAHOO.bugzilla.TUI = function(){
> [snip]

  And this I'm not reviewing yet, since you said you haven't gotten to re-working it yet.

>@@ -122,383 +306,171 @@
>+  <div class="search_field_row" id="summary_field">
>+      [% INCLUDE "search/field.html.tmpl"
>+         field = bug_fields.short_desc
>+         types = query_types
>+         selected = default.short_desc_type.0
>+         default = default.short_desc.0

  You don't need both "default" and "selected". Let's just call it "value", for consistency with bug/field.html.tmpl.

  I really love the cleanup in this file, by the way. :-) I'm so happy that this is getting so simple. :-) We might even want to BLOCK it out, like you did for edit.html.tmpl.

>+  [%# loop through a bunch of free text fields and print out their text stuff %]
>+    [% NEXT IF field.name == 'keywords' 
>+               AND NOT use_keywords
>+    %]

  Since the variable contains an item named "field", it might be better to call the loop variable something else, for clarity's sake.

>+    <div class="search_field_row">
>+      [% type = field.name _ "_type" %]
>+      [% qtype = field.qtypes || query_types %]
>+      [% INCLUDE "search/field.html.tmpl" 
>+          field => field.field
>+          types => field.qtypes || query_types

  Looks like "qtype" may be a duplicate there. Also the "type" variable may not be necessary.

>+    <div class="search_field_row">
>+      [% USE Dumper %]

  Looks like a debugging artifact.

>+        [% INCLUDE "search/field.html.tmpl" 
>+                    field = bug_fields.deadline, accesskey ="l",
>+                    default = [ default.deadlinefrom.0, default.deadlineto.0 ]
>+        %]
>+    </div>

  Remember that template indent is two characters, not four.

>+     <select name="bug_id_type">
>+       <option value="anyexact"[% " selected" IF default.bug_id_type.0 == "anyexact" %]>only included in</option>

  Nit: Very long line.

>+    <span>Narrow results to a role (ie. [% field_descs.assigned_to%], [% field_descs.reporter %], [% field_descs.commenter %], etc.) a person has on a [% terms.bug %]</span>

  Nit: Very long line.

>+      [% PROCESS role_types field = { count => n, name => "emailassigned_to",
>+                  label=> "the ${terms.Bug} ${field_descs.assigned_to}" } %]

  I like this refactor. :-)

>+<div class="bz_section_title" id="history_filter"><div class="arrow">&nbsp;</div><a href="#" >Search By Change History</a><span>Narrow results to how fields have changed during a specific time period</span></div>

  Nit: Very long line.


  Great patch, in general. :-)

Comment 21

[Guy Pyrzak]

Attachment: V3

Untested on IE6 but otherwise all changes made.

Comment 22

[Guy Pyrzak]

So i just noticed one big thing i forgot was adding the date change widget. I can add it in this bug or another one, just let me know which you guys prefer.

Comment 23

[Guy Pyrzak]

Attachment: V4

Added the calendar widgets as well as cleaned up a bug with the deadline to and from fields.

Comment 24

[Guy Pyrzak]

I made a version that uses the field-label a lot more, specifically in the history and the people sections. I'm not sure if all the links are overkill or not, let me know if you want me to post that version. It also gets rid of the JS that shows more help.

Comment 25

[Max Kanat-Alexander]

Oh, I would have rather you waited to do the calendar widgets in another bug. But I will review this one, and if it doesn't add too much complexity, it's OK.

We can do the "more field-label" stuff in another bug, also.

Comment 26

[Max Kanat-Alexander]

Comment on attachment 448476 [details] [diff] [review]
V4

  Did you ever knooooow that you're my heeeeroooo? :-D

  Love it.

  There are a few things that need to be fixed, though, before I can r+ it:

>=== modified file 'Bugzilla/Search.pm'
>-    foreach my $id ("1", "2") {
>+    foreach my $id ("1", "2", "3") {

  As LpSolit pointed out earlier, those don't need quotes, so you could remove the quotes.

>=== modified file 'js/field.js'
> function createCalendar(name) {
>+  try{

  Why are you using try/catch here?

>=== added file 'skins/standard/search_form.css'
>@@ -0,0 +1,116 @@
>+/*# The contents of this file are subject to the Mozilla Public

  Don't add # marks--see the comments in the other CSS files.

>+#bug_id_container{ 

  Nit: Space after "container".

>+#bug_id_container .field_help{

  And there.

>+ul.bug_changes li label{

  And there.

>+  font-size: 0.75em;

  FWIW, for relative font sizes, I think we usually do percentages instead of fractional em.

>+#summary_field label {
>+  margin-right:2em;

  Nit: Space after colon.

>+.hide_people_filter #people_filter_section, 
>+.hide_history_filter #history_filter_section, 
>+.hide_detailed_information #detailed_information_section {
>+  display: none;

  Nit: When there are multiple lines of specifiers, put the opening brace on the next line, like:

.foo,
.bar
{

>=== modified file 'template/en/default/bug/field-label.html.tmpl'
>-<th class="field_label [% ' bz_hidden_field' IF hidden %]
>+[% tag_name = "th" %]
>+[% IF rowspan < 0 %]
>+  [% tag_name = "span" %]
>+[% END %]

  That's a hack. Just pass in "tag" or "tag_name" to field-label or something like that. You should be able to do it from search/field.html.tmpl, so it would only have to be done there.

>=== added file 'template/en/default/search/field.html.tmpl'
>+  #   field: a Bugzilla::Field object
>+  #   value: the value or values(for date searces) that should be used to prepopulate the field

  "for date searches". Also, that line looks longer than 80 characters, and there should be a space before the opening parenthesis. Also, select fields also have multiple values.

>+  [% PROCESS "global/field-descs.none.tmpl" %]

  I don't think you have to do that...do you? field_descs itself is a global variable now.

>+      [% INCLUDE "search/type-select.html.tmpl"
>+         name = field.name _ "_type",
>+         types = types, 
>+         selected = type_selected  %]

  type_selected should be in the INTERFACE.

>+    [% CASE constants.FIELD_TYPE_DATETIME %]
>+      [% INCLUDE "bug/field-label.html.tmpl"
>+        field = field
>+        rowspan = -1
>+      %]
>+      from <input name="[% field.name %]from" id="[% field.name %]" size="10" maxlength="10"
>+                  value="[% value.0 FILTER html %]" onchange="updateCalendarFromField(this)">

  These lines look longer than 80 characters.

>+        <div id="[% field.name FILTER html %]_container" class="search_field_grid">

  Generally, you might want to make this container_[% field.name %], because somebody could have two fields: one called cf_blah and another called cf_blah_container, and then your ids would duplicate.

  (This is why I always prefix things to field names instead of appending things, unless it would break an existing API.)

>+          <select name="[% field.name FILTER html%]" id="[% field.name FILTER html %]" 

  Nit: Long line?

>+            [% IF onchange %] onchange="[% onchange FILTER html %]"[% END %]

  onchange should be in the INTERFACE.

>+            [% FOREACH current_value = values %]

  It's confusing to have one thing called "value" and another called "values". Can we this "legal_values" instead of "values"?

>+              [% IF current_value.id %]
>+                [%# This only applies for Resolution really %]
>+                [% v = current_value.name OR '---' -%]

  If it only applies to Resolution, then it shouldn't be in both the IF and the ELSE, it should only be in one place.

>+                <option value="[% v FILTER html %]"
>+                  [% " selected" IF value.contains( v ) %]>

  ' selected="selected"' instead of just "selected". We're not HTML5 yet.

>+                  [% " selected" IF value.contains( current_value ) %]>

  Same there. Also, that won't work if current_value is blank, right? Because "value" will contain ---, right?

>=== modified file 'template/en/default/search/form.html.tmpl'
>+// Hide the Advanced Fields by default, unless the user has a cookie
>+// that specifies otherwise.
>+TUI_alternates['history_query'] = '&#9656;';
>+TUI_alternates['people_query'] = '&#9656;';
>+TUI_alternates['information_query'] = '&#9656;';

  Cool. :-) Could you leave a comment saying what that character code means?

>+<div id="detailed_information" class="bz_section_title">
>+  <a href="javascript:TUI_toggle_class('information_query')">Detailed [% terms.Bug %] Information</a>
>+  <span>Narrow results by the following fields: 

  Should that span be section_help?

>+    <div class="search_field_row">
>+        [% INCLUDE "search/field.html.tmpl" 
>+                    field = bug_fields.deadline 
>+                    accesskey ="l"

  Nit: Space before "l"

>+    [% fake_version_field = { name =>  bug_fields.version.name,
>+                        type => constants.FIELD_TYPE_SINGLE_SELECT }%]

  Nit: Would be nice to align "name" and "type" if possible and if it doesn't make the lines too long.

>=== modified file 'template/en/default/search/search-advanced.html.tmpl'
>+  javascript_urls = [ "js/yui/calendar.js", "js/productform.js" "js/util.js" "js/help.js" , "js/TUI.js", "js/field.js"]

  Nit: Line too long.

Comment 27

[Max Kanat-Alexander]

  Okay, also, now that I'm looking at the UI, a few comments:

  * As I mentioned on IM, it breaks if useclassification is on.

  * TUI isn't working for me, for some reason. All of the sections appear by default, and it doesn't remember if I expand or collapse them.

  * I think Custom Search should also be collapsed with a twisty.

  * Perhaps each section should have a Search button? This is something we could do in a separate bug.

  * Perhaps the Sort widget should be moved up into the first section. It's a bit awkward having it at the bottom under the collapsed-by-default sections.

  * In another bug, we should work on the styling so that it looks more like your original screenshots.

  * The deadlineto calendar isn't working for me.

  * The sentence structure of the "Bugs numbered" line is a bit awkward. Perhaps "should be" instead of "are"? It's up to you.

  * It might be nice to update the defaultquery in Config.pm if the defaultquery is the old default defaultquery. (That is, right now upgraded installations don't get the benefit of your checkbox changes.)

  * In the "Search by Change History" section, it's no longer clear that not selecting any field will just give you *any* change to the bug.

  * In actually using the UI, I do feel slightly overwhelmed by the Detailed Bug Information section.

Comment 28

[Max Kanat-Alexander]

 Okay, some IE problems:

 * IE 8: The glyphs aren't available. They just show up as a box.

 * IE 8: Even with the fix I sent you, TUI works sometimes and doesn't work sometimes.

  * IE 8: Every single "where ANY of the fields" is selected, in Search By Change History.

  * IE 7: As you suspected, inline-block doesn't work. Everything that's inline-block renders as block instead, so you get a long vertical string of selects.

Comment 29

[Max Kanat-Alexander]

  Okay, TUI not working in IE is probably related to the deadlineto Calendar not working.

Comment 30

[Guy Pyrzak]

(In reply to comment #27)
>   * It might be nice to update the defaultquery in Config.pm if the
> defaultquery is the old default defaultquery. (That is, right now upgraded
> installations don't get the benefit of your checkbox changes.)
> 

I changed the setting how do i make it update for those who upgrade?

Comment 31

[Max Kanat-Alexander]

(In reply to comment #30)
> I changed the setting how do i make it update for those who upgrade?

  It's a bit tricky, but you'd want to do it in Bugzilla::Config's update_params. I can write the code if you want.

Comment 33

[Guy Pyrzak]

Attachment: V6

Comment 34

[Max Kanat-Alexander]

Comment on attachment 449094 [details] [diff] [review]
V6

  Instead of having the strange IE-hack CSS in the new css file, they should be in IE-fixes.css.

  Other than that, it looks OK. :-)

Comment 35

[Max Kanat-Alexander]

Also, there appears to be a long string "`                       " between <select> boxes.

Comment 36

[Max Kanat-Alexander]

Also (don't know how I missed this), Target Milestone appears to be absent from the page.

Comment 37

[Max Kanat-Alexander]

Oh, also, I'd like a little more vertical space between the labels and the selects.

Comment 38

[Guy Pyrzak]

Attachment: v7

I had to edit TUI to fix a few issues that IE7 was having with it which cause it to not work. I'm surprised it ever worked, basically it looks like item or classes are reserved words in IE7 for javascript. Changing those variables to yui_classes and yui_item fixed the bug i was running into (i'm not kidding). Feel free to ask me to change them to something else as a nit. Hopefully this is the last rev. 

I basically did my best to make sure this works with IE6 and IE7 which seemed to be the problem children

Comment 39

[Max Kanat-Alexander]

Comment on attachment 449502 [details] [diff] [review]
v7

>=== added file 'skins/contrib/Dusk/search_form.css'

  Make sure not to include this file on checkin.

>=== modified file 'skins/standard/IE-fixes.css'
>+#bug_id_container, .search_field_grid, 
>+.search_email_fields, ul.bug_changes li { 
>+  zoom: 1;
>+  *display: inline;
>+}

  Why do you have the zoom:1 and the * in this file? This file is only ever used by IE7 and below, thanks to conditional comments.


  The patch in general looks good, though. :-) Any other cosmetic changes can be done in another bug, and the above bits can be fixed on checkin.

Comment 40

[Guy Pyrzak]

the zoom: 1 is necessary, it causes the "hasLayout" hack to happen which causes the display: inline to work like an inline-block. I REALLY don't want to mess with those 2 lines because every website out there has stated that this is what is needed to get IE6 and IE7 to treat divs as "inline-blocks", here is a URL which I used to do what I did http://www.brunildo.org/test/InlineBlockLayout.html.

I am happy to get rid of the * though.

Comment 41

[Guy Pyrzak]

Ok a list of additional bugs we need to file/fix once this is committed:

1. Fix the CSS so that the whole thing follows a grid and looks like the mock, etc.
2. Add code to update the update the default query with new values (mkanat)
3. Custom Search should be in its own twisty
4. Custom search should use JS (already started)
5. Move the sort widget to the top of the form.
6. Each section should have its own search button?

Any additional bugs?

Comment 42

[Guy Pyrzak]

7. Update the text in the type-select to avoid phrases like "the string" and instead focus on less jargon-y phrases

Comment 43

[Guy Pyrzak]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                         
modified Bugzilla/Search.pm
modified Bugzilla/Config/Query.pm
modified js/TUI.js
modified skins/standard/IE-fixes.css
added skins/standard/search_form.css
modified template/en/default/filterexceptions.pl
modified template/en/default/bug/field-label.html.tmpl
modified template/en/default/search/boolean-charts.html.tmpl
added template/en/default/search/field.html.tmpl
modified template/en/default/search/form.html.tmpl
modified template/en/default/search/search-advanced.html.tmpl
modified template/en/default/search/search-create-series.html.tmpl
modified template/en/default/search/search-report-graph.html.tmpl
modified template/en/default/search/search-report-table.html.tmpl
Committed revision 7207.

Comment 44

[Guy Pyrzak]

Attachment: Fix the fire from commit

Just needed to add a bunch of filters

Comment 45

[Frédéric Buclin]

Comment on attachment 449533 [details] [diff] [review]
Fix the fire from commit

>=== modified file 'template/en/default/search/form.html.tmpl'

>+    [% terms.Bug FILTER html %] Numbers

No need to filter terms.Bug. We never filter terms.*.


>+      etc.) a person has on a [% terms.bug FILTER html%]

Must be: has on [% terms.abug %]. No filter, no "a" outside the directive.


r=LpSolit with both comments fixed.

Comment 46

[Frédéric Buclin]

Reopening, so that pyrzak doesn't forget the bustage fix.

Comment 47

[Guy Pyrzak]

bzr commit --fixes mozilla:450301 --author='Guy Pyrzak <[e-mail]>'
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/bug/field-label.html.tmpl
modified template/en/default/search/field.html.tmpl
modified template/en/default/search/form.html.tmpl
Committed revision 7208.

Comment 49

[Max Kanat-Alexander]

Added to the release notes in bug 604256.