r64718 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r64717‎ | r64718 | r64719 >
Date:19:15, 7 April 2010
Author:adam
Status:ok (Comments)
Tags:
Comment:
SimpleSearch updates: Making keyboard events cycle back around to the top of the list, preventing the text from changing with key up and down events, and allowing access to the specials via key up and down events
Modified paths:
  • /trunk/extensions/UsabilityInitiative/UsabilityInitiative.hooks.php (modified) (history)
  • /trunk/extensions/UsabilityInitiative/js/plugins.combined.js (modified) (history)
  • /trunk/extensions/UsabilityInitiative/js/plugins.combined.min.js (modified) (history)
  • /trunk/extensions/UsabilityInitiative/js/plugins/jquery.suggestions.js (modified) (history)

Diff [purge]

Index: trunk/extensions/UsabilityInitiative/UsabilityInitiative.hooks.php
——@@ -70,7 +70,7 @@
7171 array( 'src' => 'js/plugins/jquery.color.js', 'version' => 1 ),
7272 array( 'src' => 'js/plugins/jquery.cookie.js', 'version' => 4 ),
7373 array( 'src' => 'js/plugins/jquery.delayedBind.js', 'version' => 1 ),
74 - array( 'src' => 'js/plugins/jquery.suggestions.js', 'version' => 9 ),
 74+ array( 'src' => 'js/plugins/jquery.suggestions.js', 'version' => 10 ),
