Closed
Bug 455810
Opened 16 years ago
Closed 14 years ago
Improve the usability of the keywords field (perhaps by autocomplete)
Categories
(Bugzilla :: User Interface, enhancement, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.0
People
(Reporter: mkanat, Assigned: reed)
References
Details
(Keywords: ue, Whiteboard: [3.6 Focus][wanted-bmo])
Attachments
(2 files, 6 obsolete files)
84.84 KB,
patch
|
Details | Diff | Splinter Review | |
14.21 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
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.
Reporter | ||
Updated•15 years ago
|
Blocks: bz-hci2008
Reporter | ||
Updated•15 years ago
|
Whiteboard: [3.6 Focus]
Reporter | ||
Updated•15 years ago
|
Assignee: ui → mkanat
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6
Updated•15 years ago
|
Comment 2•15 years ago
|
||
We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Comment 3•15 years ago
|
||
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.
Reporter | ||
Comment 4•15 years ago
|
||
(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•14 years ago
|
||
Assignee: mkanat → guy.pyrzak
Attachment #428134 -
Flags: review?(mkanat)
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
||
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"/
Reporter | ||
Comment 8•14 years ago
|
||
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.
Attachment #428134 -
Flags: review?(mkanat) → review-
Comment 9•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Whiteboard: [3.6 Focus] → [3.6 Focus][wanted-bmo]
Assignee | ||
Comment 10•14 years ago
|
||
Patch unbitrotted and review comments mostly addressed.
Attachment #428134 -
Attachment is obsolete: true
Attachment #452338 -
Flags: review?(mkanat)
Reporter | ||
Updated•14 years ago
|
Attachment #452338 -
Flags: review?(mkanat) → review-
Reporter | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
attempt to address review comments, but completely untested.
Attachment #452338 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #454425 -
Attachment is obsolete: true
Assignee | ||
Comment 15•14 years ago
|
||
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.
Attachment #454434 -
Attachment description: wip on v3 (rev 3) → patch - v3
Attachment #454434 -
Flags: review?(mkanat)
Reporter | ||
Comment 16•14 years ago
|
||
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.
Attachment #454434 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 17•14 years ago
|
||
(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
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #454434 -
Attachment is obsolete: true
Attachment #454797 -
Flags: review?(mkanat)
Reporter | ||
Comment 19•14 years ago
|
||
(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?
Reporter | ||
Comment 20•14 years ago
|
||
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.
Attachment #454797 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 21•14 years ago
|
||
(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).
Assignee | ||
Comment 22•14 years ago
|
||
Attachment #454797 -
Attachment is obsolete: true
Attachment #456018 -
Flags: review?(mkanat)
Reporter | ||
Comment 23•14 years ago
|
||
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.
Attachment #456018 -
Flags: review?(mkanat) → review+
Assignee | ||
Comment 24•14 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•