Skip to content
Browse files

Selectmenu: Code review.

  • Loading branch information...
1 parent 134fcaf commit f416a66f59e628bc635e120a39927be0fef139d1 @scottgonzalez committed Sep 16, 2013
Showing with 61 additions and 56 deletions.
  1. +61 −56 ui/jquery.ui.selectmenu.js
View
117 ui/jquery.ui.selectmenu.js
@@ -57,7 +57,7 @@ $.widget( "ui.selectmenu", {
_drawButton: function() {
var tabindex = this.element.attr( "tabindex" );
- // Fix existing label
+ // Associate existing label with the new button
this.label = $( "label[for='" + this.ids.element + "']" ).attr( "for", this.ids.button );
this._on( this.label, {
click: function( event ) {
@@ -66,7 +66,7 @@ $.widget( "ui.selectmenu", {
}
});
- // Hide original select tag
+ // Hide original select element
this.element.hide();
// Create button
@@ -101,7 +101,7 @@ $.widget( "ui.selectmenu", {
_drawMenu: function() {
var that = this;
- // Create menu portion, append to body
+ // Create menu
this.menu = $( "<ul>", {
"aria-hidden": "true",
"aria-labelledby": this.ids.button,
@@ -110,28 +110,24 @@ $.widget( "ui.selectmenu", {
// Wrap menu
this.menuWrap = $( "<div>", {
- "class": "ui-selectmenu-menu ui-front",
- outerWidth: this.button.outerWidth()
- })
- .append( this.menu )
- .appendTo( this._appendTo() );
+ "class": "ui-selectmenu-menu ui-front",
+ outerWidth: this.button.outerWidth()
+ })
+ .append( this.menu )
+ .appendTo( this._appendTo() );
- // Init menu widget
+ // Initialize menu widget
this.menuInstance = this.menu.menu({
+ role: "listbox",
@scottgonzalez
Owner

I moved this option to the top so that it's easier to see how we're using the menu widget (options then callbacks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
select: function( event, ui ) {
var item = ui.item.data( "ui-selectmenu-item" );
that._select( item, event );
-
- if ( that.isOpen ) {
@scottgonzalez
Owner

The close() method does an early exit if the menu isn't open anyway. As far as I can tell, there's no default action to prevent.

@fnagel
fnagel added a note Sep 25, 2013

Removing the preventDefault will cause the the anchor link to be followed. Restored for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- event.preventDefault();
- that.close( 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
+ // 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 ) {
@@ -140,16 +136,18 @@ $.widget( "ui.selectmenu", {
}
that.focusIndex = item.index;
- that.button.attr( "aria-activedescendant", that.menuItems.eq( item.index ).attr( "id" ) );
- },
- role: "listbox"
+ that.button.attr( "aria-activedescendant",
+ that.menuItems.eq( item.index ).attr( "id" ) );
+ }
})
.menu( "instance" );
- // adjust menu styles to dropdown
+ // Adjust menu styles to dropdown
this.menu.addClass( "ui-corner-bottom" ).removeClass( "ui-corner-all" );
- // Unbind uneeded Menu events
+ // TODO: Can we make this cleaner?
@scottgonzalez
Owner

We should try to update menu so that there's a saner way to handle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // If not, at least update the comment to say what we're removing
+ // Unbind uneeded menu events
this.menuInstance._off( this.menu, "mouseleave" );
// Cancel the menu's collapseAll on document click
@@ -171,32 +169,34 @@ $.widget( "ui.selectmenu", {
this._readOptions( options );
this._renderMenu( this.menu, this.items );
- this.menu.menu( "refresh" );
+ this.menuInstance.refresh();
this.menuItems = this.menu.find( "li" ).not( ".ui-selectmenu-optgroup" ).find( "a" );
item = this._getSelectedItem();
- // Make sure menu is selected item aware
- this.menu.menu( "focus", null, item );
+ // Update the menu to have the correct item focused
+ this.menuInstance.focus( null, item );
this._setAria( item.data( "ui-selectmenu-item" ) );
this._setText( this.buttonText, item.text() );
// Set disabled state
- this._setOption( "disabled", !!this.element.prop( "disabled" ) );
@fnagel
fnagel added a note Sep 16, 2013

Would you mind explain to me why you remove that? Jörn advised me to do so but I cant remember why.

@scottgonzalez
Owner

Because the disabled property is a boolean, so .prop( "disabled" ) doesn't need to be typecast. This is different from working with the attribute which will always be a string, though it can be falsey.

@fnagel
fnagel added a note Sep 16, 2013

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ this._setOption( "disabled", this.element.prop( "disabled" ) );
},
open: function( event ) {
if ( this.options.disabled ) {
return;
}
- // Support: IE6-IE9 click doesn't trigger focus on the button
+ // If this is the first time the menu is being opened, render the items
if ( !this.menuItems ) {
this.refresh();
} else {
+ // TODO: Why is this necessary?
@scottgonzalez
Owner

I can't tell why the menu gets out of sync, but it seems odd that we need to reset the state like this on every open.

@fnagel
fnagel added a note Sep 16, 2013

Why do we remove the browser support comment? I was told to add them a few weeks ago :-)

It's a workaround we implemented to remove menuInstance.delay setting in selectmenu.
ticket: http://bugs.jqueryui.com/ticket/8929
commit: jquery@5a24ee0

@scottgonzalez
Owner

I removed it because during my testing, I couldn't find any indication that this comment was accurate. oldIE was behaving just fine if the items were already rendered, and all browsers need to check if this is the first rendering (for example: elem.selectmenu().selectmenu( "open" )).

@fnagel
fnagel added a note Sep 16, 2013

Mhh I've tested with modern.ie (aka MS built) VirtualBox images -- should be pretty exact.
Are you sure you used mouse and keyboard with different usecases?

Opening via API only is a usecase we missed so far. So the comment is definitely incorrect.

@scottgonzalez
Owner

Regarding the workaround, maybe there's another adjustment that should happen inside menu. I'll try to dig into it.

@fnagel
fnagel added a note Sep 16, 2013

That would be great. Please consider fixing this bug too as its related: http://bugs.jqueryui.com/ticket/9532

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // Shouldn't the underlying menu always have accurate state?
this.menu.find( ".ui-state-focus" ).removeClass( "ui-state-focus" );
- this.menu.menu( "focus", null, this._getSelectedItem() );
+ this.menuInstance.focus( null, this._getSelectedItem() );
}
this.isOpen = true;
@@ -245,9 +245,13 @@ $.widget( "ui.selectmenu", {
$.each( items, function( index, item ) {
if ( item.optgroup !== currentOptgroup ) {
$( "<li>", {
- "class": "ui-selectmenu-optgroup" + ( item.element.parent( "optgroup" ).attr( "disabled" ) ? " ui-state-disabled" : "" ),
+ "class": "ui-selectmenu-optgroup" +
+ ( item.element.parent( "optgroup" ).attr( "disabled" ) ?
+ " ui-state-disabled" :
+ "" ),
text: item.optgroup
- }).appendTo( ul );
+ })
+ .appendTo( ul );
currentOptgroup = item.optgroup;
}
that._renderItemData( ul, item );
@@ -283,7 +287,8 @@ $.widget( "ui.selectmenu", {
// Set focus manually for first or last item
this.menu.menu( "focus", event, this.menuItems[ direction ]() );
} else {
- if ( direction === "previous" && this.menu.menu( "isFirstItem" ) || direction === "next" && this.menu.menu( "isLastItem" ) ) {
+ if ( direction === "previous" && this.menu.menu( "isFirstItem" ) ||
+ direction === "next" && this.menu.menu( "isLastItem" ) ) {
return;
}
@@ -313,30 +318,25 @@ $.widget( "ui.selectmenu", {
},
_buttonEvents: {
- focus: function() {
- // Init Menu on first focus
- this.refresh();
- // Reset focus class as its removed by ui.widget._setOption
@scottgonzalez
Owner

I fixed _setOption() in the base widget in master and merged master into selectmenu so that this is no longer necessary.

@fnagel
fnagel added a note Sep 16, 2013

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- this.button.addClass( "ui-state-focus" );
- this._off( this.button, "focus" );
- },
- click: function( event ) {
- this._toggle( event );
- event.preventDefault();
+ focusin: function() {
@scottgonzalez
Owner

Clicking on the icon in IE didn't work, but using focusin it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // Delay rendering the menu items until the button receives focus
+ if ( !this.menuItems ) {
@scottgonzalez
Owner

In most cases, this check isn't necessary, but if the menu is opened programmatically before it receives focus, then refreshing is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ this.refresh();
+ }
+ this._off( this.button, "focusin" );
},
+ click: "_toggle",
@scottgonzalez
Owner

I couldn't find a reason to prevent the default action of the click. Perhaps just a leftover from when there was an anchor for the button?

@fnagel
fnagel added a note Sep 16, 2013

Thats possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
keydown: function( event ) {
- var prevDef = true;
+ var preventDefault = true;
switch ( event.keyCode ) {
case $.ui.keyCode.TAB:
case $.ui.keyCode.ESCAPE:
- if ( this.isOpen ) {
- this.close( event );
- }
- prevDef = false;
+ this.close( event );
+ preventDefault = false;
break;
case $.ui.keyCode.ENTER:
if ( this.isOpen ) {
- this.menu.menu( "select", event );
+ this.menuInstance.select( event );
}
break;
case $.ui.keyCode.UP:
@@ -355,7 +355,7 @@ $.widget( "ui.selectmenu", {
break;
case $.ui.keyCode.SPACE:
if ( this.isOpen ) {
- this.menu.menu( "select", event );
+ this.menuInstance.select( event );
} else {
this._toggle( event );
}
@@ -376,16 +376,18 @@ $.widget( "ui.selectmenu", {
break;
default:
this.menu.trigger( event );
- prevDef = false;
+ preventDefault = false;
}
- if ( prevDef ) {
+
+ if ( preventDefault ) {
event.preventDefault();
}
}
},
_select: function( item, event ) {
var oldIndex = this.element[ 0 ].selectedIndex;
+
// Change native select element
this.element[ 0 ].selectedIndex = item.index;
this._setText( this.buttonText, item.label );
@@ -395,6 +397,8 @@ $.widget( "ui.selectmenu", {
if ( item.index !== oldIndex ) {
this._trigger( "change", event, { item: item } );
}
+
+ this.close( event );
},
_setAria: function( item ) {
@@ -421,17 +425,16 @@ $.widget( "ui.selectmenu", {
this.menuWrap.appendTo( this._appendTo() );
}
if ( key === "disabled" ) {
- this.menu.menu( "option", "disabled", value );
+ this.menuInstance.option( "disabled", value );
this.button
- .toggleClass( "ui-state-disabled", !!value )
+ .toggleClass( "ui-state-disabled", value )
.attr( "aria-disabled", value );
+ this.element.prop( "disabled", value );
if ( value ) {
- this.element.attr( "disabled", "disabled" );
this.button.attr( "tabindex", -1 );
this.close();
} else {
- this.element.removeAttr( "disabled" );
this.button.attr( "tabindex", 0 );
}
}
@@ -458,14 +461,16 @@ $.widget( "ui.selectmenu", {
},
_toggleAttr: function(){
- this.button.toggleClass( "ui-corner-top", this.isOpen ).toggleClass( "ui-corner-all", !this.isOpen );
+ this.button
+ .toggleClass( "ui-corner-top", this.isOpen )
+ .toggleClass( "ui-corner-all", !this.isOpen );
this.menuWrap.toggleClass( "ui-selectmenu-open", this.isOpen );
- this.menu.attr( "aria-hidden", !this.isOpen);
- this.button.attr( "aria-expanded", this.isOpen);
+ this.menu.attr( "aria-hidden", !this.isOpen );
+ this.button.attr( "aria-expanded", this.isOpen );
},
_getCreateOptions: function() {
- return { disabled: !!this.element.prop( "disabled" ) };
+ return { disabled: this.element.prop( "disabled" ) };
},
_readOptions: function( options ) {

6 comments on commit f416a66

@fnagel
fnagel commented on f416a66 Sep 16, 2013

I noticed just one issue: click on item follows link anchor.

@fnagel
fnagel commented on f416a66 Sep 23, 2013

There is a code duplication issue we already talked about earlier: the _appendTo method is currently implemented in autocomplete, dialog and in selectmenu. Shall we move this to ui.widget?

@scottgonzalez
Owner

There is a code duplication issue we already talked about earlier: the _appendTo method is currently implemented in autocomplete, dialog and in selectmenu. Shall we move this to ui.widget?

Not yet. I'm really hesitant to add anything to the base widget. I've been playing around with ideas for removing the duplication though.

@scottgonzalez
Owner

I noticed just one issue: click on item follows link anchor.

We're working on changes to remove the need for any anchors.

@fnagel
fnagel commented on f416a66 Sep 25, 2013

Ok, just wanted to make sure its not forgotton.

Mhh, but I fix this for the moment, right?

@fnagel
fnagel commented on f416a66 Sep 25, 2013

I've merged this PR to selectmenu branch and fixed the follow anchor on item click issue.

Please sign in to comment.
Something went wrong with that request. Please try again.