jquery/jquery-ui — pull request #1224 by scottgonzalez (2014-04-08 15:28:20+00:00)
Title: Selectmenu review

This is the first pass (just reviewing the source, not the tests or demos) of today's review by @jzaefferer and I.

The cleanup commit is mostly just style fixes with a few performance and file size tweaks.

The active item commit simplifies the code by not using active items at all. Menus only use active items for submenus, which we're not concerned about here. Prior to this change, the styling for the active/focused item was different immediately after opening a menu, but switched back to the standard focus styling as soon as any other interaction occurred (via mouse or keyboard).

The simplification of the focused item selection just avoids the roundtrip from selectmenu to menu to selectmenu. This change can be debated since it does create a difference in how items are selected (we're not always going through menu now), but the logic is exactly the same as before, with less overhead.

We're also wondering if we should change the label's `for` attribute as done on https://github.com/jquery/jquery-ui/blob/baf6bc5c27003468052d81589855b6587f004d94/ui/selectmenu.js#L73. Presumably this is left over from when we were using an actual button, so it could receive focus. However, now that we're using a span, browsers will not move focus even if the `for` attribute is updated. We should probably just use `aria-labelledby` on the span instead, but we'll need testing to make sure that we're not breaking anything in AT.