Menu: Minor cleanup.

Author: scottgonzalez

ui/jquery.ui.menu.js

@@ -36,11 +36,9 @@ $.widget( "ui.menu", {
 	_create: function() {
 		this.activeMenu = this.element;
 		this.menuId = this.element.attr( "id" ) || "ui-menu-" + idIncrement++;
-		if ( this.element.find( ".ui-icon" ).length ) {
-			this.element.addClass( "ui-menu-icons" );
-		}
 		this.element
 			.addClass( "ui-menu ui-widget ui-widget-content ui-corner-all" )
+			.toggleClass( "ui-menu-icons", !!this.element.find( ".ui-icon" ).length )
 			.attr({
 				id: this.menuId,
 				role: this.options.role,
@@ -95,9 +93,9 @@ $.widget( "ui.menu", {
 				target.siblings().children( ".ui-state-active" ).removeClass( "ui-state-active" );
 				this.focus( event, target );
 			},
-			"mouseleave": "collapseAll",
+			mouseleave: "collapseAll",
 			"mouseleave .ui-menu": "collapseAll",
-			"focus": function( event ) {
+			focus: function( event ) {
 				var menu = this.element,
 					firstItem = menu.children( ".ui-menu-item" ).eq( 0 );
 				if ( this._hasScroll() && !this.active ) {
@@ -120,11 +118,13 @@ $.widget( "ui.menu", {
 					}
 				});
 			},
-			"keydown": "_keydown"
+			keydown: "_keydown"
 		});
 
 		this.refresh();
 
+		// TODO: We probably shouldn't bind to document for each menu.
+		// TODO: This isn't being cleaned up on destroy.
 		this._bind( this.document, {
 			click: function( event ) {
 				if ( !$( event.target ).closest( ".ui-menu" ).length ) {
@@ -149,7 +149,6 @@ $.widget( "ui.menu", {
 
 		// destroy menu items
 		this.element.find( ".ui-menu-item" )
-			.unbind( ".menu" )
 			.removeClass( "ui-menu-item" )
 			.removeAttr( "role" )
 			.children( "a" )
@@ -158,6 +157,7 @@ $.widget( "ui.menu", {
 				.removeAttr( "role" )
 				.removeAttr( "aria-haspopup" )
 				.removeAttr( "id" )
+				// TODO: is this correct? Don't these exist in the original markup?
 				.children( ".ui-icon" )
 					.remove();
 
@@ -166,82 +166,80 @@ $.widget( "ui.menu", {
 	},
 
 	_keydown: function( event ) {
+		var match, prev, character, skip,
+			preventDefault = true;
+
+		function escape( value ) {
+			return value.replace( /[\-\[\]{}()*+?.,\\\^$|#\s]/g, "\\$&" );
+		}
+
 		switch ( event.keyCode ) {
 		case $.ui.keyCode.PAGE_UP:
 			this.previousPage( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.PAGE_DOWN:
 			this.nextPage( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.HOME:
 			this._move( "first", "first", event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.END:
 			this._move( "last", "last", event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.UP:
 			this.previous( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.DOWN:
 			this.next( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.LEFT:
 			this.collapse( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.RIGHT:
 			if ( !this.active.is( ".ui-state-disabled" ) ) {
 				this.expand( event );
 			}
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.ENTER:
 			this._activate( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.SPACE:
 			this._activate( event );
-			event.preventDefault();
 			break;
 		case $.ui.keyCode.ESCAPE:
 			this.collapse( event );
-			event.preventDefault();
 			break;
 		default:
+			preventDefault = false;
+			prev = this.previousFilter || "";
+			character = String.fromCharCode( event.keyCode );
+			skip = false;
+
 			clearTimeout( this.filterTimer );
-			var match,
-				prev = this.previousFilter || "",
-				character = String.fromCharCode( event.keyCode ),
-				skip = false;
 
 			if ( character === prev ) {
 				skip = true;
 			} else {
 				character = prev + character;
 			}
-			function escape( value ) {
-				return value.replace(/[\-\[\]{}()*+?.,\\\^$|#\s]/g, "\\$&");
-			}
+
 			match = this.activeMenu.children( ".ui-menu-item" ).filter(function() {
 				return new RegExp( "^" + escape( character ), "i" )
 					.test( $( this ).children( "a" ).text() );
 			});
-			match = skip && match.index(this.active.next()) !== -1 ?
-				this.active.nextAll(".ui-menu-item") :
+			match = skip && match.index( this.active.next() ) !== -1 ?
+				this.active.nextAll( ".ui-menu-item" ) :
 				match;
+
+			// TODO: document what's going on here, character is reset to the original value
 			if ( !match.length ) {
-				character = String.fromCharCode(event.keyCode);
-				match = this.activeMenu.children(".ui-menu-item").filter(function() {
+				character = String.fromCharCode( event.keyCode );
+				match = this.activeMenu.children( ".ui-menu-item" ).filter(function() {
 					return new RegExp( "^" + escape(character), "i" )
 						.test( $( this ).children( "a" ).text() );
 				});
 			}
+
 			if ( match.length ) {
 				this.focus( event, match );
 				if ( match.length > 1 ) {
@@ -256,6 +254,10 @@ $.widget( "ui.menu", {
 				delete this.previousFilter;
 			}
 		}
+
+		if ( preventDefault ) {
+			event.preventDefault();
+		}
 	},
 
 	_activate: function( event ) {
@@ -289,10 +291,12 @@ $.widget( "ui.menu", {
 			.attr( "role", "presentation" )
 			.children( "a" )
 				.addClass( "ui-corner-all" )
-				.attr( "tabIndex", -1 )
-				.attr( "role", this._itemRole() )
-				.attr( "id", function( i ) {
-					return menuId + "-" + i;
+				.attr({
+					tabIndex: -1,
+					role: this._itemRole(),
+					id: function( i ) {
+						return menuId + "-" + i;
+					}
 				});
 
 		// initialize unlinked menu-items as dividers
@@ -305,7 +309,8 @@ $.widget( "ui.menu", {
 			var menu = $( this ),
 				item = menu.prev( "a" );
 
-			item.attr( "aria-haspopup", "true" )
+			item
+				.attr( "aria-haspopup", "true" )
 				.prepend( '<span class="ui-menu-icon ui-icon ui-icon-carat-1-e"></span>' );
 			menu.attr( "aria-labelledby", item.attr( "id" ) );
 		});
@@ -333,7 +338,11 @@ $.widget( "ui.menu", {
 		}
 
 		// highlight active parent menu item, if any
-		this.active.parent().closest( ".ui-menu-item" ).children( "a:first" ).addClass( "ui-state-active" );
+		this.active
+			.parent()
+			.closest( ".ui-menu-item" )
+			.children( "a:first" )
+			.addClass( "ui-state-active" );
 
 		if ( event && event.type === "keydown" ) {
 			this._close();
@@ -401,18 +410,18 @@ $.widget( "ui.menu", {
 	},
 
 	_open: function( submenu ) {
+		var position = $.extend({
+			of: this.active
+		}, $.type( this.options.position ) === "function" ?
+			this.options.position( this.active ) :
+			this.options.position
+		);
+
 		clearTimeout( this.timer );
 		this.element.find( ".ui-menu" ).not( submenu.parents() )
 			.hide()
 			.attr( "aria-hidden", "true" );
 
-		var position = $.extend( {}, {
-				of: this.active
-			}, $.type( this.options.position ) === "function" ?
-				this.options.position( this.active ) :
-				this.options.position
-			);
-
 		submenu
 			.show()
 			.removeAttr( "aria-hidden" )
@@ -476,7 +485,7 @@ $.widget( "ui.menu", {
 		if ( newItem && newItem.length ) {
 			this._open( newItem.parent() );
 
-			//timeout so Firefox will not hide activedescendant change in expanding submenu from AT
+			// timeout so Firefox will not hide activedescendant change in expanding submenu from AT
 			this._delay(function() {
 				this.focus( event, newItem );
 			}, 20 );