Bugs in focusMenuItem() in selectmenu #2327

Closed
kpozin opened this Issue Aug 23, 2011 · 8 comments

Projects

None yet

5 participants

@kpozin
kpozin commented Aug 23, 2011

The helper method

function focusMenuItem() {
    self.list.find( ".ui-btn-active" ).focus();
}

has a few bugs:

  • If there's a placeholder element, it may receive focus.
  • For multi-select lists, the focus event is unnecessarily triggered on all selected items.
  • Related to the above, if there are multiple active buttons, the last always receives focus (I don't know if this is intentional, but it's not necessarily intuitive).
  • If there are no initially selected items, then nothing receives focus, so it's not possible to use tab and up/down arrow keys.

Here's a first pass at a rewritten version that addresses all of those issues. If people think this looks reasonable, I'll put in a pull request:

function focusMenuItem() {
    var active = self.list.find( ".ui-btn-active:not(.ui-selectmenu-placeholder)" ).first();
    if ( active.length ) {
        active.focus();
    } else {
        self.list.find( "li:not(.ui-disabled, .ui-li-divider, .ui-selectmenu-placeholder)" ).first().focus();
    }
}
@toddparker

Thanks for the suggestion. Would you mind creating a pull request so this isn't lost in the shuffle?

@johnbender johnbender was assigned Sep 5, 2011
@jaspermdegroot
Member

@johnbender @scottjehl

See the 4 issue mentioned above.
The first has been fixed by Scott's commit 99e27a1
The 2nd and 3rd: Makes sense to me to add .first().
The 4th: add... if no ".ui-btn-active a" then focus on first "a" (this will probably fix #4039)

What do you think?

@jaspermdegroot
Member

I will create a PR for this.

@toddparker

@uGoMobi - Great. BTW - As long as you have tested these tweaks and unit tests are passing, can can commit right to master, no need for a PR unless you need feedback or additional testing.

@jaspermdegroot
Member

@gabrielschulhof Hi Gabriel!

I want to change focusMenuItem to make sure that the first item in a custom multiple select menu gets focus when the menu is opened, for keyboard accessibility. See "New code" below.

If I use menuItem = self.list.find( "li:not( .ui-disabled ) a" ).first() it doesn't work. The first item in the object seems to be the select button itself (which is not even wrapped in a LI). When I change .first() into .eq(1) the function works, but it doesn't make sense to me.

Can you help me with this?

New code:

function focusMenuItem() {
    var activeMenuItem = self.list.find( "." + $.mobile.activeBtnClass + " a" ).first(),
        menuItem = self.list.find( "li:not( .ui-disabled ) a" ).first();

    if ( activeMenuItem.length ) {
        activeMenuItem.focus();
    } else {
        menuItem.focus();
    }                   
}

Current code:

function focusMenuItem() {
    self.list.find( "." + $.mobile.activeBtnClass + " a" ).focus();
}

File: https://github.com/jquery/jquery-mobile/blob/master/js/widgets/forms/select.custom.js

Note: Another thing to look into is that when you select an item in a multiple select menu with keyboard you loose focus.

@jaspermdegroot
Member

I just noticed issue #4061. If we make changes there we might have to change focusMenuItem too.

@gabrielschulhof
Collaborator

I think I fixed this along with #4039 ...

@jaspermdegroot
Member

@gabrielschulhof - Works like a charm. Thanks!

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