Skip to content

Selectmenu review #1224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Selectmenu review #1224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b73efa
Selectmenu: Cleanup
scottgonzalez Apr 8, 2014
a581b04
Selectmenu: Don't use active items at all
scottgonzalez Apr 8, 2014
1dfaa19
Selectmenu: Simplify selection of focused item via keyboard
scottgonzalez Apr 8, 2014
52b6eca
[amend]: Remove logging
scottgonzalez Apr 8, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions tests/unit/selectmenu/selectmenu_core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,36 +185,36 @@ $.each([
});

asyncTest( "item focus and active state - " + settings.type, function () {
expect( 8 );
expect( 4 );

var element = $( settings.selector ).selectmenu(),
var items, focusedItem, activeItem,
element = $( settings.selector ).selectmenu(),
button = element.selectmenu( "widget" ),
menu = element.selectmenu( "menuWidget" ),
links, focusedItem, activeItem;
menu = element.selectmenu( "menuWidget" );

// init menu
// Initialize menu
button.simulate( "focus" );

setTimeout(function() {
links = menu.find( "li.ui-menu-item" );
items = menu.find( "li.ui-menu-item" );

button.trigger( "click" );
setTimeout(function() {
checkItemClasses();

links.eq( 3 ).simulate( "mouseover" ).trigger( "click" );
items.eq( 3 ).simulate( "mouseover" ).trigger( "click" );

button.trigger( "click" );
links.eq( 2 ).simulate( "mouseover" );
items.eq( 2 ).simulate( "mouseover" );
$( document ).trigger( "click" );

button.trigger( "click" );
links.eq( 1 ).simulate( "mouseover" );
items.eq( 1 ).simulate( "mouseover" );
$( document ).trigger( "click" );

button.trigger( "click" );
setTimeout(function() {
checkItemClasses();
checkItemClasses();
start();
}, 350 );
}, 350 );
Expand All @@ -223,11 +223,7 @@ $.each([
function checkItemClasses() {
focusedItem = menu.find( "li.ui-state-focus" );
equal( focusedItem.length, 1, "only one item has ui-state-focus class" );
equal( focusedItem.attr( "id" ), links.eq( element[ 0 ].selectedIndex ).attr( "id" ), "selected item has ui-state-focus class" );

activeItem = menu.find( "li.ui-state-active" );
equal( activeItem.length, 1, "only one item has ui-state-active class" );
equal( activeItem.attr( "id" ), links.eq( element[ 0 ].selectedIndex ).attr( "id" ), "selected item has ui-state-active class" );
equal( focusedItem.attr( "id" ), items.eq( element[ 0 ].selectedIndex ).attr( "id" ), "selected item has ui-state-focus class" );
}
});

Expand Down
132 changes: 70 additions & 62 deletions ui/selectmenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,24 +92,29 @@ return $.widget( "ui.selectmenu", {
"aria-owns": this.ids.menu,
"aria-haspopup": "true"
})
.insertAfter( this.element );
.insertAfter( this.element );

$( "<span>", {
"class": "ui-icon " + this.options.icons.button
}).prependTo( this.button );
})
.prependTo( this.button );

this.buttonText = $( "<span>", {
"class": "ui-selectmenu-text"
})
.appendTo( this.button );
.appendTo( this.button );

this._setText( this.buttonText, this.element.find( "option:selected" ).text() );
this._setOption( "width", this.options.width );

this._on( this.button, this._buttonEvents );
this.button.one( "focusin", function() {
// Delay rendering the menu items until the button receives focus
that._refreshMenu();

// Delay rendering the menu items until the button receives focus.
// The menu may have already been rendered via a programmatic open.
if ( !that.menuItems ) {
that._refreshMenu();
}
});
this._hoverable( this.button );
this._focusable( this.button );
Expand All @@ -129,47 +134,49 @@ return $.widget( "ui.selectmenu", {
this.menuWrap = $( "<div>", {
"class": "ui-selectmenu-menu ui-front"
})
.append( this.menu )
.appendTo( this._appendTo() );
.append( this.menu )
.appendTo( this._appendTo() );

// Initialize menu widget
this.menuInstance = this.menu.menu({
role: "listbox",
select: function( event, ui ) {
event.preventDefault();
that._select( ui.item.data( "ui-selectmenu-item" ), event );
},
focus: function( event, ui ) {
var item = ui.item.data( "ui-selectmenu-item" );

// Prevent inital focus from firing and checks if its a newly focused item
if ( that.focusIndex != null && item.index !== that.focusIndex ) {
that._trigger( "focus", event, { item: item } );
if ( !that.isOpen ) {
that._select( item, event );
this.menuInstance = this.menu
.menu({
role: "listbox",
select: function( event, ui ) {
event.preventDefault();
that._select( ui.item.data( "ui-selectmenu-item" ), event );
},
focus: function( event, ui ) {
var item = ui.item.data( "ui-selectmenu-item" );

// Prevent inital focus from firing and check if its a newly focused item
if ( that.focusIndex != null && item.index !== that.focusIndex ) {
that._trigger( "focus", event, { item: item } );
if ( !that.isOpen ) {
that._select( item, event );
}
}
}
that.focusIndex = item.index;
that.focusIndex = item.index;

that.button.attr( "aria-activedescendant",
that.menuItems.eq( item.index ).attr( "id" ) );
}
})
.menu( "instance" );
that.button.attr( "aria-activedescendant",
that.menuItems.eq( item.index ).attr( "id" ) );
}
})
.menu( "instance" );

// Adjust menu styles to dropdown
this.menu.addClass( "ui-corner-bottom" ).removeClass( "ui-corner-all" );
this.menu
.addClass( "ui-corner-bottom" )
.removeClass( "ui-corner-all" );

// TODO: Can we make this cleaner?
// If not, at least update the comment to say what we're removing
// Unbind uneeded menu events
// Don't close the menu on mouseleave
this.menuInstance._off( this.menu, "mouseleave" );

// Cancel the menu's collapseAll on document click
this.menuInstance._closeOnDocumentClick = function() {
return false;
};

// Selects often contain empty items, but never contain dividers
this.menuInstance._isDivider = function() {
return false;
};
Expand All @@ -191,7 +198,7 @@ return $.widget( "ui.selectmenu", {
return;
}

this._readOptions( options );
this._parseOptions( options );
this._renderMenu( this.menu, this.items );

this.menuInstance.refresh();
Expand All @@ -216,11 +223,10 @@ return $.widget( "ui.selectmenu", {
if ( !this.menuItems ) {
this._refreshMenu();
} else {
// TODO: Why is this necessary?
// Shouldn't the underlying menu always have accurate state?

// Menu clears focus on close, reset focus to selected item
this.menu.find( ".ui-state-focus" ).removeClass( "ui-state-focus" );
this.menuInstance.focus( null, this._getSelectedItem() );
this.menuItems.eq( this.element[ 0 ].selectedIndex ).addClass( "ui-state-active" );
}

this.isOpen = true;
Expand All @@ -245,11 +251,6 @@ return $.widget( "ui.selectmenu", {
this.isOpen = false;
this._toggleAttr();

// Check if we have an item to select
if ( this.menuItems ) {
this.menuInstance.active = this._getSelectedItem();
}

this._off( this.document );

this._trigger( "close", event );
Expand All @@ -271,14 +272,16 @@ return $.widget( "ui.selectmenu", {
if ( item.optgroup !== currentOptgroup ) {
$( "<li>", {
"class": "ui-selectmenu-optgroup ui-menu-divider" +
( item.element.parent( "optgroup" ).attr( "disabled" ) ?
( item.element.parent( "optgroup" ).prop( "disabled" ) ?
" ui-state-disabled" :
"" ),
text: item.optgroup
})
.appendTo( ul );
.appendTo( ul );

currentOptgroup = item.optgroup;
}

that._renderItemData( ul, item );
});
},
Expand Down Expand Up @@ -307,8 +310,8 @@ return $.widget( "ui.selectmenu", {
},

_move: function( direction, event ) {
var filter = ".ui-menu-item",
item, next;
var item, next,
filter = ".ui-menu-item";

if ( this.isOpen ) {
item = this.menuItems.eq( this.focusIndex );
Expand All @@ -324,7 +327,7 @@ return $.widget( "ui.selectmenu", {
}

if ( next.length ) {
this.menu.menu( "focus", event, next );
this.menuInstance.focus( event, next );
}
},

Expand All @@ -333,16 +336,16 @@ return $.widget( "ui.selectmenu", {
},

_toggle: function( event ) {
if ( this.isOpen ) {
this.close( event );
} else {
this.open( event );
}
this[ this.isOpen ? "close" : "open" ]( event );
},

_documentClick: {
mousedown: function( event ) {
if ( this.isOpen && !$( event.target ).closest( ".ui-selectmenu-menu, #" + this.ids.button ).length ) {
if ( !this.isOpen ) {
return;
}

if ( !$( event.target ).closest( ".ui-selectmenu-menu, #" + this.ids.button ).length ) {
this.close( event );
}
}
Expand All @@ -360,7 +363,7 @@ return $.widget( "ui.selectmenu", {
break;
case $.ui.keyCode.ENTER:
if ( this.isOpen ) {
this._selectMenu( event );
this._selectFocusedItem( event );
}
break;
case $.ui.keyCode.UP:
Expand All @@ -379,7 +382,7 @@ return $.widget( "ui.selectmenu", {
break;
case $.ui.keyCode.SPACE:
if ( this.isOpen ) {
this._selectMenu( event );
this._selectFocusedItem( event );
} else {
this._toggle( event );
}
Expand Down Expand Up @@ -409,9 +412,10 @@ return $.widget( "ui.selectmenu", {
}
},

_selectMenu: function( event ) {
if ( !this.menuItems.eq( this.focusIndex ).hasClass( "ui-state-disabled" ) ) {
this.menuInstance.select( event );
_selectFocusedItem: function( event ) {
var item = this.menuItems.eq( this.focusIndex );
if ( !item.hasClass( "ui-state-disabled" ) ) {
this._select( item.data( "ui-selectmenu-item" ), event );
}
},

Expand Down Expand Up @@ -453,6 +457,7 @@ return $.widget( "ui.selectmenu", {
if ( key === "appendTo" ) {
this.menuWrap.appendTo( this._appendTo() );
}

if ( key === "disabled" ) {
this.menuInstance.option( "disabled", value );
this.button
Expand All @@ -467,6 +472,7 @@ return $.widget( "ui.selectmenu", {
this.button.attr( "tabindex", 0 );
}
}

if ( key === "width" ) {
if ( !value ) {
value = this.element.outerWidth();
Expand Down Expand Up @@ -498,15 +504,17 @@ return $.widget( "ui.selectmenu", {
_toggleAttr: function() {
this.button
.toggleClass( "ui-corner-top", this.isOpen )
.toggleClass( "ui-corner-all", !this.isOpen );
.toggleClass( "ui-corner-all", !this.isOpen )
.attr( "aria-expanded", this.isOpen );
this.menuWrap.toggleClass( "ui-selectmenu-open", this.isOpen );
this.menu.attr( "aria-hidden", !this.isOpen );
this.button.attr( "aria-expanded", this.isOpen );
},

_resizeMenu: function() {
this.menu.outerWidth( Math.max(
this.button.outerWidth(),

// support: IE10
// IE10 wraps long text (possibly a rounding bug)
// so we add 1px to avoid the wrapping
this.menu.width( "" ).outerWidth() + 1
Expand All @@ -517,9 +525,9 @@ return $.widget( "ui.selectmenu", {
return { disabled: this.element.prop( "disabled" ) };
},

_readOptions: function( options ) {
_parseOptions: function( options ) {
var data = [];
options.each( function( index, item ) {
options.each(function( index, item ) {
var option = $( item ),
optgroup = option.parent( "optgroup" );
data.push({
Expand All @@ -528,7 +536,7 @@ return $.widget( "ui.selectmenu", {
value: option.attr( "value" ),
label: option.text(),
optgroup: optgroup.attr( "label" ) || "",
disabled: optgroup.attr( "disabled" ) || option.attr( "disabled" )
disabled: optgroup.prop( "disabled" ) || option.prop( "disabled" )
});
});
this.items = data;
Expand Down