455810 - Improve the usability of the keywords field (perhaps by autocomplete)

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P1)

Description

[Max Kanat-Alexander]

This is a spin-off of bug 452734, where we discovered that our "keyword chooser" was too difficult to use. We should replace it with some other method of making keywords more usable, perhaps using the YUI Autocomplete control.

Comment 1

[Max Kanat-Alexander]

pyrzak's students' research indicates that this is a significant issue.

At our design meeting with Mozilla, we agreed that autocomplete is a good solution, we just didn't like some of the implementation problems that happened on bugzilla.mozilla.org. So with a good autocomplete implementation, we should be able to solve this.

Comment 2

[Frédéric Buclin]

We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.

Comment 3

[Daniel Kabs, reporting bugs since 2002]

This bugzilla is version 3.4.4+ 

If I am not mistaken, this installation already sports an autocomplete combo box.

Thus I think this is already fixed.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> If I am not mistaken, this installation already sports an autocomplete combo
> box.

  That's a customization local to bugzilla.mozilla.org.

Comment 5

[Guy Pyrzak]

Attachment: V1

Comment 6

[Reed Loden [:reed]]

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

>         userAutoComp.queryDelay = 0.05

Missing semicolon at end of line.

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

Very much doubt that Netscape is the initial developer of this file. :)

Comment 7

[Frédéric Buclin]

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

>=== modified file 'js/field.js'

>+        keywordAutoComp.maxResultsDisplayed = 306;

Why 306? Isn't it a bit large? :)



>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+                   editable => bug.check_can_change_field(inputname, 0, 1)

s/inputname/"keywords"/

Comment 8

[Max Kanat-Alexander]

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

>=== modified file 'Bugzilla/Template.pm'
>+            # all the keywords
>+            'all_keywords' => sub { return Bugzilla::Keyword->get_all_with_bug_count();},

  You don't need them with bug count. You can just use get_all.

>=== modified file 'js/field.js'
>--- js/field.js	2010-02-22 00:00:53 +0000
>+++ js/field.js	2010-02-22 04:14:30 +0000
>@@ -621,7 +621,7 @@
>         // this is a throttle to determine the delay of the query from typing
>         // set this higher to cause fewer calls to the server
>         userAutoComp.queryDelay = 0.05
>-        userAutoComp.useIFrame = true
>+        userAutoComp.useIFrame = true;

  If you're adding a semicolon there, what about the queryDelay line also?

>@@ -630,3 +630,36 @@
>+        keywordAutoComp.delimChar = [" "];

  Did you pick space so that it will work in search?

>+        keywordAutoComp.maxResultsDisplayed = 306;

  As LpSolit said, this is probably a bit high, no?

