Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Selectmenu: Code review #492

Closed
wants to merge 292 commits into from

9 participants

@fnagel
Collaborator

Request for code review (in accordance with Jörn).

There some open issues being discussed, see the Wiki:
http://wiki.jqueryui.com/w/page/12138056/Selectmenu

@jzaefferer
Owner

I'd say its about time to start with unit tests. Most of the visual tests should be unit tests.

Collaborator

I agree. Only problem is to find some time to get into the test unit scripts :-/

ui/jquery.ui.selectmenu.js
((296 lines not shown))
+ if ( !this.opened ) this.list.menu( "select", event );
+ },
+
+ _getSelectedItem: function() {
+ return this.list.find( "li" ).not( '.ui-selectmenu-optgroup' ).eq( this.element[0].selectedIndex );
+ },
+
+ _toggle: function( event ) {
+ if ( this.opened ) {
+ this.close( event );
+ } else {
+ this.open( event );
+ }
+ },
+
+ _newelementEvents: {
@gnarf Owner
gnarf added a note

_newElementEvents?

@fnagel Collaborator
fnagel added a note

Named that way because the events are bound to this.newelement. Variable names (this.list, this.newelement, etc.) are taken over from the old branch. The naming of the vars should be discussed anyway :-)

@fnagel Collaborator
fnagel added a note

See here: #492 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gnarf
Owner

I'm seeing a lot of uses of var that = this; where its not event needed. We are trying to avoid this wherever possible now...

Should probably just replace with this in most cases, as we clean up most of these, we can look at a better workaround for any cases left over.

