1
\$\begingroup\$

This emulates behavior of tab and shifttab with and .

I.e., switch focus to the next/previous select field with parent class .tab (which are not siblings in the DOM.)

It works, but is there any way to make it more elegant? Especially the if/then and the limit to range. I'm fairly new to JavaScript.

$(document).on('keydown', '.tab select', function (e) {
    let tabs = $(".tab select");                        //get all tabs
    let tab = tabs.index($($(document.activeElement))); //focused tab
    if (e.which == 37) {                                //left arrow
        tab -= 1;
    } else {
        if (e.which == 39) {                            //right arrow
            tab += 1;
        }
    };
    tab = Math.min(tabs.length, Math.max(tab, 0));      //stay in range
    tabs.eq(tab).focus()
});
\$\endgroup\$
2
\$\begingroup\$

One thing you could consider is making this a little more generic and thus reusable. Currently you seem to assume, that there is only one .tab element in your document. If you had multiple (independent) .tab elements, each with their own set of select elements and you did't want to allow "tabbing" between then, then $(".tab select") will select all selects and not only the ones in the "current" .tab. So instead of

let tabs = $(".tab select");

I'd use

let tabs = $(this).closest(".tab").find("select");

Furthermore instead of hardcoding the selectors, consider making them variable, so that the script could be used with other class names and form elements:

function initArrowKeyTabbing(parentSelector, formElementSelector) {
  $(document).on('keydown', parentSelector + " " + formElementSelector, function (e) {
    $(this).closest(parentSelector).find(formElementSelector);
    // ...
  }
}

initArrowKeyTabbing(".tab", "select");

In the line

let tab = tabs.index($($(document.activeElement)));

you are calling jQuery twice. tabs.index($(document.activeElement)) would suffice. For the matter of the fact, .index() alternatively takes a DOM element as its parameter, so you even only need tabs.index(document.activeElement).


With an if ... else if ... construct you don't need the brackets around the second if:

if (e.which == 37) {
    tab -= 1;
} else if (e.which == 39) {
    tab += 1;
}

In this case instead of using if, a switch statement would be more flexible:

switch (e.which) {
    case 37:
       tab--; // Short form of tab -= 1;
       break;
    case 39:
       tab++;
       break;
}

The which event property is deprecated. The "proper" replacement is the key property, which however requires concessions towards IE and Edge, which use non-standard key names:

switch (e.key) {
  case "Left": // IE/Edge specific value
  case "ArrowLeft":
    tab--;
    break;
  case "Right": // IE/Edge specific value
  case "ArrowRight":
    tab++;
    break;
}
\$\endgroup\$
  • \$\begingroup\$ I understood from the (which are not siblings in the DOM) that the OP wants tabbing between all .tab select’s. \$\endgroup\$ – morbusg Mar 21 '19 at 14:11
  • \$\begingroup\$ @morbusg Yes, but as i understand it, all selects all inside the same tab. That tab element isn't really necessary, if the script should apply to all selects on the page, however if there are multiple tab elements, the "tabbing" could be "isolated" to each tab. \$\endgroup\$ – RoToRa Mar 21 '19 at 15:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.