>+        keywordAutoComp.textboxFocusEvent.subscribe( function(){
>+            var sInputValue = YAHOO.util.Dom.get('keywords').value;
>+            if(sInputValue.length === 0) {
>+                var oSelf = this;
>+                setTimeout(function(){oSelf.sendQuery(sInputValue);},0);
>+            }

  Why is this necessary? Shouldn't the minQueryLength handle this?

>+        keywordAutoComp.formatResult = function(oResultItem, sQuery) { 
>+            var sMarkup = "<span title='" +  oResultItem.desc + "'>" + oResultItem.name + "<\/span> " ; 

  You don't have to html-escape the name or description?

  Also, I think the description is too much to put into the HTML of every show_bug page, just to have it be a tooltip on the keyword. I'd suggest leaving it out for now.

>=== modified file 'template/en/default/bug/create/create.html.tmpl'
>+        <td colspan="2">          
>+          [% INCLUDE "bug/keyword-js.html.tmpl" field_id=> "keywords"
>+                   editable => 1 value =>  keywords  %]</div>
>+          
>         </td>

  My suggestion here was actually to use field.html.tmpl and then put keyword-js underneath it, as just containing the JS.

>=== modified file 'template/en/default/bug/edit.html.tmpl'
>+        [% INCLUDE "bug/keyword-js.html.tmpl" field_id=> "keywords"
>+                   editable => bug.check_can_change_field(inputname, 0, 1)

  "inputname" is going to be the previous field (or possibly undef).

>=== added file 'template/en/default/bug/keyword-js.html.tmpl'
>+[% IF editable %]
>+    <div>
>+      <input id="[% field_id %]" name="[% field_id %]" class="text_input"
>+             value="[% value FILTER html %]">
>+      <div id="keyword_autocomplete_container"></div>
>+    </div>

  Why a div around them?

>+    <script type="text/javascript">

  For some browsers, isn't there an "async" attribute or something that we can put here? That might actually be a reasonable thing to put here.

>+        YAHOO.bugzilla.keyword_array = [
>+        [%- FOREACH keyword = all_keywords %]
>+          [%-# %]{name:"[% keyword.name %]", desc:"[% keyword.description %]"}

  You need a FILTER js on the name and desc.

Comment 9

[David Lawrence [:dkl]]

Attachment: Guy's patch for keyword autocompletion backported to 3.4 with minor changes

We needed something just like this for b.r.c so I took this and ported it to 3.4 and seems to be working well for us. I did most of the suggested changes that Max mentioned as well as added additional css to global.css to make the list of keywords scrollable (we have a long list). Attaching what we have working.

Dave

Comment 10

[Reed Loden [:reed]]

Attachment: patch - v2

Patch unbitrotted and review comments mostly addressed.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 452338 [details] [diff] [review]
patch - v2

>+        this.dataSource.responseSchema = {fields : [ {key:"name"} ]};

  Instead of this, we can just make the keyword_array a flat array, and then I think you don't even need a responseSchema.

>+        keywordAutoComp.resultTypeList = false;

  And I'm not sure...is that necessary if we just have a flat array?

>+        keywordAutoComp.delimChar = [" "];

  They can be separated by both commas and spaces, so this should look like the userAutoComp delimChar instead.

>+        keywordAutoComp.textboxFocusEvent.subscribe( function(){
>+            var sInputValue = YAHOO.util.Dom.get('keywords').value;
>+            if(sInputValue.length === 0) {
>+                var oSelf = this;
>+                setTimeout(function(){oSelf.sendQuery(sInputValue);},0);

  I don't think that setTimeout is necessary--this is a local array datasource.

>=== modified file 'skins/standard/global.css'
>+#keyword_autocomplete_container .yui-ac-content {
>+  max-height: 30em;overflow:auto;overflow-x:hidden; /* set scrolling */
>+  _height: 30em; /* ie6 */

  IE fixes go into IE-fixes.css.

>=== modified file 'template/en/default/bug/create/create.html.tmpl'
>+          [% INCLUDE "bug/keyword-js.html.tmpl"

  I think since this actually contains the field, perhaps it should be "keyword-field", or a special FIELD_TYPE for field.html.tmpl.

>=== modified file 'template/en/default/bug/edit.html.tmpl'
>+           editable => bug.check_can_change_field("keywords", 0, 1)
>+           value => bug.keywords.join(', ')

  bug.keywords is a string.

>=== added file 'template/en/default/bug/keyword-js.html.tmpl'
>+[% IF editable %]
>+  <div id="keyword_autocomplete">
>+    <input id="[% field_id FILTER html %]" name="[% field_id FILTER html %]" 
>+           class="text_input" value="[% value FILTER html %]">
>+    <div id="keyword_autocomplete_container"></div>
>+  </div>

  I think logically, those divs have their ids backwards. Perhaps the top one should be "keyword_container" and the other should be "keyword_autocomplete"?

>+  <script type="text/javascript" defer>

  We're not HTML5 yet, so that would be defer="defer". (And does that actually work?)

>+[% field_id = "" %]
>+[% editable = 0  %]
>+[% value    = "" %]

  Why's that at the bottom? It shouldn't be necessary if you're using INCLUDE.

Comment 12

[Reed Loden [:reed]]

Attachment: wip on v3

attempt to address review comments, but completely untested.

Comment 13

[Reed Loden [:reed]]

Attachment: wip on v3 (rev 2)

Comment 14

[Reed Loden [:reed]]

Attachment: patch - v3

Comment 15

[Reed Loden [:reed]]

Comment on attachment 454434 [details] [diff] [review]
patch - v3

Ok, this works for all three cases (create bug, edit bug, edit multiple bugs), but edit multiple bugs looks really bad due to the huge input field (due to bad CSS). I'm going to file a separate bug to fix it, however, as the assignee field is affected by the same issue.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 454434 [details] [diff] [review]
patch - v3

  For some reason, the keywords field isn't appearing on the search page anymore, with this patch. Perhaps because its type changed?

>@@ -631,3 +631,28 @@
>+        keywordAutoComp.minQueryLength = 0;
>+        /*  Causes all the possibilities in the keyword to appear when a user 
>+         *  focuses on the textbox 
>+         */
>+        keywordAutoComp.textboxFocusEvent.subscribe( function(){

  Is that really necessary, with minQueryLength = 0?

  Also, I tested this, and it's a little inconvenient to have to move to the completed value. I think that if somebody starts *typing* in the field, it should select a value, but if they don't type, it shouldn't select a value.

>=== modified file 'skins/standard/IE-fixes.css'
>+#keyword_autocomplete .yui-ac-content {
>+    max-height: 30em;overflow:auto;overflow-x:hidden; /* set scrolling */

  Could you put those on separate lines?

>+    _height: 30em; /* ie6 */

  Is the _ necessary? That is, does this only affect IE6, and not IE7? Because this file is already only used by IE 7 and below.

>=== modified file 'skins/standard/global.css'
>+#keyword_autocomplete {
>+    width: 25em;

  Why don't you just put a div around the keywords and make the autocomplete the same width as the box? I think one of the older patches had that.


  Otherwise, this is looking pretty good. :-) I like FIELD_TYPE_KEYWORDS--maybe we can re-use that as a custom field type in the future if people want to add other keyword-like fields.

Comment 17

[Reed Loden [:reed]]

(In reply to comment #16)
>   For some reason, the keywords field isn't appearing on the search page
> anymore, with this patch. Perhaps because its type changed?

Indeed.

>   Is that really necessary, with minQueryLength = 0?

Yep. "Implementers are advised to pre-load the container with an explicit "sendQuery()" call." If you don't have it, the box won't load when you first click the input.

>   Also, I tested this, and it's a little inconvenient to have to move to the
> completed value. I think that if somebody starts *typing* in the field, it
> should select a value, but if they don't type, it shouldn't select a value.

Still trying to figure out how to do this...

> >+    _height: 30em; /* ie6 */
> 
>   Is the _ necessary? That is, does this only affect IE6, and not IE7? Because
> this file is already only used by IE 7 and below.

Yes, see http://www.matthidinger.com/archive/2008/05/07/ie6-underscore-hack.aspx

Comment 18

[Reed Loden [:reed]]

Attachment: patch - v4

Comment 19

[Max Kanat-Alexander]

(In reply to comment #17)
> Yes, see
> http://www.matthidinger.com/archive/2008/05/07/ie6-underscore-hack.aspx

  Right, but what I'm asking is--does the bug you're trying to fix affect only IE6 and not IE7?

Comment 20

[Max Kanat-Alexander]

Comment on attachment 454797 [details] [diff] [review]
patch - v4

This looks fine, but the most critical usability issue in the current system is that the first keyword is selected automatically when the box is clicked. So if I have to choose between having to press down and not having something auto-selected, I choose having to press down.

Here's what I want to do:

* Set queryDelay to 0 (0.2 is too long--I notice it, and the data source is local so there's no need to delay.)
* Immediately after the this.sendQuery in keywordAutoComplete.init, do:
    this.collapseContainer();
    this.expandContainer();
  That prevents anything from being selected when the box is first clicked in, but does autohighlight afterward.

Comment 21

[Reed Loden [:reed]]

(In reply to comment #19)
>   Right, but what I'm asking is--does the bug you're trying to fix affect only
> IE6 and not IE7?

Yes, IE6 only, as far as I can tell (I don't have Windows or IE to test, but it was a recommended fix from the YUI documentation I found).

Comment 22

[Reed Loden [:reed]]

Attachment: patch - v5

Comment 23

[Max Kanat-Alexander]

Comment on attachment 456018 [details] [diff] [review]
patch - v5

All right, this looks great. Thank you so much for your work on this patch, reed, and thanks to pyrzak for all the original versions.

Comment 24

[Reed Loden [:reed]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Constants.pm
modified Bugzilla/Field.pm
modified Bugzilla/Template.pm
modified js/field.js
modified skins/standard/IE-fixes.css
modified skins/standard/global.css
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/list/edit-multiple.html.tmpl
modified template/en/default/search/field.html.tmpl
modified template/en/default/search/search-advanced.html.tmpl
Committed revision 7262.

Comment 25

[Max Kanat-Alexander]

Added to the release notes in bug 604256.