ui/jquery.ui.selectmenu.js
((43 lines not shown))
+ selectmenuId = that.element.attr( 'id' ) || 'ui-selectmenu-' + Math.random().toString( 16 ).slice( 2, 10 );
+
+ // quick array of button and menu id's
+ that.ids = [ selectmenuId, selectmenuId + '-button', selectmenuId + '-menu' ];
+
+ // set current value
+ if ( options.value ) {
+ that.element[0].value = options.value;
+ } else {
+ options.value = that.element[0].value;
+ }
+
+ // catch click event of the label
+ that._bind({
+ 'click': function( event ) {
+ that.newelement.focus();
@jzaefferer Owner

Can just use this within a _bind callback. Should remove need for that inside _create completely.

@jzaefferer Owner

Should also rename newelement to something like button or trigger. The latter would match what we have in Popup.

@fnagel Collaborator
fnagel added a note

1) Ahh I did get that wrong. Will be changed soon.
2) Agree, button seems legit. Should we change this.list to this.menu? I will change the wrapper var too.

@jzaefferer Owner

Sounds good.

@fnagel Collaborator
fnagel added a note

Done, pushed soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((66 lines not shown))
+ that._addList();
+ that.refresh();
+ },
+
+ _addNewelement: function() {
+ var that = this,
+ options = this.options,
+ tabindex = this.element.attr( 'tabindex' );
+
+ // hide original select tag
+ that.element.hide();
+
+ // create button
+ that.newelement = $( '<a />', {
+ href: '#' + that.ids[ 0 ],
+ tabindex: ( tabindex ? tabindex : that.element.attr( 'disabled' ) ? 1 : 0 ),
@jzaefferer Owner

tabindex 1 for disabled elements? Shouldn't that be -1?

@fnagel Collaborator
fnagel added a note

Correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((31 lines not shown))
+ // callbacks
+ open: null,
+ focus: null,
+ select: null,
+ close: null,
+ change: null
+ },
+
+ _create: function() {
+ var that = this,
+ options = this.options,
+ // set a default id value, generate a new random one if not set by developer
+ selectmenuId = that.element.attr( 'id' ) || 'ui-selectmenu-' + Math.random().toString( 16 ).slice( 2, 10 );
+
+ // quick array of button and menu id's
+ that.ids = [ selectmenuId, selectmenuId + '-button', selectmenuId + '-menu' ];
@jzaefferer Owner

That's certainly more convenient to define then an object literal, but ids[ x ] gets pretty annoying to look up.

@fnagel Collaborator
fnagel added a note

Should be changed to associative array?

@jzaefferer Owner

Yeah

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((55 lines not shown))
+ // catch click event of the label
+ that._bind({
+ 'click': function( event ) {
+ that.newelement.focus();
+ event.preventDefault();
+ }
+ });
+
+ that._addNewelement();
+ that._bind( that.newelement, that._newelementEvents );
+
+ that._addList();
+ that.refresh();
+ },
+
+ _addNewelement: function() {
@jzaefferer Owner

We're using _draw eleswhere for these kind of methods, maybe use that also as the prefix here?

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((119 lines not shown))
+ var setWidth = that.newelement.outerWidth();
+ } else {
+ var text = that.newelement.find( "span.ui-button-text");
+ var setWidth = text.width() + parseFloat( text.css( "padding-left" ) ) + parseFloat( text.css( "margin-left" ) );
+ }
+
+ // wrap list
+ that.listWrap = $( '<div />' )
+ .addClass( 'ui-selectmenu-menu' )
+ .width( setWidth )
+ .append( that.list )
+ .appendTo( options.appendTo );
+
+ // init menu widget
+ that.list
+ .data( 'element.selectelemenu', that.element )
@jzaefferer Owner

Can't you just use that.list whereever you need a reference to it?

@fnagel Collaborator
fnagel added a note

I currenlty need this refernce to close all other open menus. See first lines in method open (edit: you commented below).
Perhaps there is a better way to do this?

@fnagel Collaborator
fnagel added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((148 lines not shown))
+ event.preventDefault();
+ that.close( event, true);
+ }
+ },
+ focus: function( event, ui ) {
+ var item = ui.item.data( "item.selectmenu" );
+ if ( that.focus !== undefined && item.index != that.focus ) that._trigger( "focus", event, { item: item } );
+ that.focus = item.index;
+ }
+ });
+
+ // document click closes menu
+ that._bind( document, {
+ 'mousedown': function( event ) {
+ if ( that.opened && !$( event.target ).is( that.list ) ) {
+ window.setTimeout( function() {
@jzaefferer Owner

You can use _delay instead of setTimeout, it'll give you this back to the right context. And inside _bind this is already referring to the instance.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((139 lines not shown))
+
+ if ( item.index != that.element[0].selectedIndex ) flag = true;
+
+ that._setOption( "value", item.value );
+ that._trigger( "select", event, { item: item } );
+
+ if ( flag ) that._trigger( "change", event, { item: item } );
+
+ if ( that.opened ) {
+ event.preventDefault();
+ that.close( event, true);
+ }
+ },
+ focus: function( event, ui ) {
+ var item = ui.item.data( "item.selectmenu" );
+ if ( that.focus !== undefined && item.index != that.focus ) that._trigger( "focus", event, { item: item } );
@jzaefferer Owner

Needs line breaks and braces. Same for all other if-statements.

@fnagel Collaborator
fnagel added a note

No "shorthand" clauses at all?

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((162 lines not shown))
+ if ( that.opened && !$( event.target ).is( that.list ) ) {
+ window.setTimeout( function() {
+ that.close( event );
+ }, 200 );
+ }
+ }
+ });
+ },
+
+ refresh: function() {
+ var that = this,
+ options = this.options;
+
+ that.list.empty();
+
+ that._initSource();
@jzaefferer Owner

Should rename this - it was similar to autocomplete's, but it isn't anymore. _readItems or something.

@fnagel Collaborator
fnagel added a note

Renamed to _readOptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((157 lines not shown))
+ });
+
+ // document click closes menu
+ that._bind( document, {
+ 'mousedown': function( event ) {
+ if ( that.opened && !$( event.target ).is( that.list ) ) {
+ window.setTimeout( function() {
+ that.close( event );
+ }, 200 );
+ }
+ }
+ });
+ },
+
+ refresh: function() {
+ var that = this,
@jzaefferer Owner

Unneeded

@fnagel Collaborator
fnagel added a note

Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((191 lines not shown))
+ // transfer disabled state
+ if ( that.element.attr( 'disabled' ) ) {
+ that.disable();
+ } else {
+ that.enable()
+ }
+ },
+
+ open: function( event ) {
+ var that = this,
+ options = this.options,
+ currentItem = that._getSelectedItem();
+
+ if ( !options.disabled ) {
+ // close all other selectmenus
+ $( '.ui-selectmenu-open' ).not( that.newelement ).each( function() {
@jzaefferer Owner

Is there some other way to achieve the same effect? Click on the trigger should propagate to document, other selectmenu's (or popups or anything that's popping up) should handle the event.

@fnagel Collaborator
fnagel added a note

Ahhh I mentioned to find another way to do so when you asked above to remove the data binding. But you're right seems useless at all.

@fnagel Collaborator
fnagel added a note

Mhh its needed and I did not manage to handle the window event correct. Played arround with event.stopImmediatePropagation(), stopPropagation(), preventDefault() but with no success.

@jzaefferer Owner

Take a look at how Popup handles that, with a $(document).bind("click")

@fnagel Collaborator
fnagel added a note

When using click event on window only a click on the button should fire the window click event too and I cant get this working (see post above).

@fnagel Collaborator
fnagel added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((303 lines not shown))
+ _toggle: function( event ) {
+ if ( this.opened ) {
+ this.close( event );
+ } else {
+ this.open( event );
+ }
+ },
+
+ _newelementEvents: {
+ mousedown: function( event ) {
+ this._toggle( event );
+ event.stopImmediatePropagation();
+ },
+ click: function( event ) {
+ // return false needed to prevent browser from following the anchor
+ return false;
@jzaefferer Owner

Should be just event.preventDefault, otherwise it'll also stop propagation, see above regarding 'closing other selectmenus'.

@fnagel Collaborator
fnagel added a note

Changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((367 lines not shown))
+
+ if ( key === "appendTo" ) {
+ this.listWrap.appendTo( $( value || "body", this.element[0].ownerDocument )[0] );
+ }
+ if ( key === "value" && value !== undefined ) {
+ this.element[0].value = value;
+ this.newelement.children( '.ui-button-text' ).text( this.items[ this.element[0].selectedIndex ].label );
+ }
+ if ( key === "disabled" ) {
+ this.newelement.button( "option", "disabled", value );
+ if ( value ) {
+ this.element.attr( "disabled", "disabled" );
+ this.newelement.attr( "tabindex", -1 );
+ } else {
+ this.element.removeAttr( "disabled" );
+ this.newelement.attr( "tabindex", 1 );
@jzaefferer Owner

Should be tabindex 0, as in _addNewelement?

@fnagel Collaborator
fnagel added a note

Corrcected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
Owner

Nice: Showing 13 changed files with 1,000 additions and 0 deletions.

@fnagel
Collaborator

Ive updated the branch with a new version which gets rid of the unneeded data binding and has some other minor improvements.

Its tested on Win7 with FF7, IE9, latest Chrome, Safari 5.0.x, IE8, IE7 on VM and IE6 standalone. Looks good so far. IE6 has some icon positioning issues which should be reported to Menu (if IE6 is still supported).

@jzaefferer
Owner

Update looks pretty good, nice to see the document-click-handling is now working properly.

What's the menu issue? Could you put that on the Menu wiki page? http://wiki.jqueryui.com/w/page/12137997/Menu

@fnagel
Collaborator

Looks like the the IE6 icon problem I noticed was related to the standalone IE6, but I reported some other issues I noticed when rechecking with VM.

@fnagel
Collaborator

There are currently some issues with focus handling and click event control.
Please try to use the selectmenu default demo with overflow (to see all problems pretty easy) or try to click on an item but do not release the button -- the first items is focused before releasing.

Im running into that kind of trouble since Jörn merged master into selectmenu branch so I assume the problem is about the changes to Menu. ( Im talking about this commit form Hans: 3c258bf )
Afaics I need to make use of the pop-up widget in order to make Selectmenu work again and improve focus handling and accessibility. Any comments on this?

@fnagel
Collaborator

Problem seems to be related to line 83-85 in Menu.

"focus": function( event ) {
    this.focus( event, $( event.target ).children( ".ui-menu-item:first" ) );
},

It seems this will focus the first element every time before an item is clicked.

@jzaefferer
Owner

Looks like this is the actual commit: e16e99a

@jzaefferer
Owner

Also affects autocomplete. Need to make that an option, or find some other way for other widgets to disable it.

@jzaefferer
Owner

@kborchers will look into it.

@jzaefferer
Owner

Might be addressed here? e11efb3

@kborchers
Owner

OK, this should be corrected with 74a3f2c. It was the commit from Hans that caused this. I believe my changes still accomplish what he wanted and fix the issues that were caused.

@jzaefferer
Owner

Thanks @kborchers, looks good.

@fnagel
Collaborator

Agree, Selectmenu works nice again! Thanks, @kborchers

@fnagel
Collaborator

In my opinion the current implementation of isScrolling in Menu (mousover on item) causes some unwanted usability problems in Selectmenu when using overfow. After scrolling you need to mouse over at least one item before an item receives focus. It feels like an bug when using Selectmenu not like an feature.

@jzaefferer
Owner

Yeah, noticed that, too. @kborchers could you take a look?

@fnagel
Collaborator

Sorry, I did check the latest commit again and must confess the "first item selected" problem is not solved. @kborchers The focus event mentioned above still seems like a problem to me. Problem is that the focus currently stays on the button when selectmenu is opened and the click fires the Menu focus event with first item as parameter.

@kborchers
Owner

@fnagel Hmmm, I am not seeing the issue. If I look at http://view.jqueryui.com/selectmenu/tests/visual/selectmenu/events.html, it seems to record the correct element as the item being focused when the button is clicked. Please let me know if I am missing something or not understanding. I will be looking into isScrolling tonight also.

@kborchers
Owner

@jzaefferer Do you remember the reason for implementing isScrolling? I have removed it locally and Menu, Selectmenu and Autocomplete all seem to work just fine without it. The mouseover events of course now fire during scroll but is that a problem? Any input would be appreciated.

@jzaefferer
Owner

The commit doesn't really provide any more details then we have in the comment there anyway: 9a274c0

Not sure what "prevent mouseover from firing inadvertently when scrolling the menu" means - maybe we addressed that in some other way? Or we were wrong on considering that an issue? No idea, if you can't reproduce the original issue, let's kick out the isScrolling code and keep an idea on it.

@kborchers
Owner

OK, I have removed the isScrolling code. We'll keep an eye on it.

@fnagel
Collaborator

@kborchers
The isScrolling issue is fixed, thanks for that.

Regarding the "focus first item" problem:
Afaics the view.jquery.ui.com files are outdated. When using my local branch or download a fresh zip from GitHub the problem occurs.

Please test the default.hmtl demo. Open the second selectmenu (with overflow) by click and try to use the scrollbar.
When using the event.html demo you will notice another event after closing, which should not be fired.

@kborchers
Owner

@fnagel Yeah, I noticed the issue today when I clicked on the scroll bar. Working on a solution now.

@kborchers
Owner

@fnagel FYI, I made a change to Menu and renamed the "items" option to "menus". I looked through your code and it doesn't look like it affects you but just wanted you to be aware. I will merge master into this branch again now.

@fnagel
Collaborator

@kborchers Agree, seems legit. Thanks for pinging!

@fnagel
Collaborator

@kborchers
Any progress on the "focus first item" problem?

@kborchers
Owner

@fnagel Please take a look at these changes and let me know how selectmenu works as far as the focus issues are concerned. I think this should take care of it.

@kborchers
Owner

@fnagel OK, try this now. Sorry about that.

@fnagel
Collaborator

@kborchers Works nice again, no issues in major browsers afaics. Thanks for fixing!

@kborchers
Owner

Good to hear!

@fnagel
Collaborator

Im currently working on the remaining unit test issues, please see my last comments here: #536

@jzaefferer
Owner

That's an easy fix, nice. Wondering if you could check some other state that refresh initializes, instead of introducing another one.

Collaborator

I could check for an attribute buts that more expensive than an var.

Collaborator

Nahh, it wasn't that easy. A fully initialized menu widget is needed for keyboard control. I needed to add a check in _move, too.

@fnagel
Collaborator

@danwellman
Im still working on the unit tests. Ive noticed my (and I assume your) simulation of clicks did not work. Ive managed to make it work but its still pretty chaotic. I will push asap, please leave me a note before working on it again!

@danwellman

Yeah there was something not right if I remember correctly. Probably won't get a chance to work on this for the next week or so, but I'll let you know if I do

@fnagel
Collaborator

@jzaefferer
Unit tests should be ready for a review now.

@kborchers
Owner

This is just a very basic review running the tests. I have not looked into the cause of any of the failures.

IE6 - Tests 2, 3, 8 and 12 have failures
IE7 - Same as IE6
IE8 - Same as IE7
IE9 - All pass

FF8 - All Pass
FF9 - All Pass

Chrome 15 - All Pass
Chrome 16 - All Pass

Safari 5.1 - All Pass

Opera 11.5 - All Pass
Opera 11.6 - All Pass

So it looks like you just have an issue with IE older than 9.

@jzaefferer
Owner

There are still five visual tests. We should try and reduce that further, either by porting them to unit tests, or adding another demo.

@jzaefferer
Owner

Is that useful as a unit test? Probably more interesting would be a setup where we could test various components, and it'll add console.profile() and console.profileEnd() calls around the code under test.

Collaborator

Mhh no, not really. I used to test performance. Shall I remove it?
Using profile sound nice to me!

@fnagel
Collaborator

@jzaefferer
Please check this commit as Im not sure if I did understand your note (ToDos Wiki) correct.

_getCreateOptions should return a hash of options. See what spinner does as a reference: https://github.com/jquery/jquery-ui/blob/master/ui/jquery.ui.spinner.js#L70

Collaborator

Should be fixed in this commit: 98d72c7

@fnagel
Collaborator

Subject: visual tests
We could merge themeswitcher and compatibility but I would like to keep all other files.

@fnagel
Collaborator

In order to fulfill the new ARIA specs I needed to remove lazy (menu widget) refresh (on first usage) because otherwise I have no ID attributes to use with aria-activedescendant.

@fnagel

item is undefined in "common widget: basic usage" unit test. I have no idea why. Help please!

One of the common widget unit tests is that all widgets should be able to be initialized based on their default element and using default options (no options passed to init). This failure minimizes down to

$( "<select>" ).appendTo( "body" ).selectmenu()

notice it's because the select (the default element for selectmenu) has no options. So this would pass

$( "<select><option></select>" ).appendTo( "body" ).selectmenu()

but the former needs to pass as well.

Collaborator

So its about how to handle an empty select tag. Which leads to the question how to handle empty options or even empty optgroups.
I fixed this problem and in general handling of empty tags. Empty oprgroups wont be displayed atm.

Thanks for helping out!

demos/selectmenu/custom_render.html
((24 lines not shown))
+ el = item.element;
+ var link = $( "<a />", {
+ html: icon = '<span style="' + el.attr("style") + '" class="ui-icon ' + el.attr("class") + '"></span>' + item.label,
+ href: '#'
+ });
+ li.append( link );
+ }
+
+ return li.appendTo( ul );
+ }
+ });
+
+ var files = $('select#files').iconselectmenu({
+ dropdown: false
+ });
+ files.iconselectmenu("widget").children("ul").addClass("ui-menu-icons");
@jzaefferer Owner

I wonder if we can abstract this a little. Having to know that the menu element is a child of the widget element is a bit too much coupling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((369 lines not shown))
+ },
+
+ _select: function( item, event ) {
+ var oldIndex = this.element[0].selectedIndex;
+ // change native select element
+ this.element[0].selectedIndex = item.index;
+ this._setSelected( item );
+ this._trigger( "select", event, { item: item } );
+
+ if ( item.index != oldIndex ) {
+ this._trigger( "change", event, { item: item } );
+ }
+ },
+
+ _setSelected: function( item ) {
+ var link = item.element.find("a");
@jzaefferer Owner

item.element is an element, therefore this returns an empty jQuery object, causing the attr call below to do nothing. Causing the unit test for aria-selected to fail.

@fnagel Collaborator
fnagel added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/unit/selectmenu/selectmenu_core.js
((6 lines not shown))
+ var element = $('#speed').selectmenu(),
+ widget = element.selectmenu("widget"),
+ button = widget.filter(".ui-selectmenu-button"),
+ menu = widget.filter(".ui-selectmenu-menu"),
+ link = button.find("a"),
+ selected = element.find("option:selected"),
+ ul = menu.children("ul"),
+ links = ul.find("li.ui-menu-item a");
+
+ expect(13 + links.length * 2);
+
+ equals( link.attr("role"), "combobox", "button link role" );
+ equals( link.attr("aria-haspopup"), "true", "button link aria-haspopup" );
+ equals( link.attr("aria-expanded"), "false", "button link aria-expanded" );
+ equals( link.attr("aria-autocomplete"), "list", "button link aria-autocomplete" );
+ equals( link.attr("aria-activedescendant"), links.eq(element[0].selectedIndex).attr("id"), "button link aria-activedescendant" );
@jzaefferer Owner

Currently fails, should probably removed to match the new ARIA spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/unit/selectmenu/selectmenu_core.js
((46 lines not shown))
+ }
+], function( i, settings ) {
+ test("state synchronization - " + settings.type, function () {
+ expect(10);
+
+ var element = $(settings.selector).selectmenu(),
+ widget = element.selectmenu("widget"),
+ button = widget.filter(".ui-selectmenu-button"),
+ menu = widget.filter(".ui-selectmenu-menu"),
+ link = button.find("a"),
+ ul = menu.children("ul"),
+ links = ul.find("li.ui-menu-item a"),
+ selected = element.find("option:selected");
+
+ link.simulate( "keydown", { keyCode: $.ui.keyCode.DOWN } );
+ equals( ul.attr("aria-activedescendant"), links.eq(element[0].selectedIndex).attr("id"), "after keydown menu aria-activedescendant" );
@jzaefferer Owner

Same problem as above. Not sure if it should be just dropped or replace with another check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
Owner

Hitting Escape key should close an open selectmenu, to match behaviour of native selects.

@jzaefferer
Owner

Cursor left/right behave the same as up/down, not the case with native selects.

@fnagel
Collaborator

Native select keyboard control: Cursor left/right work the same way as up/down in FF and Chrome (when menu is closed), at least on Windows.

@jzaefferer
Owner

Is the button css file still required?

@fnagel
Collaborator
@jzaefferer
Owner

If the button CSS is not required, then its all cool. Class names look good.

@fnagel
Collaborator
@fnagel
Collaborator

Ive fixed a couple of bugs and was able to improve selectmenu a lot. Ive tested heavily in FF11, IE7-9, latest Chrome stable and Safari5. All on Win7, except IE7 and IE8 (in VM). Worked pretty nice so far.

Small problem in IE with ui-state-disabled CSS class. This is a known problem which was fixed quick and dirty in the old version of selectmenu.

Unit test passed all browsers but failed in Safari :-/

@jzaefferer
Owner

Looks like Safari doesn't like simulate's defineProperty hack. Hoping for @rdworth or @scottgonzalez to take a look, they know a lot more about simulate.

What was the ui-state-disabled workaround?

@jzaefferer
Owner

Scott said: "I'll keep looking into the simulate bug, but as a temporary workaround, you can do .simulate( event, { clientX: 1, clientY: 1 } )"

@fnagel
Collaborator

We added a CSS hack with "color: silver;". Im not sure anymore but I think we did not manage to find a working solution for opacity and text in IE. I assume this should be a problem in other Widgets too.

@jzaefferer
Owner

Can you give a bit more context on the ui-state-disabled problem? I'm not sure what to look for.

@fnagel
Collaborator

Try http://view.jqueryui.com/selectmenu/tests/visual/selectmenu/disabled.html in IE8 or less
Disabled items are fully visible. Opacity seems to have no effect.

@scottgonzalez

I did some cleanup of the simulate plugin. I put in a slightly better workaround for now (one directly in simulate) and reverted the commit for the changes in the menu tests. I'd like to eventually get a better solution directly in simulate that defaults to choosing coordinates based on the position of the event target.

@jzaefferer
Owner

This looks fine to me, IE8/Win7: http://bassistance.de/i/d59d48.png

@fnagel
Collaborator
@jzaefferer
Owner

Still haven't gotten around to looking into this. @kborchers - could you help out here? About the bad rendering of ui-state-disabled in IE6 and 7.

@kborchers
Owner

Yep, I will take a look this evening and let you know what I find.

@kborchers
Owner

The ui-state-disabled issue was being caused by the menu items not having an anchor tag. Without the anchor, the ui-menu-item class was not added which is needed in IE6/7 for it's zoom: 1; that it adds to the element (among other things, obviously). Without that zoom, or some other way of giving a "position" to those items, opacity won't work there in IE6/7.

@jzaefferer
Owner

Thanks Kris! So @fnagel, what's left now?

@fnagel
Collaborator
@fnagel
Collaborator

The opacity issue works pretty nice now. Thanks for fixing that kborchers!
I needed to adjust some unit tests in order to make the latest menu changes (for disabled state) work.

@jzaefferer
Afaik there are no more steps to do except a11y testing. Next steps?

fnagel added some commits
@fnagel fnagel Merge branch 'master' into selectmenu 41dfb09
@fnagel fnagel Merge branch 'master' into selectmenu c6a8d7e
@fnagel fnagel Selectmenu: Upgrade jQuery to 1.7.2. 31a38e9
@fnagel fnagel Menu: Check that the event object is defined before checking type, in…
… conformance to revision e2a6cdd and in order to make Selectmenu work again
8fbdd7c
@fnagel fnagel Selectmenu: make optgroups work again, needed because of Menu widget …
…changes (ui-menu-divider)
eae21ff
@fnagel fnagel Selectmenu: trim trailing spaces b348d32
@fnagel fnagel Selectmenu: prevent copyright notice from being removed by minification ddf7c27
@fnagel fnagel Selectmenu: updated copyright year 360e885
@fnagel fnagel Selectmenu: Upgrade tests to jQuery 1.7.2 4460376
@fnagel fnagel Selectmenu: Update unit tests ff957dd
@fnagel fnagel Selectmenu: small fixes for lint tests c5a449d
@fnagel fnagel Selectmenu: trim trailing spaces c3f6bd9
@fnagel fnagel Selectmenu: small fixes for lint tests ec6d88f
@fnagel fnagel Merge with master b30184d
@fnagel fnagel Selectmenu: make use of Menu option 'role' 7345e43
@fnagel fnagel Selectmenu: small fixes for lint tests 37a8047
@fnagel fnagel Selectmenu: make use of Menu option 'role' (follow-up), no need to ad…
…just li role
d4ec5cb
@fnagel fnagel Merge remote-tracking branch 'origin/master' into selectmenu 8abe1d0
@fnagel fnagel Merge branch 'master' into selectmenu 749c8fb
@fnagel fnagel Selectmenu: Use new uniqueId method for generating id's 39532f0
@fnagel fnagel Merge branch 'master' into selectmenu 5092d02
@fnagel fnagel Selectmenu: changed _bind to _on 8f5f4b3
@fnagel fnagel Selectmenu: changed _unbind to _off b19986a
@fnagel fnagel Selectmenu: trim trailing spaces 001bd3d
@fnagel fnagel Selectmenu: added complex menu demo to compatibility visual test c7e9906
@fnagel
Collaborator

Merged with master branch. Some unit tests fail. Seems menu related.

@kborchers
Owner

I am not seeing any menu related unit test failures in the selectmenu branch. Can you double check and let me know what browser and what failures your are seeing?

@fnagel
Collaborator

Thanks for answering :-)

In latest FF when checking the selectmenu.html unit test I get 5 errors: http://www.pic-upload.de/view-15100661/selectmenu_unittest.png.html

This errors occur since the latest merge with the master. Any idea?

@kborchers
Owner
@kborchers
Owner
@fnagel
Collaborator

Im not sure how this change should break Selectmenus unit test. Whats your idea?
I look into a few things but I was not able to track this one down.

Facts:

  • Only fails in "click on item" test, not in the "arrow down on button" which uses Menu's next method
  • Unit test does the following: focus button, click button, mouseover and click last element, TESTS
  • The attributes mentioned in the unit test seem to be correct when checked manually
  • activedescendant is only set in Menu widget
  • aria-selected is only set in Selectmenu widget

Please note: Ive updated the unit test to separate the problem (so its possible to use the rerun link).

I noticed another unwanted behavior: the style of a selected element is removed after a few moments.

@scottgonzalez

The tests are now passing. The events weren't being unbound using _off(). See 549b97e.

@fnagel
Collaborator

Thanks @scottgonzalez!

I think I need to study how the new on and off event binding works exactly.

@scottgonzalez

@fnagel If you have any questions, feel free to ping me in IRC, but the short answer of why this broke is that every instance now gets a unique namespace for event bindings.

@fnagel
Collaborator

@scottgonzalez

I have a question regarding your "Added test for widget() method." commmits.
Selectmenu is a little special regarding the widget method as we replace the original element. I've discussed this with @jzaefferer and we decided to return the created a tag when calling widget() and implemented a function called menuWidget to return the created ul (Menu widget). Is this still valid? If so, how should the unit test look like?

@fnagel
Collaborator

Thanks, that was my first thought too.

@joshuahiggins

I have a question and a bug and they are sort of related... Is the menu supposed to display on tab focus in (and if not by default, is this a setting that can be set)?

Now for the bug... On first tab focus in, ui-button is setting and unsetting the ui-state-focus class instantaneously. If you tab focus out and back in, ui-state-focus will remain set. So, on first tab in, if you're relying on ui-state-focus classes to visually queue your audience, you will be at a loss.

@jzaefferer
Owner

@fnagel I've got plenty time this week to review and discuss things. Would be great to get it done really soon. We should consider doing a new PR, this has quite a long and not-so-interesting history.

@fnagel
Collaborator

@joshuahiggins
No, open on tab focus will not be possible, but it should be possible by using the API.

Mhh, Im not able to reproduce this issue. Steps to reproduce would be: tab to select, a tag has class ui-state-focus, tab out, a tag has no class ui-state-focus. Which browser do you use?

@fnagel fnagel referenced this pull request in fnagel/jquery-ui
Closed

Official jQuery UI Selectmenu Widget #140

@joshuahiggins

@fnagel I'm able to recreate it in Chrome 22 and Firefox 15 and on all instantiations of selectmenu (including your demo page).

Open the demo page and don't interact with the selectmenu. Use the tab key to focus on the selectmenu. On first focus, ui-state-focus is being set and unset immediately. To the user, it appears that selectmenu does not have focus, as the styling is not present. If you tab out and tab back in to the selectmenu, the styling will be present. The bug only happens on the first focus of each selectmenu.

If you watch it in your browser's developer tools, you'll see the blip of ui-state-focus only lasts for a fraction of a second.

@fnagel
Collaborator

Ok, I'm now aware of the problem. I was missing "only on first focus" :-)

I've pushed a fix, please test and give feedback!

@joshuahiggins

That actually didn't solve the issue. It's still dropping focus on first tab in.

@fnagel
Collaborator

Sorry, this commit should really fix this problem. Class is remove by _setOption (in ui.widget) which is called in refresh method (setting the disabled state).

@joshuahiggins

Works great! Thanks.

Any chance you'll be bringing back the ability to set custom formatting, such as for multiple line selects?

Also, I have another bug. After focus is lost from Selectmenu, Menu calls blur which waits 300 milliseconds and removes ui-state-focus from the selected item. I understand why Menu calls this, as this behavior makes sense for the type of menu that Menu was built for. This doesn't make sense for a dropdown menu like Selectmenu.

After clicking and selecting an item from Selectmenu the menu closes but remains focused. From there, 1 of 2 scenarios will happen:

1) If you click back into Selectmenu without losing focus, you will see your selected item has the ui-state-focus state for 300 milliseconds before blur removes that class. This is buggy behavior.

2) If you dropped focus from Selectmenu (clicking outside of the menu) before focusing back in, your selected item does not have the ui-state-focus class at all. This feels less buggy, but doesn't feel like the appropriate behavior for a dropdown.

So I guess a discussion could be made about how Selectmenu should behave. Should the selected item already be highlighted when the menu is opened, or should it wait until a hover interaction from the keyboard / mouse?

Either way, the 300 milliseconds of highlighting from scenario one is a bug for Selectmenu.

@fnagel
Collaborator

@joshuahiggins

Custom formatting is possible by using a custom rendering. See custom_render.html demo for example. Multiple line should be no problem.

I guess it should have focus to the currently selected item when opened. This is the native behaviour of the select in FF, too. Should be fixed.

The timeout issue is a little tricky. Perhaps @jzaefferer could join us here.
It could be fixed by changing this.delay in Menu widget to a high number (as josh already proposed). Its not a clean solution but it should work. Please see latest commit.

@jzaefferer
Owner

Let's clean up the visual tests. They are useful while prototyping, but need to be replaced by unit tests. Units are all there, so let's remove most of the visual tests, maybe merge one from each into a composite one.

ui/jquery.ui.selectmenu.js
@@ -0,0 +1,452 @@
+/*!
+ * jQuery UI Selectmenu @VERSION
+ * http://jqueryui.com
+ *
+ * Copyright 2012 jQuery Foundation and other contributors
+ * Released under the MIT license.
+ * http://jquery.org/license
+ *
+ * http://docs.jquery.com/UI/Selectmenu
@jzaefferer Owner

Should be http://api.jqueryui.com/selectmenu

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((8 lines not shown))
+ *
+ * http://docs.jquery.com/UI/Selectmenu
+ *
+ * Depends:
+ * jquery.ui.core.js
+ * jquery.ui.widget.js
+ * jquery.ui.position.js
+ * jquery.ui.menu.js
+ */
+(function( $, undefined ) {
+
+$.widget( "ui.selectmenu", {
+ version: "@VERSION",
+ defaultElement: "<select>",
+ options: {
+ dropdown: true,
@jzaefferer Owner

Options should be alphabetically ordered, then a line break, then callbacks, also alphabetically ordered.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((24 lines not shown))
+ appendTo: "body",
+ position: {
+ my: "left top",
+ at: "left bottom",
+ collision: "none"
+ },
+ // callbacks
+ open: null,
+ focus: null,
+ select: null,
+ close: null,
+ change: null
+ },
+
+ _create: function() {
+ // make / set unique id
@jzaefferer Owner

Unnecessary comment, the purpose of uniqueId() should be clear.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((27 lines not shown))
+ at: "left bottom",
+ collision: "none"
+ },
+ // callbacks
+ open: null,
+ focus: null,
+ select: null,
+ close: null,
+ change: null
+ },
+
+ _create: function() {
+ // make / set unique id
+ var selectmenuId = this.element.uniqueId().attr( "id" );
+
+ // array of button and menu id's
@jzaefferer Owner

Dunno if this comment is necessary, but at least its wrong, as this isn't an array anymore.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((31 lines not shown))
+ open: null,
+ focus: null,
+ select: null,
+ close: null,
+ change: null
+ },
+
+ _create: function() {
+ // make / set unique id
+ var selectmenuId = this.element.uniqueId().attr( "id" );
+
+ // array of button and menu id's
+ this.ids = { id: selectmenuId, button: selectmenuId + "-button", menu: selectmenuId + "-menu" };
+
+ this._drawButton();
+ this._on( this.button, this._buttonEvents );
@jzaefferer Owner

Move this into _drawButton.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((40 lines not shown))
+ var selectmenuId = this.element.uniqueId().attr( "id" );
+
+ // array of button and menu id's
+ this.ids = { id: selectmenuId, button: selectmenuId + "-button", menu: selectmenuId + "-menu" };
+
+ this._drawButton();
+ this._on( this.button, this._buttonEvents );
+ this._hoverable( this.button );
+ this._focusable( this.button );
+
+ this._drawMenu();
+
+ // document click closes menu
+ this._on( document, {
+ click: function( event ) {
+ if ( this.isOpen && !$( event.target ).closest( "li.ui-state-disabled, li.ui-selectmenu-optgroup, #" + this.ids.button ).length ) {
@jzaefferer Owner

Does this still run when the selectmenu gets disabled? If it should, pass the supressDisabledCheck flag (see ui.widget, was added recently).

Either way, let's move this handler into a documentClick method or something like that. Should remove the need for the comment above.

@fnagel Collaborator
fnagel added a note

No, this does not run when a selectmenu is disabled as the selectmenu is closed on disabling. So do we need suppressDisabledCheck here?

I've added a _documentClick method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((56 lines not shown))
+ this.close( event );
+ }
+ }
+ });
+
+ if ( this.options.disabled ) {
+ this.disable();
+ }
+ },
+
+ _drawButton: function() {
+ var tabindex = this.element.attr( "tabindex" );
+
+ // fix existing label
+ this.label = $( "label[for='" + this.ids.id + "']" ).attr( "for", this.ids.button );
+ // catch click event of the label
@jzaefferer Owner

Unnecessary.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on the diff
ui/jquery.ui.selectmenu.js
((54 lines not shown))
+ click: function( event ) {
+ if ( this.isOpen && !$( event.target ).closest( "li.ui-state-disabled, li.ui-selectmenu-optgroup, #" + this.ids.button ).length ) {
+ this.close( event );
+ }
+ }
+ });
+
+ if ( this.options.disabled ) {
+ this.disable();
+ }
+ },
+
+ _drawButton: function() {
+ var tabindex = this.element.attr( "tabindex" );
+
+ // fix existing label
@jzaefferer Owner

Why do you need both this and a click handler? Should the updated for-attribute take care of the click-forwarding already?

@fnagel Collaborator
fnagel added a note

This was added by Robert. I'm not quite sure why this was added.

@jzaefferer Owner

So Robert added the for-attribute-update, the click handler was already there?

If so: I think he added it to fix screenreaders reading the button label, I guess the for-attribute is more reliable then aria-labelledby. With that in place, the manual click binding can likely be removed. Can you give that a try?

@fnagel Collaborator
fnagel added a note

You did something on that in your branch. Any need to test this for me any further?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((68 lines not shown))
+
+ // fix existing label
+ this.label = $( "label[for='" + this.ids.id + "']" ).attr( "for", this.ids.button );
+ // catch click event of the label
+ this._on( this.label, {
+ "click": function( event ) {
+ this.button.focus();
+ event.preventDefault();
+ }
+ });
+
+ // hide original select tag
+ this.element.hide();
+
+ // create button
+ this.button = $( "<a />", {
@jzaefferer Owner

Why are we using an anchor here? Can it be a <button> or even just a <span>?

@fnagel Collaborator
fnagel added a note

Good question. It was a anchor since the filaments group first selectmenu script.

I assume changing this will lead to different behavior in screenreaders.

@jzaefferer Owner

We used to require anchors in other places as well, I think old Safaris sucked at handling tabindex. That's why Accordion doesn't require anchors anymore, that was one of the big changes in 1.9. We should try to replace the anchor and see if that helps improving screenreader compat anywhere.

@fnagel Collaborator
fnagel added a note

I can do that. Should I change it to a button element?

@fnagel Collaborator
fnagel added a note

Sorry, I've just noticed you changed this in the aria-tweaks branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((82 lines not shown))
+ // create button
+ this.button = $( "<a />", {
+ "class": "ui-button ui-widget ui-state-default ui-corner-all",
+ href: "#" + this.ids.id,
+ tabindex: ( tabindex ? tabindex : this.options.disabled ? -1 : 0 ),
+ id: this.ids.button,
+ width: this.element.outerWidth(),
+ role: "combobox",
+ "aria-expanded": false,
+ "aria-autocomplete": "list",
+ "aria-owns": this.ids.menu,
+ "aria-haspopup": true
+ });
+
+ this.button.prepend( $( "<span />", {
+ "class": "ui-icon " + ( ( this.options.dropdown ) ? "ui-icon-triangle-1-s" : "ui-icon-triangle-2-n-s" )
@jzaefferer Owner

Unnecessary parens around the option.

@jzaefferer Owner

Also, we should make this configurable as an icons option, similar to accordion's. Let's wait for @scottgonzalez on that, though.

@fnagel Collaborator
fnagel added a note

We did discuss this earlier. The span is needed for CSS scope if a custom style should be added to this specific selectmenu.

@jzaefferer Owner

Wrong line? I didn't question the span, as far as I can tell. Was suggesting something like this:

"class": "ui-icon " + ( this.options.dropdown ? options.icons.dropdown : options.icons.popup )

Without the icons option, you should still remove the parens around this.options.dropdown

@fnagel Collaborator
fnagel added a note

Sorry, I misunderstood.

Seems a nice idea. I will wait for @scottgonzalez response.

@fnagel Collaborator
fnagel added a note

Implemented in 52fc8e3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((81 lines not shown))
+
+ // create button
+ this.button = $( "<a />", {
+ "class": "ui-button ui-widget ui-state-default ui-corner-all",
+ href: "#" + this.ids.id,
+ tabindex: ( tabindex ? tabindex : this.options.disabled ? -1 : 0 ),
+ id: this.ids.button,
+ width: this.element.outerWidth(),
+ role: "combobox",
+ "aria-expanded": false,
+ "aria-autocomplete": "list",
+ "aria-owns": this.ids.menu,
+ "aria-haspopup": true
+ });
+
+ this.button.prepend( $( "<span />", {
@jzaefferer Owner

Use <span></span> for non-empty elements.

@jzaefferer Owner

There's four more of this below.

@scottgonzalez Owner

Actually just <span> if all you're doing is creating the element.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((87 lines not shown))
+ id: this.ids.button,
+ width: this.element.outerWidth(),
+ role: "combobox",
+ "aria-expanded": false,
+ "aria-autocomplete": "list",
+ "aria-owns": this.ids.menu,
+ "aria-haspopup": true
+ });
+
+ this.button.prepend( $( "<span />", {
+ "class": "ui-icon " + ( ( this.options.dropdown ) ? "ui-icon-triangle-1-s" : "ui-icon-triangle-2-n-s" )
+ }));
+
+ this.buttonText = $( "<span />", {
+ "class": "ui-selectmenu-text" ,
+ html: this.element.find( "option:selected" ).text() || "&nbsp;"
@jzaefferer Owner

In dialog we use &#160;. The result should be same as &nbsp;, not sure about the reason for one or the other.

@scottgonzalez Owner

I don't remember the exact reason we used &#160; but I seem to remember there being a reason. I can say that it's more portable than &nbsp; as numeric values will work without additional definitions for named entities.

@fnagel Collaborator
fnagel added a note

Seems to work without problems. Changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on the diff
ui/jquery.ui.selectmenu.js
((101 lines not shown))
+ "class": "ui-selectmenu-text" ,
+ html: this.element.find( "option:selected" ).text() || "&nbsp;"
+ })
+ .appendTo( this.button );
+
+ // wrap and insert new button
+ this.buttonWrap = $( "<span />", {
+ "class": "ui-selectmenu-button"
+ })
+ .append( this.button )
+ .insertAfter( this.element );
+ },
+
+ _drawMenu: function() {
+ var menuInstance,
+ that = this;
@jzaefferer Owner

@scottgonzalez that is needed here to use regular menu callbacks. Any idea how to replace it here?

@scottgonzalez Owner

It's fine to use that in these situations, but if you really want to get rid of it, you would do so by using _on() to bind to the events instead of using callbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on the diff
ui/jquery.ui.selectmenu.js
((157 lines not shown))
+
+ // Set ARIA active decendent
+ that.button.attr( "aria-activedescendant", that.menuItems.eq( item.index ).find( "a" ).attr( "id" ) );
+ },
+ // set ARIA role
+ role: "listbox"
+ })
+ .data( "ui-menu" );
+
+ // change menu styles?
+ if ( this.options.dropdown ) {
+ this.menu.addClass( "ui-corner-bottom" ).removeClass( "ui-corner-all" );
+ }
+
+ // make sure focus stays on selected item
+ menuInstance.delay = 999999999;
@jzaefferer Owner

This looks like it should get addressed in menu itself. What issue is this working around, is there a ticket? Ping @kborchers, too.

@fnagel Collaborator
fnagel added a note

No, there is no ticket, but a a comment in this CR, see:

I agree, this should be handled in Menu.

@jzaefferer Owner

There is no link in your comment. Could you create a ticket and link to whatever is available? http://bugs.jqueryui.com/newticket

@fnagel Collaborator
fnagel added a note

Ahh sorry: #492 (comment) and following

@fnagel Collaborator
fnagel added a note

Mhh, wanted to add a ticket but some wild python tracelog appeared ;-)

@fnagel Collaborator
fnagel added a note

The login system seems to have serious issues. I've needed to reset my password (again) and login ended up in a redirect loop.

Finally: http://bugs.jqueryui.com/ticket/8929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((133 lines not shown))
+ // init menu widget
+ menuInstance = this.menu.menu({
+ select: function( event, ui ) {
+ var item = ui.item.data( "ui-selectmenu-item" );
+
+ that._select( item, event );
+
+ if ( that.isOpen ) {
+ event.preventDefault();
+ that.close( event );
+ }
+ },
+ focus: function( event, ui ) {
+ var item = ui.item.data( "ui-selectmenu-item" );
+
+ if ( that.focus !== undefined ) {
@jzaefferer Owner

Put these two if statements into one. if ( that.focus !== undefined && item.index !== that.focus ) {.

Also here a comment explaining the purpose would be nice.

@scottgonzalez Owner

We very rarely check !== undefined, can't this just be != null?

@jzaefferer Owner

Still need to merge the two if statements. If it works, also replace !== undefined with != null.

@fnagel Collaborator
fnagel added a note

I've merged these statements.

Check for undefined is needed as we need to call the Menu widget focus method on init. So test for undefined prevents focus from firing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((151 lines not shown))
+ if ( !that.isOpen ) {
+ that._select( item, event );
+ }
+ }
+ }
+ that.focus = item.index;
+
+ // Set ARIA active decendent
+ that.button.attr( "aria-activedescendant", that.menuItems.eq( item.index ).find( "a" ).attr( "id" ) );
+ },
+ // set ARIA role
+ role: "listbox"
+ })
+ .data( "ui-menu" );
+
+ // change menu styles?
@jzaefferer Owner

Weird comment. Should inform about WHY styles need to be changed.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((192 lines not shown))
+ this._setSelected( item.data( "ui-selectmenu-item" ) );
+
+ // set disabled state
+ this._setOption( "disabled", this._getCreateOptions().disabled );
+ }
+ },
+
+ open: function( event ) {
+ if ( !this.options.disabled ) {
+ var _position = {
+ of: this.button
+ };
+
+ // make sure menu is refreshed on first init (needed at least for IE9)
+ if ( this.isOpen === undefined ) {
+ this.button.trigger( "focus" );
@jzaefferer Owner

So you trigger the focus handler to refresh the menu, and it'll also remove the focus handler, right? Can we remove a level of indirection here?

@fnagel Collaborator
fnagel added a note

Any idea how?

@jzaefferer Owner

Nope, would've wrote it down otherwise. Let's try to come up with something.

@fnagel Collaborator
fnagel added a note

I will do some tests and see whats possible.

@fnagel Collaborator
fnagel added a note

After some tests I removed this snippet. It was needed once, but it seems bo be useless now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((185 lines not shown))
+
+ this.menu.menu( "refresh" );
+ this.menuItems = this.menu.find( "li" ).not( ".ui-selectmenu-optgroup" );
+
+ // select current item
+ item = this._getSelectedItem();
+ this.menu.menu( "focus", null, item );
+ this._setSelected( item.data( "ui-selectmenu-item" ) );
+
+ // set disabled state
+ this._setOption( "disabled", this._getCreateOptions().disabled );
+ }
+ },
+
+ open: function( event ) {
+ if ( !this.options.disabled ) {
@jzaefferer Owner

Use a guard clause:

if ( this.options.disabled ) {
    return;
}
...
@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((199 lines not shown))
+ open: function( event ) {
+ if ( !this.options.disabled ) {
+ var _position = {
+ of: this.button
+ };
+
+ // make sure menu is refreshed on first init (needed at least for IE9)
+ if ( this.isOpen === undefined ) {
+ this.button.trigger( "focus" );
+ }
+
+ this.isOpen = true;
+ this._toggleAttr();
+ this.menu.menu( "focus", event, this._getSelectedItem() );
+
+ if ( this.items && !this.options.dropdown && this.options.position.my == "left top" && this.options.position.at == "left bottom" ) {
@jzaefferer Owner

This deserves a comment. Why do you need the position checks?

@fnagel Collaborator
fnagel added a note

Added a comment.

Thats the check I talked about in my email.
With this check its possible to have custom positioned menus in popup style. Otherwise the
Only other solution would be to separate pop-up styling and positioning in two different options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((212 lines not shown))
+ this.menu.menu( "focus", event, this._getSelectedItem() );
+
+ if ( this.items && !this.options.dropdown && this.options.position.my == "left top" && this.options.position.at == "left bottom" ) {
+ var currentItem = this._getSelectedItem();
+ // center current item
+ if ( this.menu.outerHeight() < this.menu.prop( "scrollHeight" ) ) {
+ this.menuWrap.css( "left" , -10000 );
+ this.menu.scrollTop( this.menu.scrollTop() + currentItem.position().top - this.menu.outerHeight() / 2 + currentItem.outerHeight() / 2 );
+ this.menuWrap.css( "left" , "auto" );
+ }
+ _position.my = "left top" + ( this.menu.offset().top - currentItem.offset().top + ( this.button.outerHeight() - currentItem.outerHeight() ) / 2 );
+ _position.at = "left top";
+ }
+
+ this.menuWrap
+ .zIndex( this.element.zIndex() + 1 )
@jzaefferer Owner

Since the default for the appendTo option, which is used append the menuWrap element, is "body", we shouldn't have to set a z-index. If the selectmenu is used inside a dialog, it needs to be appended to the dialog, and the z-index on the dialog takes care of stacking. Is that correct @scottgonzalez?

@scottgonzalez Owner

Yeah, we need to follow the .ui-front logic here and not set z-index directly.

@fnagel Collaborator
fnagel added a note

Using

.addClass( "ui-front" )

instead of

.zIndex( this.element.zIndex() + 1 )

as Jörn said does not work when Selectmenu is inside a Dialog (see compatibility visual test). Do I need to adjust some CSS or something?

@jzaefferer Owner

Did you set appendTo: dialog? That should be all that's needed.

@scottgonzalez Owner

No, you shouldn't set appendTo at all. Dialog should automatically be found because of .ui-front.

@jzaefferer Owner

This still needs to be addressed. See "z-index and stacking" for details on using ui-front here: http://wiki.jqueryui.com/w/page/12137737/Coding%20standards

@fnagel Collaborator
fnagel added a note

What dialog?

Sorry but I still struggling to make this work. Any code example? I see that autocomplete and dialog use a _appendTo function but without making use of a ui-front class.

@fnagel Collaborator
fnagel added a note

Please see 57ecee8

@jzaefferer Owner

That commit changes the appendTo logic, but the z-index is still set. Is it still necessary?

@fnagel Collaborator
fnagel added a note

Nah, its not. Removed.

@fnagel Collaborator
fnagel added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((220 lines not shown))
+ this.menuWrap.css( "left" , "auto" );
+ }
+ _position.my = "left top" + ( this.menu.offset().top - currentItem.offset().top + ( this.button.outerHeight() - currentItem.outerHeight() ) / 2 );
+ _position.at = "left top";
+ }
+
+ this.menuWrap
+ .zIndex( this.element.zIndex() + 1 )
+ .position( $.extend( {}, this.options.position, _position ) );
+
+ this._trigger( "open", event );
+ }
+ },
+
+ close: function( event ) {
+ if ( this.isOpen ) {
@jzaefferer Owner

Let's make this a guard clause, too. Less indentation FTW!

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((260 lines not shown))
+ "class": "ui-selectmenu-optgroup" + ( item.element.parent( "optgroup" ).attr( "disabled" ) ? " ui-state-disabled" : "" ),
+ html: item.optgroup
+ }).appendTo( ul );
+ currentOptgroup = item.optgroup;
+ }
+ that._renderItem( ul, item );
+ });
+ },
+
+ _renderItem: function( ul, item ) {
+ var li = $( "<li />" ).data( "ui-selectmenu-item", item );
+ if ( item.disabled ) {
+ li.addClass( "ui-state-disabled" );
+ }
+ li.append( $( "<a />", {
+ html: item.label,
@jzaefferer Owner

The option is read as text, we should also set it as text. Same above for optgroup.

@fnagel Collaborator
fnagel added a note

Done.

@fnagel Collaborator
fnagel added a note

Partly reverted, otherwise &#160; will be displayed as string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on the diff
ui/jquery.ui.selectmenu.js
((295 lines not shown))
+ },
+
+ _toggle: function( event ) {
+ if ( this.isOpen ) {
+ this.close( event );
+ } else {
+ this.open( event );
+ }
+ },
+
+ _buttonEvents: {
+ focus: function() {
+ // init Menu on first focus
+ this.refresh();
+ // reset focus class as its removed by ui.widget._setOption
+ this.button.addClass( "ui-state-focus" );
@jzaefferer Owner

Only because you use focusable together with disabling, right? If there's something accidentally removing the class, it should be reported there.

@fnagel Collaborator
fnagel added a note

Yes. With reported there you mean Widget Wiki page?

@jzaefferer Owner

I meant, reported against the buggy component.

@fnagel Collaborator
fnagel added a note

Im still not sure where to report. Sorry :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((375 lines not shown))
+ // change native select element
+ this.element[ 0 ].selectedIndex = item.index;
+ this._setSelected( item );
+ this._trigger( "select", event, { item: item } );
+
+ if ( item.index !== oldIndex ) {
+ this._trigger( "change", event, { item: item } );
+ }
+ },
+
+ _setSelected: function( item ) {
+ // update button text
+ this.buttonText.html( item.label );
+ // change ARIA attr
+ this.menuItems.find( "a" ).attr( "aria-selected", false );
+ this._getSelectedItem().find( "a" ).attr( "aria-selected", true );
@jzaefferer Owner

Can't you use item here?

@fnagel Collaborator
fnagel added a note

No, as we need the anchor within the menu. The item object contains the original option only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((374 lines not shown))
+ var oldIndex = this.element[ 0 ].selectedIndex;
+ // change native select element
+ this.element[ 0 ].selectedIndex = item.index;
+ this._setSelected( item );
+ this._trigger( "select", event, { item: item } );
+
+ if ( item.index !== oldIndex ) {
+ this._trigger( "change", event, { item: item } );
+ }
+ },
+
+ _setSelected: function( item ) {
+ // update button text
+ this.buttonText.html( item.label );
+ // change ARIA attr
+ this.menuItems.find( "a" ).attr( "aria-selected", false );
@jzaefferer Owner

Or filter on menuItems here with item.index? (regarding the below comment)

@fnagel Collaborator
fnagel added a note

Yes, that works fine. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((383 lines not shown))
+ },
+
+ _setSelected: function( item ) {
+ // update button text
+ this.buttonText.html( item.label );
+ // change ARIA attr
+ this.menuItems.find( "a" ).attr( "aria-selected", false );
+ this._getSelectedItem().find( "a" ).attr( "aria-selected", true );
+ this.button.attr( "aria-labelledby", this.menuItems.eq( item.index ).find( "a" ).attr( "id" ) );
+ },
+
+ _setOption: function( key, value ) {
+ this._super( key, value );
+
+ if ( key === "appendTo" ) {
+ this.menuWrap.appendTo( $( value || "body", this.element[ 0 ].ownerDocument )[ 0 ] );
@jzaefferer Owner

Why is this handled differently compared to the appendTo call in _drawMenu?

To access the body element, use this.document[ 0 ].body.

@fnagel Collaborator
fnagel added a note

I think I copied this from another widget and I have no idea why its different to to the appendTo call in _drawMenu. I will adopt autocomplete's code here, ok?

@jzaefferer Owner

Yep, let's do that.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ui/jquery.ui.selectmenu.js
((413 lines not shown))
+ _toggleAttr: function(){
+ if ( this.options.dropdown ) {
+ 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);
+ },
+
+ _getCreateOptions: function() {
+ return { disabled: !!this.element.attr( "disabled" ) };
+ },
+
+ _readOptions: function( options ) {
+ var data = [];
+ $.each( options, function( index, item ) {
@jzaefferer Owner

Use options.each instead? Its a jQuery object.

@fnagel Collaborator
fnagel added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on the diff
ui/jquery.ui.selectmenu.js
((433 lines not shown))
+ index: index,
+ value: option.attr( "value" ),
+ label: option.text() || "&nbsp;",
+ optgroup: optgroup.attr( "label" ) || "",
+ disabled: optgroup.attr( "disabled" ) || option.attr( "disabled" )
+ });
+ });
+ this.items = data;
+ },
+
+ _destroy: function() {
+ this.menuWrap.remove();
+ this.buttonWrap.remove();
+ this.element.show();
+ this.element.removeUniqueId();
+ this.label.attr( "for", this.ids.id );
@jzaefferer Owner

If element didn't have an id to start with, this will set the for attribute to a non-existing id. It won't be worse, but maybe there's something more useful to do? Not sure.

@fnagel Collaborator
fnagel added a note

Perhaps this should be handled when working on Hans mail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
Owner

I'm through with my code review. In the beginning I commented on a few unnecessary line comments, there are a bunch more later. I'd prefer to have most of them removed, and leave comments only to document workarounds and other less obvious sausages.