r56391 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r56390‎ | r56391 | r56392 >
Date:19:40, 15 September 2009
Author:catrope
Status:ok (Comments)
Tags:
Comment:
EditToolbar: (bug 20605) Trigger Insert/Replace when Enter key is pressed by inserting hidden submit buttons. Also make the suggestions plugin stop propagation of Enter/Esc/Arrow keypresses when it's handling it itself, so that pressing Enter to select a suggestion doesn't also submit the form
Modified paths:
  • /trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.js (modified) (history)
  • /trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.php (modified) (history)
  • /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
——@@ -61,7 +61,7 @@
6262 array( 'src' => 'js/plugins/jquery.browser.js', 'version' => 3 ),
6363 array( 'src' => 'js/plugins/jquery.cookie.js', 'version' => 3 ),
6464 array( 'src' => 'js/plugins/jquery.namespaceSelect.js', 'version' => 1 ),
65 - array( 'src' => 'js/plugins/jquery.suggestions.js', 'version' => 2 ),
 65+ array( 'src' => 'js/plugins/jquery.suggestions.js', 'version' => 3 ),
6666 array( 'src' => 'js/plugins/jquery.textSelection.js', 'version' => 9 ),
6767 array( 'src' => 'js/plugins/jquery.wikiEditor.js', 'version' => 4 ),
6868 array( 'src' => 'js/plugins/jquery.wikiEditor.toolbar.js', 'version' => 11 ),
——@@ -70,10 +70,10 @@
7171 array( 'src' => 'js/js2/jquery-ui-1.7.2.js', 'version' => '1.7.2x' ),
7272 ),
7373 'combined' => array(
74 - array( 'src' => 'js/plugins.combined.js', 'version' => 20 ),
 74+ array( 'src' => 'js/plugins.combined.js', 'version' => 21 ),
7575 ),
7676 'minified' => array(
77 - array( 'src' => 'js/plugins.combined.min.js', 'version' => 20 ),
 77+ array( 'src' => 'js/plugins.combined.min.js', 'version' => 21 ),
7878 ),
7979 ),
8080 );
Index: trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.js
——@@ -716,14 +716,14 @@
717717 </tr><tr>\
718718 <td><label for="edittoolbar-link-int-text" rel="edittoolbar-tool-link-int-text"></label></td>\
719719 <td><input type="text" id="edittoolbar-link-int-text" /></td>\
720 - </table></fieldset></form></div>\
 720+ </table></fieldset><input type="submit" style="display: none;" /></form></div>\
721721 <div id="edittoolbar-link-dialog-tab-ext"><form><fieldset><table><tr>\
722722 <td><label for="edittoolbar-link-ext-target" rel="edittoolbar-tool-link-ext-target"></label></td>\
723723 <td><input type="text" id="edittoolbar-link-ext-target" /></td>\
724724 </tr><tr>\
725725 <td><label for="edittoolbar-link-ext-text" rel="edittoolbar-tool-link-ext-text"></label></td>\
726726 <td><input type="text" id="edittoolbar-link-ext-text" /></td>\
727 - </table></fieldset></form></div>\
 727+ </table></fieldset><input type="submit" style="display: none;" /></form></div>\
