Selectmenu review #1224

scottgonzalez · 2014-04-08T15:28:20Z

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

jquery-ui/ui/selectmenu.js

Line 73 in baf6bc5

    
           this.label = $( "label[for='" + this.ids.element + "']" ).attr( "for", this.ids.button );

. 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.

Menus only use active items for nested menus which selectmenu doesn't support. Selectmenu should only be working with focused items.

Don't go through menu to select the currently focused item. Renamed _selectMenu() to _selectFocusedItem() for clarity.

tjvantoll · 2014-04-08T15:40:11Z

tests/unit/selectmenu/selectmenu_core.js

@@ -222,12 +222,11 @@ $.each([

 		function checkItemClasses() {
 			focusedItem = menu.find( "li.ui-state-focus" );
+			focusedItem.each(function() {
+				console.log( this.innerHTML );
+			})


Did you mean to leave this logging in?

fnagel · 2014-04-09T10:16:18Z

Hey Scott, hey Jörn,

PR looks nice and seem to work just fine in my tests.

As Scott mentioned we now lost the possibility to distinguish between the active and the focused item after menu has been opened. Please keep in mind we just implemented this in the last (?) review.

Regarding the label for change: sounds reasonable but needs testing. I'm not sure if this was needed to prevent the default behavior but it's possible it's just a leftover.
We'll keep the click event to jump to the span, right?

scottgonzalez · 2014-04-09T12:30:18Z

As Scott mentioned we now lost the possibility to distinguish between the active and the focused item after menu has been opened. Please keep in mind we just implemented this in the last (?) review.

We looked back during our review and even with 67e7f18 it didn't actually work properly. If we can get this to work properly, that'd be great. But right now it's kinda hacky and doesn't work, so we just removed it.

Regarding the label for change: sounds reasonable but needs testing.

Yup. I tried testing with VoiceOver during our review, but VoiceOver wasn't announcing anything useful even for standard elements. I need to figure out if I somehow got it into a less useful state.

We'll keep the click event to jump to the span, right?

Yup.

jzaefferer · 2014-04-09T12:33:36Z

But right now it's kinda hacky and doesn't work, so we just removed it.

I'm still not completely sure what it is supposed to do. The only difference I see between master and this PR is that the active style is used when opening the menu, until focus is moved to another item. That seems to work fine. I can't tell why that would be preferred over just styling the active item as focused.

scottgonzalez · 2014-04-09T12:37:16Z

Ignore the broken implementation. Go look at any select in Chrome and you'll see persistent active styling (via a checkmark icon).

fnagel · 2014-04-09T13:01:17Z

But right now it's kinda hacky and doesn't work, so we just removed it.

I agree it was unclean, hacky solution but I was not able to figure out a better way without adding too much code.

I'm still not completely sure what it is supposed to do. The only difference I see between master and this PR is that the active style is used when opening the menu, until focus is moved to another item.

Adding a different class for the selected option upon opening. That's it.

Ignore the broken implementation. Go look at any select in Chrome and you'll see persistent active styling (via a checkmark icon).

I'm not sure what exactly you are talking about. Can you provide a screenshot?

scottgonzalez · 2014-04-09T13:08:14Z

I'm not sure what exactly you are talking about. Can you provide a screenshot?

Of course you don't, you're on Windows ;-)

jzaefferer · 2014-04-09T13:50:13Z

Gotcha. So that class needs to stay until something is select, and not be removed immediately when focus moves. This may need more modifications in menu to implement. Since Windows doesn't implement this in their native controls, I think we're okay with releasing without it.

fnagel · 2014-04-09T16:17:35Z

I already thought about that :-/ On what data OSX decides if to show the icon?

This may need more modifications in menu to implement. Since Windows doesn't implement this in their native controls, I think we're okay with releasing without it.

Agree.

fnagel · 2014-04-16T18:55:58Z

Regarding the label issue:

I've tested three versions: current master, Scott's review branch and a version with aria-labelledby which can be found here: https://github.com/fnagel/jquery-ui/tree/scottgonzalez-selectmenu-review

Conclusion: label is not read at all. aria-labelledby and for seem to have no effect - at least on Win 8.1 with FF 28 and NVDA 2014.1

Any ideas?

scottgonzalez · 2014-04-16T19:17:29Z

@colinbdclark @hanshillen @dylanb @Wildebrew Any ideas on why we're not getting an association between the label and the custom select?

hanshillen · 2014-04-17T00:08:23Z

I only had a quick look, but from what I can see the labeling relationship is broken by the aria-labelled attribute that gets added on focus. You will notice that if you use virtual navigation or an inspection tool to announce the combo, the label text is announced correctly. But the moment you tab to it, the currently selected dropdown item is announced, because of the added aria-labelledby attribute that targets the currently selected item in the dropdown.

If you look at how a native select element is exposed to AT, you can see the following:

on initial focus, the element is exposed as a combobox role, with the associated label's text as accessible name, and the currently selected item's text as accessible value.
As the user starts changing the selected value with the arrow keys, each change causes an accessible focus event to fire, with an exposed role of "list item" and the selected item's text as accessible name (an accessible value is not exposed here).

The trick is to mimic this as closely as possible. This means that on initial focus you want the combobox to expose the currently selected item as accessible value, not as accessible name (which should be the label's text). Unfortunately there is no aria-value attribute, so it's a bit tricky to achieve. You can consider wrapping the [role=combobox] around a text input or span[role=textbox] element, and make that the focusable element. The text inside the input or textbox role will automatically be exposed as accessible value. The fact that the parent is [role=combobox] will ensure the screen reader announces it as such. Unfortunately, simply using [role=combobox] does not seem to cause the contained text to be exposed as accessible value the way it does with role=textbox (although IE does not even do that).

If the textbox approach is not feasible, you can also consider extending the aria-labelledby attribute to target both the label and the currently selected item:

<label for="speed-button" id="speed-button-label">Select a speed</label>
<span class="ui-selectmenu-button ui-widget ui-state-default ui-corner-all" tabindex="0" id="speed-button" role="combobox" aria-expanded="false" aria-autocomplete="list" aria-owns="speed-menu" aria-haspopup="true" style="width: 198px;" aria-activedescendant=" aria-labelledby="**speed-button-label** ui-id-3" ui-id-3" aria-disabled="false">...</span>

(note the two values in the aria-labelledby attribute)
This way the combo will be announced as "Select a speed medium combobox", which is better than what it announces now.

You would think that aria-activedescendant would be sufficient to expose the value on initial focus, but but that would cause the list item to be exposed as focused, not the combobox (for some reason JAWS won't announce the combobox role and label before announcing the active dropdown item, but maybe that's just a bug we have to file).

Speaking or aria-activedescendant, I also noticed that in FF the screen reader does not announce changes to the selected item while the selectmenu is collapsed. This is caused by the fact that the dropdown is hidden with display:none, breaking the aria-activedescendant relationship. In IE the newly selected item is announced, but since there is no element to focus it will appear as if focus went back to the document. So screen magnifiers will scroll to the top left corner of the viewport every time the selected item is changed with the arrow keys. To resolve this, display:none should not be used, or if it is, then aria-activedescendant should not point to it. With the first option, you could consider if it's possible to overlay the the dropdown on top of the combobox, using clipping to only show the selected item. With the second option, only use aria-activedescendant while the dropdown is open. When it's collapsed, the value change in the "button" should be used to announce the selected value (although this is difficult to get announced consistently as well).

In other words, as usual there is no ideal or perfectly supported solution. You'll have to try different things and see what works best. I hope my ramblings can be of some use!

Ref gh-1224

Menus only use active items for nested menus which selectmenu doesn't support. Selectmenu should only be working with focused items. Ref gh-1224

scottgonzalez added 3 commits April 8, 2014 11:17

Selectmenu: Cleanup

3b73efa

Selectmenu: Don't use active items at all

a581b04

Menus only use active items for nested menus which selectmenu doesn't support. Selectmenu should only be working with focused items.

Selectmenu: Simplify selection of focused item via keyboard

1dfaa19

Don't go through menu to select the currently focused item. Renamed _selectMenu() to _selectFocusedItem() for clarity.

tjvantoll reviewed Apr 8, 2014
View reviewed changes

[amend]: Remove logging

52b6eca

scottgonzalez added a commit that referenced this pull request Apr 18, 2014

Selectmenu: Cleanup

1849655

Ref gh-1224

scottgonzalez added a commit that referenced this pull request Apr 18, 2014

Selectmenu: Don't use active items at all

1272fca

Menus only use active items for nested menus which selectmenu doesn't support. Selectmenu should only be working with focused items. Ref gh-1224

scottgonzalez closed this in 62812d9 Apr 18, 2014

scottgonzalez deleted the selectmenu-review branch July 17, 2014 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Selectmenu review #1224

Selectmenu review #1224

Uh oh!

scottgonzalez commented Apr 8, 2014

Uh oh!

tjvantoll Apr 8, 2014

Uh oh!

scottgonzalez Apr 8, 2014

Uh oh!

fnagel commented Apr 9, 2014

Uh oh!

scottgonzalez commented Apr 9, 2014

Uh oh!

jzaefferer commented Apr 9, 2014

Uh oh!

scottgonzalez commented Apr 9, 2014

Uh oh!

fnagel commented Apr 9, 2014

Uh oh!

scottgonzalez commented Apr 9, 2014

Uh oh!

jzaefferer commented Apr 9, 2014

Uh oh!

fnagel commented Apr 9, 2014

Uh oh!

fnagel commented Apr 16, 2014

Uh oh!

scottgonzalez commented Apr 16, 2014

Uh oh!

hanshillen commented Apr 17, 2014

Uh oh!

Uh oh!

Selectmenu review #1224

Selectmenu review #1224

Uh oh!

Conversation

scottgonzalez commented Apr 8, 2014

Uh oh!

tjvantoll Apr 8, 2014

Choose a reason for hiding this comment

Uh oh!

scottgonzalez Apr 8, 2014

Choose a reason for hiding this comment

Uh oh!

fnagel commented Apr 9, 2014

Uh oh!

scottgonzalez commented Apr 9, 2014

Uh oh!

jzaefferer commented Apr 9, 2014

Uh oh!

scottgonzalez commented Apr 9, 2014

Uh oh!

fnagel commented Apr 9, 2014

Uh oh!

scottgonzalez commented Apr 9, 2014

Uh oh!

jzaefferer commented Apr 9, 2014

Uh oh!

fnagel commented Apr 9, 2014

Uh oh!

fnagel commented Apr 16, 2014

Uh oh!

scottgonzalez commented Apr 16, 2014

Uh oh!

hanshillen commented Apr 17, 2014

Uh oh!

Uh oh!