Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17492 closed defect (fixed)

[regression] dijit/form/Select breaks tab navigation with custom tabindex

Reported by: Colin Snover Owned by: Colin Snover
Priority: high Milestone: 1.9.2
Component: Dijit - Form Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description

Reproduction:

  1. Create a DOM structure like:
<input tabindex="1">
<select tabindex="3" data-dojo-type="dijit/form/Select"></select>
<select tabindex="2" data-dojo-type="dijit/form/Select"></select>
<input tabindex="4">
  1. Click the first input element
  2. Tab twice

Actual: First tab goes to the select with tabindex 2; second tab causes focus to disappear, or move to the wrong element, or refuses to move, depending upon browser

Expected: First tab goes to the select with tabindex 2; second tab goes to the select with tabindex 3

This is caused by an incorrect call to this.inherited in dijit/form/Select#_onFocus that causes _KeyNavMixin#_onFocus to be invoked which sets the tabindex of the select to -1. this.inherited should just not be called (which will match FilteringSelect? and others).

This is not an issue in 1.8 because _KeyNavMixin was not a thing that was mixed in.

Change History (7)

comment:1 Changed 6 years ago by Colin Snover <github.com@…>

Resolution: fixed
Status: newclosed

In 4a29a24f1d5d1d3a262432a5f17c75b68523cad0/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:2 Changed 6 years ago by Colin Snover <github.com@…>

In 8ce0043af0b962b726e4be6f46d80d264ed872b3/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:3 Changed 6 years ago by bill

Your change prevents _FormWidgetMixin::_onFocus() from getting called too, so it's quite possible you broke something. Also, you need to have an (automated) test case.

comment:4 Changed 6 years ago by bill

this.inherited should just not be called (which will match FilteringSelect? and others).

I don't know why you think that matches FilteringSelect?. FilteringSelect? doesn't even have an _onFocus() method.

comment:5 Changed 6 years ago by Colin Snover

I don’t like to make loud statements about my ability to be perfect, since I’m not, but it’s much more possible I didn’t break anything, since:

  1. I bloody tested it,
  2. the unit tests still pass, and
  3. there is nothing worthwhile about the code in _FormWidgetMixin#_onFocus. At best it itself is a waste of bytes since the problem it “fixes” is vanishingly tiny; in reality it seems to do nothing. In fact there is even a comment right in the code to remove it in 2.0 or earlier.

Also, please, let’s not have the pot calling the kettle black here. If you want to ask for tests, that’s fine and respectable, but don’t *demand* them selectively. When this regression was introduced, a major refactoring of the widget occurred with no new tests. Neither you nor Doug added tests in changeset [31124], nor did you put new tests in changeset [31291] which was a response to a regression complaint too. In fact, out of the last 20 changesets committed to Dijit there have only been two new tests, in 8ec57c206baffb89d104b4e378e8aff522c84926 and in 8a13496a301eaa5f0cbe6b984cb95af19b99acff. Shall I complain about that? No, because writing tests in DOH extremely sucks, and with no ability to see test coverage and plans for Dojo 1.x coming to an end, there are vastly better things both of us can do with our time.

When I said this will match FilteringSelect? I meant despite the fact it has very similar behaviour it has no such need for _onFocus.

comment:6 Changed 6 years ago by bill

The rule is to always add tests when possible. There might have been some checkins that slipped through w/out tests, but as #16925 explains, [31124] and [31291] don't need new tests because they address a failure in an existing test.

In any case, please submit future changes as pull requests so that I can review them. This is the policy for anyone (outside of Doug and me) making changes to dijit.

comment:7 Changed 6 years ago by Bill Keese <bill@…>

In bfd8d2aa02ae704a847a0795ce26d564018b5638/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.