Selectmenu #866

Closed
wants to merge 409 commits into
from

Conversation

Projects
None yet
7 participants
Owner

jzaefferer commented Dec 18, 2012

New PR for selectmenu, since #492 got too long.

See also http://wiki.jqueryui.com/w/page/12138056/Selectmenu

fnagel and others added some commits Feb 26, 2012

@fnagel fnagel Selectmenu: remove unwanted Menu mouseover event, fixes problem with …
…selected item highlighting on menu open
f2228b0
@fnagel fnagel Selectmenu: fix for IEs item hover problem, overflow CSS properties n…
…ow included
156d0fb
@fnagel fnagel Selectmenu: added empty.html visual test, small improvements in html …
…files
e299e9a
@fnagel fnagel Selectmenu: removed working but illogical closest context cea6844
@fnagel fnagel Selectmenu: small improvements, code clean-up, lint, coding guidelines b1a72e6
@fnagel fnagel Selectmenu: added a temp fix for Safari to make unit test work, see #… 72c17f0
@scottgonzalez scottgonzalez Revert "Selectmenu: added a temp fix for Safari to make unit test wor…
…k, see #492 (comment)"

This reverts commit 72c17f0.
8c10c1e
@scottgonzalez scottgonzalez Merge branch 'master' into selectmenu b4d9eee
@scottgonzalez scottgonzalez Tests: equals() -> equal(). 94f8514
@scottgonzalez scottgonzalez Merge branch 'master' into selectmenu 682a321
@kborchers kborchers Merge branch 'master' into selectmenu e38feea
@kborchers kborchers Selectmenu: Add links to all menu items to be in line with what menu …
…expects and remove the extra space added to disabled items
4a9e26c
@fnagel fnagel Selectmenu: fixed small regression which added unwanted, empty optgro…
…up to every menu
1092551
@fnagel fnagel Selectmenu: removed unnecessary unit test (has link test for disabled…
… items)
ec5f2ae
@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 fnagel Selectmenu: updated copyright notice 44f9b8f
@fnagel fnagel Selectmenu: split core unit test for state synchronization in keydown…
… and click
83f4249
@fnagel fnagel Selectmenu: follow-up to 'split core unit test for state synchronizat…
…ion in keydown and click'
06372cd
@scottgonzalez scottgonzalez Selectmenu: Fixed event unbinding. 549b97e
@fnagel fnagel Merge branch 'master' into selectmenu dd272e7
@fnagel fnagel Selectmenu: remove unbind Menu document event to reset mouseHandled f…
…lag, related to revision b8ad711
b6e2467
@fnagel fnagel Simplify licensing. c179902
@fnagel fnagel Merge branch 'master' into selectmenu daadc84
@fnagel fnagel Selectmenu: Upgrade jQuery to 1.8.0. 66156aa
@fnagel fnagel Selectmenu tests: Added test for widget() method. fb87e1c
@fnagel fnagel Selectmenu tests: Upgrade visual tests to jQuery to 1.8.0. 5e12c54
@fnagel fnagel Merge branch 'master' into selectmenu cca4e77
@fnagel fnagel Selectmenu: Upgrade jQuery to 1.8.2. fc729a8
@fnagel fnagel Merge branch 'master' into selectmenu c59fbba
@robertbeuligmann robertbeuligmann Selectmenu: ARIA tweaks for keyboard events and closed menu operation 20d19db
@robertbeuligmann robertbeuligmann Selectmenu: corrections based on code review. 8e1e956
@jzaefferer jzaefferer Selectmenu: Fix data naming, should be [namespace]-[widgetname]-[value] 99aa0ff
@fnagel fnagel Selectmenu: follow-up to "Fix data naming" 86f4c2f
@fnagel fnagel Selectmenu tests: add test for button aria-labelledby attribute e059376
@fnagel fnagel Selectmenu: small improvement for label id setting b8cce0c
@fnagel fnagel Selectmenu: add removeUniqueId to destroy method 9cf0f21
@fnagel fnagel Selectmenu: fixed aria-activedescendant for button element b2d50f9
@fnagel fnagel Selectmenu tests: aria-activedescendant für button e7e73ce
@fnagel fnagel Selectmenu: hopefully fixed multiple a11y bugs (value is recognized b…
…y screenreader: in collapsed state, when selecting a value, on tab focus; label points to button now)
7c1a9d9
@fnagel fnagel Selectmenu: reset aria-activedescendant to selected item when menu is…
… closed
a59b3c7
@fnagel fnagel Selectmenu: follow-up for menu, reset aria-activedescendant to select…
…ed item when menu is closed
228a9b9
@fnagel fnagel Selectmenu: Do not remove ui-state-focus class on first button focus b6e1c86
@fnagel fnagel Selectmenu: Do not remove ui-state-focus class on first button focus …
…(follow-up)
684b55b
@fnagel fnagel Selectmenu: focus selected item when menu opens 7725e4a
@fnagel fnagel Selectmenu: proposed fix for menu delay issue a9a6e3b
@fnagel fnagel Selectmenu tests: adjust accessibility test 94b3a65
@fnagel fnagel Merge branch 'master' into selectmenu 7ce8e05
@fnagel fnagel Selectmenu: cleanup, CGL and lint fix 12fe28f
@fnagel fnagel Selectmenu: updated copyright notice 0734314
@fnagel fnagel Selectmenu: fix doctype declaration 967e2b7
@fnagel fnagel Merge branch 'master' into selectmenu daec559
@fnagel fnagel Selectmenu tests: lint fixes 2bb459f
@fnagel fnagel Selectmenu: CSS fix for gravatar demo 4160978
@fnagel fnagel Selectmenu: lint fixes b7ee7b2
@fnagel fnagel Selectmenu tests: lint fixes d24e62b
@jzaefferer jzaefferer Selectmenu: Add selectmenu to build/test 8cbbf33
@jzaefferer jzaefferer Merge branch 'master' into selectmenu f86168b
@jzaefferer jzaefferer Selectmenu: Customize job title for testswarm, until merge to master dff9917
@jzaefferer jzaefferer Selectmenu: Fix testswarm job title a2d3ef3
@fnagel fnagel Selectmenu: Upgrade jQuery to 1.8.3 dec8399
@fnagel fnagel Selectmenu tests: Upgrade jQuery to 1.8.3 0d4a8dc
@fnagel fnagel Merge branch 'master' into selectmenu e25cdd8
@fnagel fnagel Selectmenu: Reformatted CSS to use better coding standard 709f75b
@fnagel fnagel Selectmenu: removed Position Plugin offset option to fix broken pop-u…
…p functionality
4e68c52
@fnagel fnagel Merge branch 'master' into selectmenu 36533a7
@fnagel fnagel Selectmenu: better position handling, fixes issue with wrong styles w…
…hen using custom positioning
7328333
@fnagel fnagel Selectmenu: updated documentation link 8a62210
@fnagel fnagel Selectmenu: alphabetical ordering of options and callback events b014bc6
@fnagel fnagel Selectmenu: remove unnecessary comments from _create 080b5bc
@fnagel fnagel Selectmenu: move button event binding in _drawButton 71e744a
@fnagel fnagel Selectmenu: remove unnecessary comments from _drawButton e581a13
@fnagel fnagel Selectmenu: improve element creation a177a92
@fnagel fnagel Selectmenu: replace placeholder entity with   1e6808a
@fnagel fnagel Selectmenu: improve comment in _drawMenu ac7b8f9
@fnagel fnagel Selectmenu: improve disabled check in open method f622428
@fnagel fnagel Selectmenu: added comment about popup positioning 18ecaf4

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ });
+
+ // hide original select tag
+ this.element.hide();
+
+ // create button
+ this.button = $( "<span>", {
+ "class": "ui-selectmenu-button ui-widget ui-state-default ui-corner-all",
+ 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

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+
+ // create button
+ this.button = $( "<span>", {
+ "class": "ui-selectmenu-button ui-widget ui-state-default ui-corner-all",
+ 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
+ })
+ .insertAfter( this.element );
+
+ this.button.prepend( $( "<span>", {
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

$( ... ).prependTo() for easier reading.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+
+ _drawMenu: function() {
+ var menuInstance,
+ that = this;
+
+ // create menu portion, append to body
+ this.menu = $( "<ul>", {
+ "aria-hidden": true,
+ "aria-labelledby": this.ids.button,
+ id: this.ids.menu
+ });
+
+ // wrap menu
+ this.menuWrap = $( "<div>", {
+ "class": "ui-selectmenu-menu",
+ width: this.button.outerWidth()
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

You probably want .outerWidth( this.button.outerWidth() ).

@fnagel

fnagel Apr 29, 2013

Member

.outerWidth is a setter?

@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Yup, as of jQuery 1.8. But it's been in UI for a long time (defined in jquery.ui.core.js).

@fnagel

fnagel Apr 29, 2013

Member

Ok, changed as more fitting.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+
+ // 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" );
+ // prevent inital focus from firing and checks if its a newly focused item
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Blank line before comments.

@scottgonzalez scottgonzalez and 2 others commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ // 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" );
+ // prevent inital focus from firing and checks if its a newly focused item
+ if ( !that.isOpen && that.focus !== undefined && item.index !== that.focus ) {
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Can that.focus !== undefined be collapsed to that.focus ?

@fnagel

fnagel Apr 29, 2013

Member

No, not possible. We discussed this before, I would need to search the thread in @jzaefferer review (older pull request).

@scottgonzalez

scottgonzalez May 7, 2013

Owner

It'd be good to find that thread, or figure out why again, because if this isn't covered by a unit test, we're almost guaranteed to regress on this.

@fnagel

fnagel May 7, 2013

Member

There is a test, its the "close" you asked about , see

https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003180

Old thread:

https://github.com/jquery/jquery-ui/pull/492#discussion_r2266258

There is another one, but I did not manage to find it

@jzaefferer

jzaefferer May 30, 2013

Owner

Use that.focusIndex != null instead of strict comparison to undefined. That works (test passes) and we use that everywhere else.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+
+ if ( that.isOpen ) {
+ 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
+ if ( !that.isOpen && that.focus !== undefined && item.index !== that.focus ) {
+ that._trigger( "focus", event, { item: item } );
+ if ( !that.isOpen ) {
+ that._select( item, event );
+ }
+ }
+ that.focus = item.index;
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

focus is a strange name for an index. It sounds like a method.

@fnagel

fnagel Apr 29, 2013

Member

Renamed to focusIndex.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ },
+ 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.isOpen && that.focus !== undefined && item.index !== that.focus ) {
+ that._trigger( "focus", event, { item: item } );
+ if ( !that.isOpen ) {
+ that._select( item, event );
+ }
+ }
+ that.focus = item.index;
+
+ // Set ARIA active descendant
+ that.button.attr( "aria-activedescendant", that.menuItems.eq( item.index ).attr( "id" ) );
+ },
+ // set ARIA role
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Comments like this aren't useful. It exactly duplicate the code.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ that._trigger( "focus", event, { item: item } );
+ if ( !that.isOpen ) {
+ that._select( item, event );
+ }
+ }
+ that.focus = item.index;
+
+ // Set ARIA active descendant
+ that.button.attr( "aria-activedescendant", that.menuItems.eq( item.index ).attr( "id" ) );
+ },
+ // set ARIA role
+ role: "listbox"
+ })
+ .menu( "instance" );
+
+ // adjust border radius
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Either remove this comment, or explain why the corners are changed.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+
+ // adjust border radius
+ this.menu.addClass( "ui-corner-bottom" ).removeClass( "ui-corner-all" );
+
+ // make sure focus stays on selected item
+ menuInstance.delay = 999999999;
+ // unbind uneeded Menu events
+ menuInstance._off( this.menu, "mouseleave" );
+ },
+
+ refresh: function() {
+ this.menu.empty();
+
+ var item,
+ options = this.element.find( "option" );
+ if ( options.length ) {
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Invert to:

if ( !options.length ) {
    return;
}

// full method logic

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ var item,
+ options = this.element.find( "option" );
+ if ( options.length ) {
+ this._readOptions( options );
+ this._renderMenu( this.menu, this.items );
+
+ this.menu.menu( "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 );
+ this._setAria( item.data( "ui-selectmenu-item" ) );
+
+ // set disabled state
+ this._setOption( "disabled", this._getCreateOptions().disabled );
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Why is this using _getCreateOptions()?

@fnagel

fnagel Apr 29, 2013

Member

Same functionality needed like on init. I assume @jzaefferer advised me to do so ;-)

@scottgonzalez scottgonzalez and 2 others commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ item = this._getSelectedItem();
+ // make sure menu is selected item aware
+ this.menu.menu( "focus", null, item );
+ this._setAria( item.data( "ui-selectmenu-item" ) );
+
+ // set disabled state
+ this._setOption( "disabled", this._getCreateOptions().disabled );
+ }
+ },
+
+ open: function( event ) {
+ if ( this.options.disabled ) {
+ return;
+ }
+ if ( !this.menuItems ) {
+ this.refresh();
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

When do we need to refresh on open?

@fnagel

fnagel Apr 29, 2013

Member

This is a fallback to make sure refresh() is called for sure before opening. Its only needed when the focus event fails on certain browsers / interactions.

@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

I just don't understand when this would ever occur. How do we end up in a situation where there are no processed items?

@fnagel

fnagel Apr 29, 2013

Member

We render the menu items lazy, after the button receives focus first time. At least IE9 does not fire the focus event.

@scottgonzalez

scottgonzalez May 7, 2013

Owner

We need a comment documenting why this is done, including a // support: IE9 (or whatever is accurate) line.

@fnagel

fnagel May 7, 2013

Member

I will recheck this in all browsers and add a comment.

@jzaefferer

jzaefferer May 10, 2013

Owner

I think related, or the actual issue, was this: On certain OS/browser combinations, a click doesn't trigger focus on the button. It triggers a click event, but the button doesn't actually receive focus, observable by the lack of focus outline and lack of keyboard interactions (click, then hitting space triggers only one click event, not two).

@fnagel

fnagel May 16, 2013

Member

I've added a comment.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ currentOptgroup = "";
+
+ $.each( items, function( index, item ) {
+ if ( item.optgroup !== currentOptgroup ) {
+ $( "<li>", {
+ "class": "ui-selectmenu-optgroup" + ( item.element.parent( "optgroup" ).attr( "disabled" ) ? " ui-state-disabled" : "" ),
+ text: item.optgroup
+ }).appendTo( ul );
+ currentOptgroup = item.optgroup;
+ }
+ that._renderItem( ul, item );
+ });
+ },
+
+ _renderItem: function( ul, item ) {
+ var li = $( "<li>" ).data( "ui-selectmenu-item", item ),
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

See autocomplete for _renderItemData(). Custom renderings shouldn't have to worry about storing the data.

@scottgonzalez scottgonzalez commented on the diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ },
+
+ _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" );
+ this._off( this.button, "focus" );
+ },
+ click: function( event ) {
+ this._toggle( event );
+ event.preventDefault();
+ },
+ keydown: function( event ) {
+ var prevDef = true;
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Just do the call inline.

@fnagel

fnagel Apr 29, 2013

Member

This would need to add eight times event.preventDefault();

@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Just like the assignment was added eight times?

@fnagel

fnagel Apr 29, 2013

Member

Perhaps I did not understand you. I should remove the prefDef = false statements and use preventDefault instead in the needed cases, right?

Currently we use event.preventDefault(); in eight of ten cases.

@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Yes, every place that you have prevDef = false;, replace it with event.preventDefault(); and remove the variable.

@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Oh, nevermind. I see now that you're inverting it to avoid the duplication. Let's just leave it.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ // change native select element
+ this.element[ 0 ].selectedIndex = item.index;
+ this._setText( this.buttonText, item.label );
+ this._setAria( item );
+ this._trigger( "select", event, { item: item } );
+
+ if ( item.index !== oldIndex ) {
+ this._trigger( "change", event, { item: item } );
+ }
+ },
+
+ _setAria: function( item ) {
+ var link = this.menuItems.eq( item.index ),
+ id = link.attr( "id" );
+
+ // change ARIA attr
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

Remove comment.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+
+ _appendTo: function() {
+ var element = this.options.appendTo;
+
+ if ( element ) {
+ element = element.jquery || element.nodeType ?
+ $( element ) :
+ this.document.find( element ).eq( 0 );
+ }
+
+ if ( !element ) {
+ element = this.element.closest( ".ui-front" );
+ }
+
+ if ( !element.length ) {
+ element = this.document[0].body;

fnagel added some commits Apr 29, 2013

@fnagel fnagel Selectmenu: clean up custom render demo e1590c4
@fnagel fnagel Selectmenu: clean up default demo c0fa69d
@fnagel fnagel Selectmenu Tests: updated jQuery and old testsuite version 026f517
@fnagel fnagel Selectmenu Tests: rename wrapper divs 4d986a4
@fnagel fnagel Selectmenu Tests: cleanup unit tests c73f7e8
@fnagel fnagel Selectmenu Tests: remove unneeded test 8b6c81e
@fnagel fnagel Selectmenu Tests: use simulate instead of trigger 1fa8c20
@fnagel fnagel Selectmenu Tests: remove unneeded test f2bd236
@fnagel fnagel Selectmenu Tests: add toLowerCase() to node type checks 1cca829
@fnagel fnagel Selectmenu Tests: clean up spacing and intention 0d7b25d
@fnagel fnagel Selectmenu Test: rename widget method test d66528b
@fnagel fnagel Selectmenu Tests: change doctype to lower case c1667ba
@fnagel fnagel Selectmenu Tests: added labels to disabled empty visual test ad30163
@fnagel fnagel Selectmenu: clean up CSS 0264f07
@fnagel fnagel Selectmenu: improve id generation 60eed79
@fnagel fnagel Selectmenu: don't quote click, remove extraneous space a29cfb8
@fnagel fnagel Selectmenu: Comments start with an uppercase letter 68c7485
@fnagel fnagel Selectmenu: simplify tabindex attribute setting on button create b84cd7e
@fnagel fnagel Selectmenu: set aria states as string if needed 8b4a304
@fnagel fnagel Selectmenu: improve readability when creating button elements 00ab497
@fnagel fnagel Selectmenu: blank before comment 5e9ad9b
@fnagel fnagel Selectmenu: rename var to focusIndex 0631d76
@fnagel fnagel Selectmenu: adjust comments, trim trailing spaces ee9c443
@fnagel fnagel Selectmenu: use guard clause for option length checking in refresh 17b6ff7
@fnagel fnagel Selectmenu: add spaces d4f13bc
@fnagel fnagel Selectmenu: add renderItemData method similar to autocomplete 666927b

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ }
+
+ this._readOptions( options );
+ this._renderMenu( this.menu, this.items );
+
+ this.menu.menu( "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 );
+ this._setAria( item.data( "ui-selectmenu-item" ) );
+
+ // Set disabled state
+ this._setOption( "disabled", this._getCreateOptions().disabled );
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

You should read the disabled property directly. You should never have a direct call to _getCreateOptions().

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 29, 2013

ui/jquery.ui.selectmenu.js
+ if ( !element.length ) {
+ element = this.document[ 0 ].body;
+ }
+
+ return element;
+ },
+
+ _toggleAttr: function(){
+ 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" ) };
@scottgonzalez

scottgonzalez Apr 29, 2013

Owner

.prop() not .attr()

@fnagel

fnagel Apr 29, 2013

Member

Changed.

Member

fnagel commented Apr 30, 2013

I've fixed most of @scottgonzalez hints and bookmarked all others. From tomorrow I'm on vacation, so I will miss the UI meeting and cant work on Selectmenu until next week.

Thanks for reviewing!

Member

fnagel commented May 7, 2013

Hey @scottgonzalez & @jzaefferer,

please check the following comments in order to fix all of Scott's comments:

https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003174
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003180
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003347
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003358
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003472
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003527
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003653
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003788
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4002071

ps: I needed to add the links as code, otherwise the needed anchors are removed by GitHub.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff May 7, 2013

tests/unit/selectmenu/selectmenu_core.js
@@ -0,0 +1,161 @@
+(function( $ ) {
+
+module( "selectmenu: core" );
+
+asyncTest("accessibility", function () {
@scottgonzalez

scottgonzalez May 7, 2013

Owner

Two spacing issues here:
asyncTest( "accessibility", function() {

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff May 7, 2013

tests/unit/selectmenu/selectmenu_core.js
+
+ equal( menu.attr( "role" ), "listbox", "menu role" );
+ equal( menu.attr( "aria-labelledby" ), button.attr( "id" ), "menu aria-labelledby" );
+ equal( menu.attr( "aria-hidden" ), "true", "menu aria-hidden" );
+ equal( menu.attr( "tabindex" ), 0, "menu tabindex" );
+ equal(
+ menu.attr( "aria-activedescendant" ),
+ links.eq( element[ 0 ].selectedIndex ).attr( "id" ),
+ "menu aria-activedescendant"
+ );
+ $.each( links, function( index ){
+ equal( $( this ).attr( "role" ), "option", "menu link #" + index +" role" );
+ equal( $( this ).attr( "tabindex" ), -1, "menu link #" + index +" tabindex" );
+ });
+ start();
+ }, 1 );
@scottgonzalez

scottgonzalez May 7, 2013

Owner

You can leave out the , 1 here.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff May 7, 2013

tests/unit/selectmenu/selectmenu_core.js
@@ -0,0 +1,161 @@
+(function( $ ) {
+
+module( "selectmenu: core" );
+
+asyncTest("accessibility", function () {
+ var links,
+ element = $( "#speed" ).selectmenu(),
+ button = element.selectmenu( "widget" ),
+ menu = element.selectmenu( "menuWidget" );
+
+ button.simulate( "focus" );
+ links = menu.find( "li.ui-menu-item a" );
+
+ expect(12 + links.length * 2);
@scottgonzalez

scottgonzalez May 7, 2013

Owner

spacing inside parens

Owner

jzaefferer commented May 10, 2013

I think I responded to all open questions. Let me know if I missed something.

Member

fnagel commented May 15, 2013

Hey @scottgonzalez please check the following discussions:

https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003180
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003358
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003472
https://github.com/jquery/jquery-ui/pull/866/#discussion-diff-4003653
Member

fnagel commented May 16, 2013

I've rechecked in all browser and added Support comments as proposed.

I noticed an issue in IE8 and IE7: the change event unit test fails. The change event wont fire but it fails only in the unit test. It works when used by actual mouse / keyboard interaction. Any ideas?

shouldn't it be this._off( this.document, this._documentClick ); ?

Member

fnagel replied May 17, 2013

It should remove all events on document within our namespace, should be ok. What do you think @jzaefferer ?

is it still ok when i have 2 selectmenus?

Member

fnagel replied May 17, 2013

Tested successfully in default demo, which includes three Selectmenus. Unit test work too.

sorry ;)

Member

fnagel replied May 17, 2013

Member

fnagel commented May 24, 2013

Any feedback on my latest comments?

fnagel referenced this pull request in fnagel/jquery-ui May 30, 2013

Closed

Official jQuery UI Selectmenu Widget #140

Owner

jzaefferer commented May 30, 2013

@fnagel once you've addressed those four we just discussed, can you close this PR and create a new one, once more? Then we can do a (hopefully) final review there.

Member

fnagel commented May 30, 2013

@jzaefferer Yes, I will pen a new one.

There is one thing I mentioned in a comment above:

I noticed an issue in IE8 and IE7: the change event unit test fails. 
The change event wont fire but it fails only in the unit test. 
It works when used by actual mouse / keyboard interaction. Any ideas?
Owner

scottgonzalez commented May 30, 2013

Perhaps related to the fact that you're treating .simulate( "focus" ) as synchronous, while focus is always async in IE.

Member

fnagel commented May 30, 2013

@scottgonzalez I've changed to asyncTest but that didn't help.

Owner

scottgonzalez commented Jun 26, 2013

We were able to track down the problem to .simulate( "click" ) and delegated events. Can you go through the tests and change all of the instances of .simulate( "click" ) to just .trigger( "click" ) and see if everything continues to pass for you? We'll probably want to write a real click simulation at some point that handles mouse down, up, etc.

Owner

scottgonzalez commented Jun 26, 2013

To remove the looping, you can add something like this inside _move():

if ( this.menu.menu( "isFirstItem" ) && /^previous/.test( direction ) ||
        this.menu.menu( "isLastItem" ) && /^next/.test( direction ) ) {
    return;
}

We'll need tests for this as well.

fnagel closed this Jul 1, 2013

fnagel referenced this pull request Jul 1, 2013

Merged

Selectmenu #1023

Owner

scottgonzalez commented on 8e1e956 Dec 11, 2013

@robertbeuligmann Can you please sign our CLA?

Contributor

robertbeuligmann replied Dec 11, 2013

Owner

scottgonzalez replied Dec 11, 2013

Thanks Robert.

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