7575 array( 'src' => 'js/plugins/jquery.textSelection.js', 'version' => 30 ),
7676 array( 'src' => 'js/plugins/jquery.wikiEditor.js', 'version' => 181 ),
7777 array( 'src' => 'js/plugins/jquery.wikiEditor.highlight.js', 'version' => 52 ),
——@@ -82,10 +82,10 @@
8383 array( 'src' => 'js/plugins/jquery.wikiEditor.publish.js', 'version' => 5 ),
8484 ),
8585 'combined' => array(
86 - array( 'src' => 'js/plugins.combined.js', 'version' => 377 ),
 86+ array( 'src' => 'js/plugins.combined.js', 'version' => 378 ),
8787 ),
8888 'minified' => array(
89 - array( 'src' => 'js/plugins.combined.min.js', 'version' => 377 ),
 89+ array( 'src' => 'js/plugins.combined.min.js', 'version' => 378 ),
9090 ),
9191 ),
9292 );
Index: trunk/extensions/UsabilityInitiative/js/plugins/jquery.suggestions.js
——@@ -177,16 +177,32 @@
178178 var selected = context.data.$container.find( '.suggestions-result-current' );
179179 if ( !result.get || selected.get( 0 ) != result.get( 0 ) ) {
180180 if ( result == 'prev' ) {
181 - result = selected.prev();
 181+ if( selected.is( '.suggestions-special' ) ) {
 182+ result = context.data.$container.find( '.suggestions-results div:last' )
 183+ } else {
 184+ result = selected.prev();
 185+ if ( selected.size() == 0 ) {
 186+ // we are at the begginning, so lets jump to the last item
 187+ if ( context.data.$container.find( '.suggestions-special' ).html() != "" ) {
 188+ result = context.data.$container.find( '.suggestions-special' );
 189+ } else {
 190+ result = context.data.$container.find( '.suggestions-results div:last' );
 191+ }
 192+
 193+ }
 194+ }
182195 } else if ( result == 'next' ) {
183196 if ( selected.size() == 0 )
184197 // No item selected, go to the first one
185198 result = context.data.$container.find( '.suggestions-results div:first' );
186199 else {
187200 result = selected.next();
188 - if ( result.size() == 0 )
189 - // We were at the last item, stay there
190 - result = selected;
 201+ if ( selected.is( '.suggestions-special' ) ) {
 202+ result = [];
 203+ } else if ( result.size() == 0 && context.data.$container.find( '.suggestions-special' ).html() != "" ) {
 204+ // We were at the last item, jump to the specials!
 205+ result = context.data.$container.find( '.suggestions-special' );
 206+ }
191207 }
192208 }
193209 selected.removeClass( 'suggestions-result-current' );
——@@ -215,7 +231,7 @@
216232 // Arrow down
217233 case 40:
218234 if ( wasVisible ) {
219 - $.suggestions.highlight( context, 'next', true );
 235+ $.suggestions.highlight( context, 'next', false );
220236 } else {
221237 $.suggestions.update( context, false );
222238 }
——@@ -225,7 +241,7 @@
226242 // Arrow up
227243 case 38:
228244 if ( wasVisible ) {
229 - $.suggestions.highlight( context, 'prev', true );
 245+ $.suggestions.highlight( context, 'prev', false );
230246 }
231247 context.data.$textbox.trigger( 'change' );
232248 preventDefault = wasVisible;
——@@ -242,11 +258,16 @@
243259 case 13:
244260 context.data.$container.hide();
245261 preventDefault = wasVisible;
246 - if ( typeof context.config.result.select == 'function' ) {
247 - context.config.result.select.call(
248 - context.data.$container.find( '.suggestions-result-current' ),
249 - context.data.$textbox
250 - );
 262+ selected = context.data.$container.find( '.suggestions-result-current' )
 263+ if ( selected.is( '.suggestions-special' ) ) {
 264+ if ( typeof context.config.special.select == 'function' ) {
 265+ context.config.special.select.call( selected, context.data.$textbox );
 266+ }
 267+ } else {
 268+ if ( typeof context.config.result.select == 'function' ) {
 269+ context.data.$textbox.val( selected.text() );
 270+ context.config.result.select.call( selected, context.data.$textbox );
 271+ }
251272 }
252273 break;
253274 default:
Index: trunk/extensions/UsabilityInitiative/js/plugins.combined.js
——@@ -5915,16 +5915,32 @@
59165916 var selected = context.data.$container.find( '.suggestions-result-current' );
59175917 if ( !result.get || selected.get( 0 ) != result.get( 0 ) ) {
59185918 if ( result == 'prev' ) {
5919 - result = selected.prev();
 5919+ if( selected.is( '.suggestions-special' ) ) {
 5920+ result = context.data.$container.find( '.suggestions-results div:last' )
 5921+ } else {
 5922+ result = selected.prev();
 5923+ if ( selected.size() == 0 ) {
 5924+ // we are at the begginning, so lets jump to the last item
 5925+ if ( context.data.$container.find( '.suggestions-special' ).html() != "" ) {
 5926+ result = context.data.$container.find( '.suggestions-special' );
 5927+ } else {
 5928+ result = context.data.$container.find( '.suggestions-results div:last' );
 5929+ }
 5930+
 5931+ }
 5932+ }
59205933 } else if ( result == 'next' ) {
59215934 if ( selected.size() == 0 )
59225935 // No item selected, go to the first one
59235936 result = context.data.$container.find( '.suggestions-results div:first' );
59245937 else {
59255938 result = selected.next();
5926 - if ( result.size() == 0 )
5927 - // We were at the last item, stay there
5928 - result = selected;
 5939+ if ( selected.is( '.suggestions-special' ) ) {
 5940+ result = [];
 5941+ } else if ( result.size() == 0 && context.data.$container.find( '.suggestions-special' ).html() != "" ) {
 5942+ // We were at the last item, jump to the specials!
 5943+ result = context.data.$container.find( '.suggestions-special' );
 5944+ }
59295945 }
59305946 }
59315947 selected.removeClass( 'suggestions-result-current' );
——@@ -5953,7 +5969,7 @@
59545970 // Arrow down
59555971 case 40:
59565972 if ( wasVisible ) {
5957 - $.suggestions.highlight( context, 'next', true );
 5973+ $.suggestions.highlight( context, 'next', false );
59585974 } else {
59595975 $.suggestions.update( context, false );
59605976 }
——@@ -5963,7 +5979,7 @@
59645980 // Arrow up
59655981 case 38:
59665982 if ( wasVisible ) {
5967 - $.suggestions.highlight( context, 'prev', true );
 5983+ $.suggestions.highlight( context, 'prev', false );
59685984 }
59695985 context.data.$textbox.trigger( 'change' );
59705986 preventDefault = wasVisible;
——@@ -5980,11 +5996,16 @@
59815997 case 13:
59825998 context.data.$container.hide();
59835999 preventDefault = wasVisible;
5984 - if ( typeof context.config.result.select == 'function' ) {
5985 - context.config.result.select.call(
5986 - context.data.$container.find( '.suggestions-result-current' ),
5987 - context.data.$textbox
5988 - );
 6000+ selected = context.data.$container.find( '.suggestions-result-current' )
 6001+ if ( selected.is( '.suggestions-special' ) ) {
 6002+ if ( typeof context.config.special.select == 'function' ) {
 6003+ context.config.special.select.call( selected, context.data.$textbox );
 6004+ }
 6005+ } else {
 6006+ if ( typeof context.config.result.select == 'function' ) {
 6007+ context.data.$textbox.val( selected.text() );
 6008+ context.config.result.select.call( selected, context.data.$textbox );
 6009+ }
59896010 }
59906011 break;
59916012 default:
Index: trunk/extensions/UsabilityInitiative/js/plugins.combined.min.js
——@@ -395,14 +395,14 @@
396396 if(delayed){context.data.timerID=setTimeout(maybeFetch,context.config.delay);}else{maybeFetch();}
397397 $.suggestions.special(context);},special:function(context){if(typeof context.config.special.render=='function'){setTimeout(function(){$special=context.data.$container.find('.suggestions-special');if($special.children().length==0){$special.mousemove(function(){$.suggestions.highlight(context,$([]),false);});}
398398 context.config.special.render.call($special,context.data.$textbox.val());},1);}},configure:function(context,property,value){switch(property){case'fetch':case'cancel':case'special':case'result':case'$region':context.config[property]=value;break;case'suggestions':context.config[property]=value;if(typeof context.data!=='undefined'){if(context.data.$textbox.val().length==0){context.data.$container.hide();}else{context.data.$container.show();context.data.$container.css({'top':context.config.$region.offset().top+context.config.$region.outerHeight(),'bottom':'auto','width':context.config.$region.outerWidth(),'height':'auto','left':context.config.$region.offset().left,'right':'auto'});var $results=context.data.$container.children('.suggestions-results');$results.empty();for(var i=0;i<context.config.suggestions.length;i++){$result=$('<div />').addClass('suggestions-result').attr('rel',i).data('text',context.config.suggestions[i]).mouseover(function(e){$.suggestions.highlight(context,$(this).closest('.suggestions-results div'),false);}).appendTo($results);if(typeof context.config.result.render=='function'){context.config.result.render.call($result,context.config.suggestions[i]);}else{$result.text(context.config.suggestions[i]).autoEllipsis();}}}}
399 -break;case'maxRows':context.config[property]=Math.max(1,Math.min(100,value));break;case'delay':context.config[property]=Math.max(0,Math.min(1200,value));break;case'submitOnClick':context.config[property]=value?true:false;break;}},highlight:function(context,result,updateTextbox){var selected=context.data.$container.find('.suggestions-result-current');if(!result.get||selected.get(0)!=result.get(0)){if(result=='prev'){result=selected.prev();}else if(result=='next'){if(selected.size()==0)
400 -result=context.data.$container.find('.suggestions-results div:first');else{result=selected.next();if(result.size()==0)
401 -result=selected;}}
 399+break;case'maxRows':context.config[property]=Math.max(1,Math.min(100,value));break;case'delay':context.config[property]=Math.max(0,Math.min(1200,value));break;case'submitOnClick':context.config[property]=value?true:false;break;}},highlight:function(context,result,updateTextbox){var selected=context.data.$container.find('.suggestions-result-current');if(!result.get||selected.get(0)!=result.get(0)){if(result=='prev'){if(selected.is('.suggestions-special')){result=context.data.$container.find('.suggestions-results div:last')}else{result=selected.prev();if(selected.size()==0){if(context.data.$container.find('.suggestions-special').html()!=""){result=context.data.$container.find('.suggestions-special');}else{result=context.data.$container.find('.suggestions-results div:last');}}}}else if(result=='next'){if(selected.size()==0)
 400+result=context.data.$container.find('.suggestions-results div:first');else{result=selected.next();if(selected.is('.suggestions-special')){result=[];}else if(result.size()==0&&context.data.$container.find('.suggestions-special').html()!=""){result=context.data.$container.find('.suggestions-special');}}}
402401 selected.removeClass('suggestions-result-current');result.addClass('suggestions-result-current');}
403402 if(updateTextbox){if(result.size()==0){$.suggestions.restore(context);}else{context.data.$textbox.val(result.data('text'));context.data.$textbox.change();}}
404 -$.suggestions.special(context);},keypress:function(e,context,key){var wasVisible=context.data.$container.is(':visible');var preventDefault=false;switch(key){case 40:if(wasVisible){$.suggestions.highlight(context,'next',true);}else{$.suggestions.update(context,false);}
405 -context.data.$textbox.trigger('change');preventDefault=true;break;case 38:if(wasVisible){$.suggestions.highlight(context,'prev',true);}
406 -context.data.$textbox.trigger('change');preventDefault=wasVisible;break;case 27:context.data.$container.hide();$.suggestions.restore(context);$.suggestions.cancel(context);context.data.$textbox.trigger('change');preventDefault=wasVisible;break;case 13:context.data.$container.hide();preventDefault=wasVisible;if(typeof context.config.result.select=='function'){context.config.result.select.call(context.data.$container.find('.suggestions-result-current'),context.data.$textbox);}
 403+$.suggestions.special(context);},keypress:function(e,context,key){var wasVisible=context.data.$container.is(':visible');var preventDefault=false;switch(key){case 40:if(wasVisible){$.suggestions.highlight(context,'next',false);}else{$.suggestions.update(context,false);}
 404+context.data.$textbox.trigger('change');preventDefault=true;break;case 38:if(wasVisible){$.suggestions.highlight(context,'prev',false);}
 405+context.data.$textbox.trigger('change');preventDefault=wasVisible;break;case 27:context.data.$container.hide();$.suggestions.restore(context);$.suggestions.cancel(context);context.data.$textbox.trigger('change');preventDefault=wasVisible;break;case 13:context.data.$container.hide();preventDefault=wasVisible;selected=context.data.$container.find('.suggestions-result-current')
 406+if(selected.is('.suggestions-special')){if(typeof context.config.special.select=='function'){context.config.special.select.call(selected,context.data.$textbox);}}else{if(typeof context.config.result.select=='function'){context.data.$textbox.val(selected.text());context.config.result.select.call(selected,context.data.$textbox);}}
407407 break;default:$.suggestions.update(context,true);break;}
408408 if(preventDefault){e.preventDefault();e.stopImmediatePropagation();}}};$.fn.suggestions=function(){var returnValue=null;var args=arguments;$(this).each(function(){var context=$(this).data('suggestions-context');if(typeof context=='undefined'){context={config:{'fetch':function(){},'cancel':function(){},'special':{},'result':{},'$region':$(this),'suggestions':[],'maxRows':7,'delay':120,'submitOnClick':false}};}
409409 if(args.length>0){if(typeof args[0]=='object'){for(var key in args[0]){$.suggestions.configure(context,key,args[0][key]);}}else if(typeof args[0]=='string'){if(args.length>1){$.suggestions.configure(context,args[0],args[1]);}else if(returnValue==null){returnValue=(args[0]in context.config?undefined:context.config[args[0]]);}}}

Comments

#Comment by Nimish Gautam (talk | contribs)   22:50, 12 June 2010

Why was this changed? (the up/down keys replacing the text in the search box)

#Comment by Adammiller~mediawikiwiki (talk | contribs)   02:03, 15 June 2010

Why was it disabled in the first place? Do you know? Trevor couldn't remember, Roan couldn't remember, and I couldn't remember. I've been seeing bug reports related to this, a few other people just asking for this functionality and I personally feel this is how it should work, so I reenabled it.

#Comment by Trevor Parscal (WMF) (talk | contribs)   02:10, 15 June 2010

I do remember - sorry if I said otherwise at some point...

This was part of a redesign of the control. Howie, Parul and I worked together to figure out how to fix some of the odd bits with the UI. The problem with changing the term in the textbox as you arrow up and down is that if you reach the "containing" item, it's unclear what term you mean to use a full text search because the textbox is now filled with the last suggestion, even if you were to restore it upon reaching that item to the originally typed text, it's just not intuitive to have it changing like that. Now, the textbox only shows the term you have actually typed, making the "containing" item consistent despite the currently selected suggestion.

As for bug reports, they should be closed with wontfix, this is not an uncommon way to do this control, and for our particular use case (with the "containing" item at the bottom of the list) it's the best solution.

#Comment by Adammiller~mediawikiwiki (talk | contribs)   02:11, 15 June 2010

Oh......you seem to be asking the same thing. Sorry I didn't realize how old this revision was. Thought this comment was on r67755 where I reenabled it.

Yeah, no clue why I did this. Maybe I misheard some instructions from Trevor when he was asking me to make these changes.

#Comment by Trevor Parscal (WMF) (talk | contribs)   02:12, 15 June 2010

Perhaps, maybe I was unclear - sorry about that.

Status & tagging log