#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
Status: | new → open |
---|---|
Summary: | When calling refresh on a selectmenu, tabindex is lost → Selectmenu: does not preserve tabindex |
comment:2 Changed 8 years ago by
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
Resolution: | → wontfix |
---|---|
Status: | open → closed |
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
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 select
s, 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 tabindex
es, 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.
comment:5 Changed 8 years ago by
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
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
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.
comment:8 Changed 8 years ago by
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
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?
Actually the problem seems bigger than just
refresh()
. If I assign atabindex
to the<select>
it is not applied to the button, before even callingrefresh()
: http://jsfiddle.net/tj_vantoll/5jqjfzvt/.SimenB > Could you expand your PR to address this as well?