728728 </div>',
729729 init: function() {
730730 // Updates the UI to show if the page title being inputed by the user exists or not
——@@ -788,6 +788,15 @@
789789 $j(this).find( '[rel]' ).each( function() {
790790 $j(this).text( gM( $j(this).attr( 'rel' ) ) );
791791 });
 792+ // Assign the proper action to the hidden submit buttons
 793+ // triggered when the user presses Enter
 794+ $j(this).find( 'form' ).submit( function( e ) {
 795+ e.preventDefault();
 796+ $j(this)
 797+ .closest( '.ui-dialog' )
 798+ .find( 'button:first' )
 799+ .click();
 800+ });
792801 // Build tabs
793802 $j( '#edittoolbar-link-tabs' ).tabs();
794803 // Automatically copy the value of the internal link page title field to the link text field unless the user
——@@ -1022,11 +1031,20 @@
10231032 <td class="label"><label for="edittoolbar-table-dimensions-rows"\
10241033 rel="edittoolbar-tool-table-dimensions-rows"></label></td>\
10251034 <td><input type="text" id="edittoolbar-table-dimensions-rows" size="2" /></td>\
1026 - </tr></table></fieldset></form>',
 1035+ </tr></table></fieldset><input type="submit" style="display: none;" /></form>',
10271036 init: function() {
10281037 $j(this).find( '[rel]' ).each( function() {
10291038 $j(this).text( gM( $j(this).attr( 'rel' ) ) );
10301039 });
 1040+ // Assign the proper action to the hidden submit buttons
 1041+ // triggered when the user presses Enter
 1042+ $j(this).find( 'form' ).submit( function( e ) {
 1043+ e.preventDefault();
 1044+ $j(this)
 1045+ .closest( '.ui-dialog' )
 1046+ .find( 'button:first' )
 1047+ .click();
 1048+ });
10311049 $j( '#edittoolbar-table-dimensions-rows' ).val( 2 );
10321050 $j( '#edittoolbar-table-dimensions-columns' ).val( 2 );
10331051 },
——@@ -1093,11 +1111,20 @@
10941112 </tr><tr>\
10951113 <td><input type="checkbox" id="edittoolbar-replace-all" /></td>\
10961114 <td><label for="edittoolbar-replace-all" rel="edittoolbar-tool-replace-all"></label></td>\
1097 - </tr></table></fieldset></form>',
 1115+ </tr></table></fieldset><input type="submit" style="display: none;" /></form>',
10981116 init: function() {
10991117 $j(this).find( '[rel]' ).each( function() {
11001118 $j(this).text( gM( $j(this).attr( 'rel' ) ) );
11011119 });
 1120+ // Assign the proper action to the hidden submit buttons
 1121+ // triggered when the user presses Enter
 1122+ $j(this).find( 'form' ).submit( function( e ) {
 1123+ e.preventDefault();
 1124+ $j(this)
 1125+ .closest( '.ui-dialog' )
 1126+ .find( 'button:first' )
 1127+ .click();
 1128+ });
11021129 },
11031130 dialog: {
11041131 buttons: {
Index: trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.php
——@@ -19,7 +19,7 @@
2020 /* Configuration */
2121
2222 // Bump the version number every time you change any of the .css/.js files
23 -$wgEditToolbarStyleVersion = 31;
 23+$wgEditToolbarStyleVersion = 32;
2424
2525 // Set this to true to simply override the stock toolbar for everyone
2626 $wgEditToolbarGlobalEnable = false;
Index: trunk/extensions/UsabilityInitiative/js/plugins/jquery.suggestions.js
——@@ -199,23 +199,27 @@
200200 * Respond to keypress event
201201 * @param {Integer} key Code of key pressed
202202 */
203 - keypress: function( context, key ) {
 203+ keypress: function( e, context, key ) {
 204+ var wasVisible = context.data.$container.is( ':visible' );
 205+ var preventDefault = false;
204206 switch ( key ) {
205207 // Arrow down
206208 case 40:
207 - if ( context.data.$container.is( ':visible' ) ) {
 209+ if ( wasVisible ) {
208210 $.suggestions.highlight( context, 'next', true );
209211 } else {
210212 $.suggestions.update( context, false );
211213 }
212214 context.data.$textbox.trigger( 'change' );
 215+ preventDefault = true;
213216 break;
214217 // Arrow up
215218 case 38:
216 - if ( context.data.$container.is( ':visible' ) ) {
 219+ if ( wasVisible ) {
217220 $.suggestions.highlight( context, 'prev', true );
218221 }
219222 context.data.$textbox.trigger( 'change' );
 223+ preventDefault = wasVisible;
220224 break;
221225 // Escape
222226 case 27:
——@@ -223,15 +227,21 @@
224228 $.suggestions.restore( context );
225229 $.suggestions.cancel( context );
226230 context.data.$textbox.trigger( 'change' );
 231+ preventDefault = wasVisible;
227232 break;
228233 // Enter
229234 case 13:
230235 context.data.$container.hide();
 236+ preventDefault = wasVisible;
231237 break;
232238 default:
233239 $.suggestions.update( context, true );
234240 break;
235241 }
 242+ if ( preventDefault ) {
 243+ e.preventDefault();
 244+ e.stopImmediatePropagation();
 245+ }
236246 }
237247 };
238248 $.fn.suggestions = function() {
——@@ -357,16 +367,32 @@
358368 // Store key pressed to handle later
359369 context.data.keypressed = ( e.keyCode == undefined ) ? e.which : e.keyCode;
360370 context.data.keypressedCount = 0;
 371+
 372+ switch ( context.data.keypressed ) {
 373+ // This preventDefault logic is duplicated from
 374+ // $.suggestions.keypress(), which sucks
 375+ case 40:
 376+ e.preventDefault();
 377+ e.stopImmediatePropagation();
 378+ break;
 379+ case 38:
 380+ case 27:
 381+ case 13:
 382+ if ( context.data.$container.is( ':visible' ) ) {
 383+ e.preventDefault();
 384+ e.stopImmediatePropagation();
 385+ }
 386+ }
361387 } )
362 - .keypress( function() {
 388+ .keypress( function( e ) {
363389 context.data.keypressedCount++;
364 - $.suggestions.keypress( context, context.data.keypressed );
 390+ $.suggestions.keypress( e, context, context.data.keypressed );
365391 } )
366392 .keyup( function() {
367393 // Some browsers won't throw keypress() for arrow keys. If we got a keydown and a keyup without a
368394 // keypress in between, solve it
369395 if ( context.data.keypressedCount == 0 ) {
370 - $.suggestions.keypress( context, context.data.keypressed );
 396+ $.suggestions.keypress( e, context, context.data.keypressed );
371397 }
372398 } )
373399 .blur( function() {
Index: trunk/extensions/UsabilityInitiative/js/plugins.combined.js
——@@ -496,23 +496,27 @@
497497 * Respond to keypress event
498498 * @param {Integer} key Code of key pressed
499499 */
500 - keypress: function( context, key ) {
 500+ keypress: function( e, context, key ) {
 501+ var wasVisible = context.data.$container.is( ':visible' );
 502+ var preventDefault = false;
501503 switch ( key ) {
502504 // Arrow down
503505 case 40:
504 - if ( context.data.$container.is( ':visible' ) ) {
 506+ if ( wasVisible ) {
505507 $.suggestions.highlight( context, 'next', true );
506508 } else {
507509 $.suggestions.update( context, false );
508510 }
509511 context.data.$textbox.trigger( 'change' );
 512+ preventDefault = true;
510513 break;
511514 // Arrow up
512515 case 38:
513 - if ( context.data.$container.is( ':visible' ) ) {
 516+ if ( wasVisible ) {
514517 $.suggestions.highlight( context, 'prev', true );
515518 }
516519 context.data.$textbox.trigger( 'change' );
 520+ preventDefault = wasVisible;
517521 break;
518522 // Escape
519523 case 27:
——@@ -520,15 +524,21 @@
521525 $.suggestions.restore( context );
522526 $.suggestions.cancel( context );
523527 context.data.$textbox.trigger( 'change' );
 528+ preventDefault = wasVisible;
524529 break;
525530 // Enter
526531 case 13:
527532 context.data.$container.hide();
 533+ preventDefault = wasVisible;
528534 break;
529535 default:
530536 $.suggestions.update( context, true );
531537 break;
532538 }
 539+ if ( preventDefault ) {
 540+ e.preventDefault();
 541+ e.stopImmediatePropagation();
 542+ }
533543 }
534544 };
535545 $.fn.suggestions = function() {
——@@ -654,16 +664,32 @@
655665 // Store key pressed to handle later
656666 context.data.keypressed = ( e.keyCode == undefined ) ? e.which : e.keyCode;
657667 context.data.keypressedCount = 0;
 668+
 669+ switch ( context.data.keypressed ) {
 670+ // This preventDefault logic is duplicated from
 671+ // $.suggestions.keypress(), which sucks
 672+ case 40:
 673+ e.preventDefault();
 674+ e.stopImmediatePropagation();
 675+ break;
 676+ case 38:
 677+ case 27:
 678+ case 13:
 679+ if ( context.data.$container.is( ':visible' ) ) {
 680+ e.preventDefault();
 681+ e.stopImmediatePropagation();
 682+ }
 683+ }
658684 } )
659 - .keypress( function() {
 685+ .keypress( function( e ) {
660686 context.data.keypressedCount++;
661 - $.suggestions.keypress( context, context.data.keypressed );
 687+ $.suggestions.keypress( e, context, context.data.keypressed );
662688 } )
663689 .keyup( function() {
664690 // Some browsers won't throw keypress() for arrow keys. If we got a keydown and a keyup without a
665691 // keypress in between, solve it
666692 if ( context.data.keypressedCount == 0 ) {
667 - $.suggestions.keypress( context, context.data.keypressed );
 693+ $.suggestions.keypress( e, context, context.data.keypressed );
668694 }
669695 } )
670696 .blur( function() {
Index: trunk/extensions/UsabilityInitiative/js/plugins.combined.min.js
——@@ -32,15 +32,16 @@
3333 result=selected;}}
3434 selected.removeClass('suggestions-result-current');result.addClass('suggestions-result-current');}
3535 if(updateTextbox){if(result.size()==0){$.suggestions.restore(context);}else{context.data.$textbox.val(result.data('text'));context.data.$textbox.change();}}
36 -$.suggestions.special(context);},keypress:function(context,key){switch(key){case 40:if(context.data.$container.is(':visible')){$.suggestions.highlight(context,'next',true);}else{$.suggestions.update(context,false);}
37 -context.data.$textbox.trigger('change');break;case 38:if(context.data.$container.is(':visible')){$.suggestions.highlight(context,'prev',true);}
38 -context.data.$textbox.trigger('change');break;case 27:context.data.$container.hide();$.suggestions.restore(context);$.suggestions.cancel(context);context.data.$textbox.trigger('change');break;case 13:context.data.$container.hide();break;default:$.suggestions.update(context,true);break;}}};$.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':1200,'submitOnClick':false}};}
 36+$.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);}
 37+context.data.$textbox.trigger('change');preventDefault=true;break;case 38:if(wasVisible){$.suggestions.highlight(context,'prev',true);}
 38+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;break;default:$.suggestions.update(context,true);break;}
 39+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':1200,'submitOnClick':false}};}
3940 if(args.length>0){if(typeof args[0]=='object'){for(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]]);}}}
4041 if(typeof context.data=='undefined'){context.data={'timerID':null,'prevText':null,'visibleResults':0,'mouseDownOn':$([]),'$textbox':$(this)};context.data.$container=$('<div />').css({'top':Math.round(context.data.$textbox.offset().top+context.data.$textbox.outerHeight()),'left':Math.round(context.data.$textbox.offset().left),'width':context.data.$textbox.outerWidth(),'display':'none'}).mouseover(function(e){$.suggestions.highlight(context,$(e.target).closest('.suggestions-results div'),false);}).addClass('suggestions').append($('<div />').addClass('suggestions-results').mousedown(function(e){context.data.mouseDownOn=$(e.target).closest('.suggestions-results div');}).mouseup(function(e){var $result=$(e.target).closest('.suggestions-results div');var $other=context.data.mouseDownOn;context.data.mouseDownOn=$([]);if($result.get(0)!=$other.get(0)){return;}
4142 $.suggestions.highlight(context,$result,true);context.data.$container.hide();if(typeof context.config.result.select=='function'){context.config.result.select.call($result,context.data.$textbox);}
4243 context.data.$textbox.focus();})).append($('<div />').addClass('suggestions-special').mousedown(function(e){context.data.mouseDownOn=$(e.target).closest('.suggestions-special');}).mouseup(function(e){var $special=$(e.target).closest('.suggestions-special');var $other=context.data.mouseDownOn;context.data.mouseDownOn=$([]);if($special.get(0)!=$other.get(0)){return;}
4344 context.data.$container.hide();if(typeof context.config.special.select=='function'){context.config.special.select.call($special,context.data.$textbox);}
44 -context.data.$textbox.focus();})).appendTo($('body'));$(this).attr('autocomplete','off').keydown(function(e){context.data.keypressed=(e.keyCode==undefined)?e.which:e.keyCode;context.data.keypressedCount=0;}).keypress(function(){context.data.keypressedCount++;$.suggestions.keypress(context,context.data.keypressed);}).keyup(function(){if(context.data.keypressedCount==0){$.suggestions.keypress(context,context.data.keypressed);}}).blur(function(){if(context.data.mouseDownOn.size()>0){return;}
 45+context.data.$textbox.focus();})).appendTo($('body'));$(this).attr('autocomplete','off').keydown(function(e){context.data.keypressed=(e.keyCode==undefined)?e.which:e.keyCode;context.data.keypressedCount=0;switch(context.data.keypressed){case 40:e.preventDefault();e.stopImmediatePropagation();break;case 38:case 27:case 13:if(context.data.$container.is(':visible')){e.preventDefault();e.stopImmediatePropagation();}}}).keypress(function(e){context.data.keypressedCount++;$.suggestions.keypress(e,context,context.data.keypressed);}).keyup(function(){if(context.data.keypressedCount==0){$.suggestions.keypress(e,context,context.data.keypressed);}}).blur(function(){if(context.data.mouseDownOn.size()>0){return;}
4546 context.data.$container.hide();$.suggestions.cancel(context);});}
4647 $(this).data('suggestions-context',context);});return returnValue!==null?returnValue:$(this);};})(jQuery);(function($){$.fn.extend({getSelection:function(){var e=this.jquery?this[0]:this;var retval='';if(e.style.display=='none'){}else if(document.selection&&document.selection.createRange){var range=document.selection.createRange();retval=range.text;}else if(e.selectionStart||e.selectionStart=='0'){retval=e.value.substring(e.selectionStart,e.selectionEnd);}
4748 return retval;},encapsulateSelection:function(pre,peri,post,ownline,replace){function checkSelectedText(){if(!selText){selText=peri;isSample=true;}else if(replace){selText=peri;}else if(selText.charAt(selText.length-1)==' '){selText=selText.substring(0,selText.length-1);post+=' '}}

Follow-up revisions

RevisionCommit summaryAuthorDate
r56948EditToolbar: (bug 20605) Also trigger Insert/Replace button when Enter key is...catrope11:23, 26 September 2009

Comments

#Comment by Simetrical (talk | contribs)   21:05, 9 November 2009

I'm confused, what was the problem here? Hitting Enter when a form field is focused should always submit the form, even in the absence of a submit button, as this test case illustrates:

data:text/html,<!doctype html><form action="http://en.wikipedia.org"><input>

Why should adding a hidden submit button ever do anything?

#Comment by Catrope (talk | contribs)   21:18, 9 November 2009

I don't recall exactly why I did it, as this revision is nearly two months old, but I guess I wouldn't have added hidden submit buttons if they weren't necessary in some browsers. I'll try to remove them and see what happens when I have time.

#Comment by Tim Starling (talk | contribs)   07:51, 1 December 2009

IIRC the major browsers have some odd heuristic behaviour when you press enter in a form field.

Status & tagging log