Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10665 closed bug (wontfix)

Selectmenu: does not preserve tabindex

Reported by: Simen Bekkhus Owned by:
Priority: minor Milestone: none
Component: ui.selectmenu Version: 1.11.1
Keywords: Cc:
Blocked by: Blocking:

Description

I'm not able to provide a fiddle for this, so I'll just reference the code that has what I think the problem is.

When you call the refresh-method of selectmenu, the _refreshMenu method always sets the disabled option, based on whether or not the actual element is disabled. See this line of code. Then, if the element is disabled, the tabindex is set to -1. If the element is *not* disabled, it's set to 0. See this line of code.

That last thing I believe is wrong. tabindex shouldn't be set to 0 if the element is not disabled, it should be set to whatever the tabindex of the backing select-element is, and if that's not set, then it should be set to 0.

I created a pull request with a proposed fix for this.

Change History (9)

comment:1 Changed 8 years ago by tj.vantoll

Status: newopen
Summary: When calling refresh on a selectmenu, tabindex is lostSelectmenu: does not preserve tabindex

Actually the problem seems bigger than just refresh(). If I assign a tabindex to the <select> it is not applied to the button, before even calling refresh(): http://jsfiddle.net/tj_vantoll/5jqjfzvt/.

SimenB > Could you expand your PR to address this as well?

comment:2 Changed 8 years ago by Scott González

It's generally a bad idea to set a tab index greater than 0. I'd prefer to keep the refresh logic the way it is.

comment:3 Changed 8 years ago by Scott González

Resolution: wontfix
Status: openclosed

Here's a recent W3C discussion related to tabindex greater than 0: http://lists.w3.org/Archives/Public/public-html/2014Oct/0027.html

comment:4 Changed 8 years ago by Simen Bekkhus

While I appreciate (and totally agree) with your reasoning, I still think it's flawed.

The intro for Selectmenu on your site here is

Duplicates and extends the functionality of a native HTML select element to overcome the limitations of the native control.

If you ignore tabindex, you no longer duplicate the functionality of native HTML select.

Even though W3C might in the future discourage use of tabindex, I still think Selectmenu should mirror whatever the markup for the element is. If someone has set a tabindex on the select-element, they've probably put it on surrounding elements as well. Then when using Selectmenu, the tabbing gets messed up.

It seems weird that Selectmenu should be so opinionated. We use it (mainly) to be able to style our selects, not for it to impose a suggestion still in draft form from W3C that removes (maybe wrong word) functionality that's been in the browser forever.

How about printing a warning of some sort in the console instead, maybe when using jQuery Migrate or something?

Again, I completely agree with the reasoning, but I'm currently working on an application with thousands of lines of code, a lot of it (infested) with tabindexes, and for jQuery UI Selectmenu to require quite a massive refactoring before working properly with regards to tabindex (without modifying/extending the source, which we currently do) seems wrong to me.

Last edited 8 years ago by Simen Bekkhus (previous) (diff)

comment:5 Changed 8 years ago by Scott González

jQuery Migrate is for jQuery core, not jQuery UI or any other jQuery plugins. It also prints messages about deprecated functionality, while this was never something that worked in jQuery UI.

comment:6 Changed 8 years ago by Simen Bekkhus

Ah, OK, I thought it was for jQuery in general.

We had tabbing working, and with the change in the PR, refresh worked as well.

Would it be possible to take a look at why tabindex isn't working properly, instead of removing support for it altogether?

comment:7 Changed 8 years ago by Simen Bekkhus

If I expand the PR to cover the initialization as well, can that make it in?

The problem is with tabindex || this.options.disabled ? -1 : 0. Pseudo code: if tabindex or disabled, then -1, otherwise zero.

Wrapping the right side of || in parenthesis fixes it. See fiddle.

I override _drawMenu, otherwise it's the same as tj.vantoll's fiddle.

Last edited 8 years ago by Simen Bekkhus (previous) (diff)

comment:8 Changed 8 years ago by tj.vantoll

Here's an extension that implements the tabindex mirroring you're looking for:

$.widget( "ui.selectmenu", $.ui.selectmenu, {
    _create: function() {
        this._super();
        this._setTabIndex();
    },
    _setTabIndex: function() {
        this.button.attr( "tabindex",
            this.options.disabled ? -1 :
            this.element.attr( "tabindex" ) || 0 );
    },
    _setOption: function( key, value ) {
        this._super( key, value );
        if ( key === "disabled" ) {
            this._setTabIndex();
        }
    }
});

Demo: http://jsfiddle.net/tj_vantoll/b3fuezsb/

If others are interested in this functionality being natively incorporated they can comment here or vote on this issue. For now we're going to leave the behavior as is, and if you need custom tabindex attributes use the extension.

comment:9 Changed 8 years ago by Simen Bekkhus

I still think it's inherently wrong not to mimic the native select as closely as possible out of the box, but this works for me. Thank you for the short and sweet snippet!

EDIT: Maybe you could add this to the examples, like with autocomplete with categoriers?

Last edited 8 years ago by Simen Bekkhus (previous) (diff)
Note: See TracTickets for help on using tickets.