fix(index.js) The tabIndex of audio, video and details was left to th…

…e default if set to some NaN (#610)

* fix(index.js) The tabIndex of contentEditable elements was assumed to be zero in any case, not only in the case it was not specifically set.

* Simplified and optimized 'getTabIndex'.

* Made better use of short-circuit evaluation in 'isNodeMatchingSelectorTabbable', reducing the chances to call the computationally expensive 'isNodeMatchingSelectorFocusable'.

* (Re)Added 'isScope' parameter to 'getTabIndex'. This parameter wasn't present in the master branch, so I lost it in the rebase process.

* Added tests for a `contenteditable` with negative tab index.

* Fixed bug, now the getTabIndex can return 0 not only when the tabindex is not explicitly set, but also when is set to a value that gives NaN when parsed as integer (which would have been resulted in the default browser tabIndex, as if the tabindex wasn't set at all). Also added test for the case an element has a tab index that can't be turned into an integer.

* Added changeset, added entry in CHANGELOG.md and wrote more tests.

* Be consistent with asterisks

* Sync package.json/yarn.lock with beta-530 base branch

Co-authored-by: Stefan Cameron <[email protected]>

.changeset/happy-zebras-attend.md

@@ -0,0 +1,5 @@
+---
+'tabbable': patch
+---
+
+Fixed a bug in `getTabIndex`: the tab index of `<audio>`, `<viedo>` and `<details>` was left to the browser default if explicitly set to a value that couldn't be parsed as integer, leading to inconsistent behaviour across browsers. Also slightly modified the function's logic to make it more efficient. Finally added tests to cover the fix.

CHANGELOG.md

@@ -1,8 +1,12 @@
 # Changelog
 
+## UNRELEASED
+
+- Fixed a bug in `getTabIndex`: the tab index of `<audio>`, `<viedo>` and `<details>` was left to the browser default if explicitly set to a value that couldn't be parsed as integer, leading to inconsistent behaviour across browsers. Also slightly modified the function's logic to make it more efficient. Finally added tests to cover the fix.
+
 ## 5.3.0-beta.1
 
-- Add support for setting `getShadowRoot: true` as an easy way to simply _enable_ shadow DOM support. This is the equivalent of setting `getShadowRoot: () => false`, which means tabbable will find nodes in __open__ shadow roots only.
+- Add support for setting `getShadowRoot: true` as an easy way to simply *enable* shadow DOM support. This is the equivalent of setting `getShadowRoot: () => false`, which means tabbable will find nodes in **open** shadow roots only.
 
 ## 5.3.0-beta.0
 

src/index.js

@@ -141,40 +141,27 @@ const getCandidatesIteratively = function (
   return candidates;
 };
 
-const isContentEditable = function (node) {
-  return node.contentEditable === 'true';
-};
-
 const getTabindex = function (node, isScope) {
-  const tabindexAttr = parseInt(node.getAttribute('tabindex'), 10);
-
-  if (!isNaN(tabindexAttr)) {
-    return tabindexAttr;
-  }
-
-  // Browsers do not return `tabIndex` correctly for contentEditable nodes;
-  // so if they don't have a tabindex attribute specifically set, assume it's 0.
-  if (isContentEditable(node)) {
-    return 0;
-  }
-
-  // in Chrome, <details/>, <audio controls/> and <video controls/> elements get a default
-  //  `tabIndex` of -1 when the 'tabindex' attribute isn't specified in the DOM,
-  //  yet they are still part of the regular tab order; in FF, they get a default
-  //  `tabIndex` of 0; since Chrome still puts those elements in the regular tab
-  //  order, consider their tab index to be 0.
-  //
-  // isScope is positive for custom element with shadow root or slot that by default
-  // have tabIndex -1, but need to be sorted by document order in order for their
-  // content to be inserted in the correct position
-  if (
-    (isScope ||
-      node.nodeName === 'AUDIO' ||
-      node.nodeName === 'VIDEO' ||
-      node.nodeName === 'DETAILS') &&
-    node.getAttribute('tabindex') === null
-  ) {
-    return 0;
+  if (node.tabIndex < 0) {
+    // in Chrome, <details/>, <audio controls/> and <video controls/> elements get a default
+    // `tabIndex` of -1 when the 'tabindex' attribute isn't specified in the DOM,
+    // yet they are still part of the regular tab order; in FF, they get a default
+    // `tabIndex` of 0; since Chrome still puts those elements in the regular tab
+    // order, consider their tab index to be 0.
+    // Also browsers do not return `tabIndex` correctly for contentEditable nodes;
+    // so if they don't have a tabindex attribute specifically set, assume it's 0.
+    //
+    // isScope is positive for custom element with shadow root or slot that by default
+    // have tabIndex -1, but need to be sorted by document order in order for their
+    // content to be inserted in the correct position
+    if (
+      (isScope ||
+        /^(AUDIO|VIDEO|DETAILS)$/.test(node.tagName) ||
+        node.isContentEditable) &&
+      isNaN(parseInt(node.getAttribute('tabindex'), 10))
+    ) {
+      return 0;
+    }
   }
 
   return node.tabIndex;
@@ -365,9 +352,9 @@ const isNodeMatchingSelectorFocusable = function (options, node) {
 
 const isNodeMatchingSelectorTabbable = function (options, node) {
   if (
-    !isNodeMatchingSelectorFocusable(options, node) ||
     isNonTabbableRadio(node) ||
-    getTabindex(node) < 0
+    getTabindex(node) < 0 ||
+    !isNodeMatchingSelectorFocusable(options, node)
   ) {
     return false;
   }

test/e2e/focusable.e2e.js

@@ -25,6 +25,8 @@ describe('focusable', () => {
       const expectedFocusableIds = [
         'contenteditable-true',
         'contenteditable-nesting',
+        'contenteditable-negative-tabindex',
+        'contenteditable-NaN-tabindex',
         'input',
         'input-readonly',
         'select',
@@ -38,7 +40,9 @@ describe('focusable', () => {
         'negative-select',
         'hiddenParentVisible-button',
         'audio-control',
+        'audio-control-NaN-tabindex',
         'video-control',
+        'video-control-NaN-tabindex',
       ];
 
       const container = document.createElement('div');

test/e2e/isFocusable.e2e.js

@@ -107,17 +107,41 @@ describe('isFocusable', () => {
     it('returns true for any element with a `contenteditable` attribute with a truthy value', () => {
       const container = document.createElement('div');
       container.innerHTML = `<div contenteditable="true">contenteditable div</div>
-        <p contenteditable="true">contenteditable paragraph</p>`;
+        <p contenteditable="true">contenteditable paragraph</p>
+        <div contenteditable="true" tabindex="-1">contenteditable div focusable but not tabbable</div>
+        <div contenteditable="true" tabindex="NaN">contenteditable div focusable and tabbable</div>
+        <audio tabindex="foo" controls>audio controls focusable and tabbable</audio>
+        <video tabindex="bar" controls>video controls focusable and tabbable</video>`;
       document.body.append(container);
 
       const editableDiv = getByText(container, 'contenteditable div');
       const editableParagraph = getByText(
         container,
         'contenteditable paragraph'
       );
+      const editableDivWithNegativeTabIndex = getByText(
+        container,
+        'contenteditable div focusable but not tabbable'
+      );
+      const editableDivWithNanTabIndex = getByText(
+        container,
+        'contenteditable div focusable and tabbable'
+      );
+      const audioWithNanTabIndex = getByText(
+        container,
+        'audio controls focusable and tabbable'
+      );
+      const videoWithNanTabIndex = getByText(
+        container,
+        'video controls focusable and tabbable'
+      );
 
       expect(isFocusable(editableDiv)).to.eql(true);
       expect(isFocusable(editableParagraph)).to.eql(true);
+      expect(isFocusable(editableDivWithNegativeTabIndex)).to.eql(true);
+      expect(isFocusable(editableDivWithNanTabIndex)).to.eql(true);
+      expect(isFocusable(audioWithNanTabIndex)).to.eql(true);
+      expect(isFocusable(videoWithNanTabIndex)).to.eql(true);
     });
 
     it('returns true for any element with a non-negative `tabindex` attribute', () => {

test/e2e/isTabbable.e2e.js

@@ -107,17 +107,41 @@ describe('isTabbable', () => {
     it('returns true for any element with a `contenteditable` attribute with a truthy value', () => {
       const container = document.createElement('div');
       container.innerHTML = `<div contenteditable="true">contenteditable div</div>
-        <p contenteditable="true">contenteditable paragraph</p>`;
+        <p contenteditable="true">contenteditable paragraph</p>
+        <div contenteditable="true" tabindex="-1">contenteditable div focusable but not tabbable</div>
+        <div contenteditable="true" tabindex="NaN">contenteditable div focusable and tabbable</div>
+        <audio tabindex="foo" controls>audio controls focusable and tabbable</audio>
+        <video tabindex="bar" controls>video controls focusable and tabbable</video>`;
       document.body.append(container);
 
       const editableDiv = getByText(container, 'contenteditable div');
       const editableParagraph = getByText(
         container,
         'contenteditable paragraph'
       );
+      const editableDivWithNegativeTabIndex = getByText(
+        container,
+        'contenteditable div focusable but not tabbable'
+      );
+      const editableDivWithNanTabIndex = getByText(
+        container,
+        'contenteditable div focusable and tabbable'
+      );
+      const audioWithNanTabIndex = getByText(
+        container,
+        'audio controls focusable and tabbable'
+      );
+      const videoWithNanTabIndex = getByText(
+        container,
+        'video controls focusable and tabbable'
+      );
 
       expect(isTabbable(editableDiv)).to.eql(true);
       expect(isTabbable(editableParagraph)).to.eql(true);
+      expect(isTabbable(editableDivWithNegativeTabIndex)).to.eql(false);
+      expect(isTabbable(editableDivWithNanTabIndex)).to.eql(true);
+      expect(isTabbable(audioWithNanTabIndex)).to.eql(true);
+      expect(isTabbable(videoWithNanTabIndex)).to.eql(true);
     });
 
     it('returns true for any element with a non-negative `tabindex` attribute', () => {

test/e2e/tabbable.e2e.js

@@ -26,6 +26,7 @@ describe('tabbable', () => {
         'tabindex-hrefless-anchor',
         'contenteditable-true',
         'contenteditable-nesting',
+        'contenteditable-NaN-tabindex',
         'input',
         'input-readonly',
         'select',
@@ -37,7 +38,9 @@ describe('tabbable', () => {
         'tabindex-div',
         'hiddenParentVisible-button',
         'audio-control',
+        'audio-control-NaN-tabindex',
         'video-control',
+        'video-control-NaN-tabindex',
       ];
 
       const container = document.createElement('div');

test/fixtures/basic.html

@@ -11,6 +11,12 @@
     </div>
   </div>
 </div>
+<div id="contenteditable-negative-tabindex" contenteditable="true" tabindex="-1" style="padding:1em">
+  this editable text is focusable but not tabbable
+</div>
+<div id="contenteditable-NaN-tabindex" contenteditable="true" tabindex="NaN" style="padding:1em">
+  editable text
+</div>
 
 <input id="input" type="text" />
 <input id="input-readonly" readonly />
@@ -50,9 +56,11 @@
 
 <audio id="audio-control" controls></audio>
 <audio id="audio-nocontrol"></audio>
+<audio id="audio-control-NaN-tabindex" tabindex="foo" controls></audio>
 
 <video id="video-control" controls></video>
 <video id="video-nocontrol"></video>
+<video id="video-control-NaN-tabindex" tabindex="bar" controls></video>
 
 <iframe
   id="iframe"