[#664] Re-introduce fallback to displayCheck=full if node not in docu…

…ment Fixes #664 There are legitimate cases where tabbable's APIs are expected to work on container nodes that are not actually attached to the document. The result is that the `getClientRects()` API never returns any rects for visible nodes, resulting in tabbable thinking ALL nodes in the container are hidden.
focus-trap · May 6, 2022 · 986a526 · 986a526
1 parent 37411b3
commit 986a526
Show file tree

Hide file tree

Showing 10 changed files with 337 additions and 124 deletions.
diff --git a/.changeset/swift-jeans-roll.md b/.changeset/swift-jeans-roll.md
@@ -0,0 +1,5 @@
+---
+'tabbable': patch
+---
+
+Fixed an issue with `displayCheck=full` (default setting) determining all nodes were hidden when the container is not attached to the document. In this case, tabbable will revert to a `displayCheck=none` mode, which is the equivalent legacy behavior. Also updated the `displayCheck` option docs to add warnings about this corner case for the `full` and `non-zero-area` modes. `non-zero-area` behaves differently in the corner case. See the docs for more info.
diff --git a/README.md b/README.md
@@ -133,12 +133,18 @@ Type: `full` | `non-zero-area` | `none` . Default: `full`.
 
 Configures how to check if an element is displayed.
 
-To reliably check if an element is tabbable/focusable, Tabbable defaults to the most reliable option to keep consistent with browser behavior, however this comes at a cost since every node needs to be validated as displayed. The `full` process checks for computed display property of an element and each of the element ancestors. For this reason Tabbable offers the ability of an alternative way to check if an element is displayed (or completely opt out of the check).
+To reliably check if an element is tabbable/focusable, Tabbable defaults to the most reliable option to keep consistent with browser behavior, however this comes at a cost since every node needs to be validated as displayed using Web APIs that cause layout reflow.
+
+For this reason Tabbable offers the ability of an alternative way to check if an element is displayed (or completely opt out of the check).
 
 The `displayCheck` configuration accepts the following options:
 
-- `full`: (default) Most reliably resembling browser behavior, this option checks that an element is displayed and all of his ancestors are displayed as well (notice that this doesn't exclude `visibility: hidden` or elements with zero size). This option will cause layout reflow, however. If that is a concern, consider the `none` option.
-- `non-zero-area`: This option checks display under the assumption that elements that are not displayed have zero area (width AND height equals zero). While not keeping true to browser behavior, this option may enhance accessibility, as zero-size elements with focusable content are considered a strong accessibility anti-pattern. Like the `full` option, this option also causes layout reflow, and should have basically the same performance. Consider the `none` option if reflow is a concern.
+- `full`: (default) Most reliably resembling browser behavior, this option checks that an element is displayed, which requires all of his ancestors are displayed as well (notice that this doesn't exclude `visibility: hidden` or elements with zero size). This option will cause layout reflow, however. If that is a concern, consider the `none` option.
+    - ⚠️ If the container given to `tabbable()` or `focusable()`, or the node given to `isTabbable()` or `isFocusable()`, is not attached to the window's main `document`, the display check will default to __"none"__ (see below for details) because the APIs used to determine a node's display are not supported unless it is attached (i.e. the browser does not calculate its display unless it is attached). This has effectively been tabbable's behavior for a _very_ long time, and you may never have encountered an issue if the nodes with which you used tabbable were always displayed anyway (i.e. the "none" mode assumption was coincidentally correct).
+    - You may encounter the above situation if, for example, you render to a node via React, and this node is [not attached](https://github.com/facebook/react/issues/9117#issuecomment-284228870) to the document (or perhaps, due to timing, it is not _yet_ attached at the time you use tabbable's APIs).
+- `non-zero-area`: This option checks display under the assumption that elements that are not displayed have zero area (width AND height equals zero). While not keeping true to browser behavior, this option may enhance accessibility, as zero-size elements with focusable content are considered a strong accessibility anti-pattern.
+    - Like the `full` option, this option also causes layout reflow, and should have basically the same performance. Consider the `none` option if reflow is a concern.
+    - ⚠️ As with the `full` option, there is a nuance in behavior depending on whether tabbable APIs are executed on attached vs detached nodes using this mode: Attached nodes that are actually displayed will be deemed visible. Detached nodes, _even though displayed_ will always be deemed __hidden__ because detached nodes always have a zero area as the browser does not calculate is dimensions.
 - `none`: This completely opts out of the display check. **This option is not recommended**, as it might return elements that are not displayed, and as such not tabbable/focusable and can break accessibility. Make sure you know which elements in your DOM are not displayed and can filter them out yourself before using this option.
 
 > ⚠️ __Testing in JSDom__ (e.g. with Jest): See notes about [testing in JSDom](#testing-in-jsdom).
@@ -153,7 +159,7 @@ Type: `boolean | (node: FocusableElement) => ShadowRoot | boolean | undefined`
 
 - `boolean`:
     - `true` simply enables shadow DOM support for any __open__ shadow roots, but never presumes there is an undisclosed shadow. This is the equivalent of setting `getShadowRoot: () => false`
-    - `false` (default) disables shadow DOM support.
+    - `false` (default) disables shadow DOM support in so far as calculated tab order and closed shadow roots are concerned. If a child of a shadow (open or closed) is given to `isTabbable()` or `isFocusable()`, the shadow DOM is still considered for visibility and display checks.
 - `function`:
     - `node` will be a descendent of the `rootNode` given to `tabbable()`, `isTabbable()`, `focusable()`, or `isFocusable()`.
     - Returns: The node's `ShadowRoot` if available, `true` indicating a `ShadowRoot` is attached but not available (i.e. "undisclosed"), or a _falsy_ value indicating there is no shadow attached to the node.

diff --git a/src/index.js b/src/index.js
@@ -246,6 +246,11 @@ const isZeroArea = function (node) {
   return width === 0 && height === 0;
 };
 const isHidden = function (node, { displayCheck, getShadowRoot }) {
+  // NOTE: visibility will be `undefined` if node is detached from the document
+  //  (see notes about this further down), which means we will consider it visible
+  //  (this is legacy behavior from a very long way back)
+  // NOTE: we check this regardless of `displayCheck="none"` because this is a
+  //  _visibility_ check, not a _display_ check
   if (getComputedStyle(node).visibility === 'hidden') {
     return true;
   }
@@ -256,6 +261,28 @@ const isHidden = function (node, { displayCheck, getShadowRoot }) {
     return true;
   }
 
+  // The root node is the shadow root if the node is in a shadow DOM; some document otherwise
+  //  (but NOT _the_ document; see second 'If' comment below for more).
+  // If rootNode is shadow root, it'll have a host, which is the element to which the shadow
+  //  is attached, and the one we need to check if it's in the document or not (because the
+  //  shadow, and all nodes it contains, is never considered in the document since shadows
+  //  behave like self-contained DOMs; but if the shadow's HOST, which is part of the document,
+  //  is hidden, or is not in the document itself but is detached, it will affect the shadow's
+  //  visibility, including all the nodes it contains). The host could be any normal node,
+  //  or a custom element (i.e. web component). Either way, that's the one that is considered
+  //  part of the document, not the shadow root, nor any of its children (i.e. the node being
+  //  tested).
+  // If rootNode is not a shadow root, it won't have a host, and so rootNode should be the
+  //  document (per the docs) and while it's a Document-type object, that document does not
+  //  appear to be the same as the node's `ownerDocument` for some reason, so it's safer
+  //  to ignore the rootNode at this point, and use `node.ownerDocument`. Otherwise,
+  //  using `rootNode.contains(node)` will _always_ be true we'll get false-positives when
+  //  node is actually detached.
+  const nodeRootHost = getRootNode(node).host;
+  const nodeIsAttached =
+    nodeRootHost?.ownerDocument.contains(nodeRootHost) ||
+    node.ownerDocument.contains(node);
+
   if (!displayCheck || displayCheck === 'full') {
     if (typeof getShadowRoot === 'function') {
       // figure out if we should consider the node to be in an undisclosed shadow and use the
@@ -283,23 +310,50 @@ const isHidden = function (node, { displayCheck, getShadowRoot }) {
           node = parentElement;
         }
       }
+
       node = originalNode;
     }
     // else, `getShadowRoot` might be true, but all that does is enable shadow DOM support
     //  (i.e. it does not also presume that all nodes might have undisclosed shadows); or
     //  it might be a falsy value, which means shadow DOM support is disabled
 
-    // didn't find it sitting in an undisclosed shadow (or shadows are disabled) so now we
-    //  can just test to see if it would normally be visible or not
-    // this works wherever the node is: if there's at least one client rect, it's
-    //  somehow displayed; it also covers the CSS 'display: contents' case where the
-    //  node itself is hidden in place of its contents; and there's no need to search
-    //  up the hierarchy either
-    return !node.getClientRects().length;
+    // Since we didn't find it sitting in an undisclosed shadow (or shadows are disabled)
+    //  now we can just test to see if it would normally be visible or not, provided it's
+    //  attached to the main document.
+    // NOTE: We must consider case where node is inside a shadow DOM and given directly to
+    //  `isTabbable()` or `isFocusable()` -- regardless of `getShadowRoot` option setting.
+
+    if (nodeIsAttached) {
+      // this works wherever the node is: if there's at least one client rect, it's
+      //  somehow displayed; it also covers the CSS 'display: contents' case where the
+      //  node itself is hidden in place of its contents; and there's no need to search
+      //  up the hierarchy either
+      return !node.getClientRects().length;
+    }
+
+    // Else, the node isn't attached to the document, which means the `getClientRects()`
+    //  API will __always__ return zero rects (this can happen, for example, if React
+    //  is used to render nodes onto a detached tree, as confirmed in this thread:
+    //  https://github.com/facebook/react/issues/9117#issuecomment-284228870)
+    //
+    // It also means that even window.getComputedStyle(node).display will return `undefined`
+    //  because styles are only computed for nodes that are in the document.
+    //
+    // NOTE: THIS HAS BEEN THE CASE FOR YEARS. It is not new, nor is it caused by tabbable
+    //  somehow. Though it was never stated officially, anyone who has ever used tabbable
+    //  APIs on nodes in detached containers has actually implicitly used tabbable in what
+    //  was later (as of v5.2.0 on Apr 9, 2021) called `displayCheck="none"` mode -- essentially
+    //  considering __everything__ to be visible because of the innability to determine styles.
   } else if (displayCheck === 'non-zero-area') {
+    // NOTE: Even though this tests that the node's client rect is non-zero to determine
+    //  whether it's displayed, and that a detached node will __always__ have a zero-area
+    //  client rect, we don't special-case for whether the node is attached or not. In
+    //  this mode, we do want to consider nodes that have a zero area to be hidden at all
+    //  times, and that includes attached or not.
     return isZeroArea(node);
   }
 
+  // visible, as far as we can tell, or per current `displayCheck` mode
   return false;
 };
 

diff --git a/test/e2e/e2e.helpers.js b/test/e2e/e2e.helpers.js
@@ -26,7 +26,8 @@ export function removeAllChildNodes(parent) {
  * Renders a fixture into the body with support for shadow dom hydration
  * @param {string} content html content to be used as fixture
  * @param {SetupFixtureOptions} options
- * @returns {HTMLDivElement} return.container the element the fixture was rendered into
+ * @returns {{ container: HTMLDivElement }}
+ *  - container: the element the fixture was rendered into
  */
 export function setupFixture(content, options = {}) {
   const win = options.window || window;

diff --git a/test/e2e/focusable.e2e.js b/test/e2e/focusable.e2e.js
@@ -21,39 +21,80 @@ describe('focusable', () => {
   });
 
   describe('example fixtures', () => {
-    it('correctly identifies focusable elements in the "basic" example', () => {
-      const expectedFocusableIds = [
-        'contenteditable-true',
-        'contenteditable-nesting',
-        'contenteditable-negative-tabindex',
-        'contenteditable-NaN-tabindex',
-        'input',
-        'input-readonly',
-        'select',
-        'select-readonly',
-        'href-anchor',
-        'tabindex-hrefless-anchor',
-        'textarea',
-        'textarea-readonly',
-        'button',
-        'tabindex-div',
-        'negative-select',
-        'hiddenParentVisible-button',
-        'audio-control',
-        'audio-control-NaN-tabindex',
-        'video-control',
-        'video-control-NaN-tabindex',
-      ];
+    [true, false].forEach((inDocument) => {
+      it(`correctly identifies focusable elements in the "basic" example ${
+        inDocument ? '(container IN doc)' : '(container NOT in doc)'
+      }`, () => {
+        let expectedFocusableIds;
+
+        if (inDocument) {
+          expectedFocusableIds = [
+            'contenteditable-true',
+            'contenteditable-nesting',
+            'contenteditable-negative-tabindex',
+            'contenteditable-NaN-tabindex',
+            'input',
+            'input-readonly',
+            'select',
+            'select-readonly',
+            'href-anchor',
+            'tabindex-hrefless-anchor',
+            'textarea',
+            'textarea-readonly',
+            'button',
+            'tabindex-div',
+            'negative-select',
+            'hiddenParentVisible-button',
+            'displaycontents-child',
+            'audio-control',
+            'audio-control-NaN-tabindex',
+            'video-control',
+            'video-control-NaN-tabindex',
+          ];
+        } else {
+          expectedFocusableIds = [
+            'contenteditable-true',
+            'contenteditable-nesting',
+            'contenteditable-negative-tabindex',
+            'contenteditable-NaN-tabindex',
+            'input',
+            'input-readonly',
+            'select',
+            'select-readonly',
+            'href-anchor',
+            'tabindex-hrefless-anchor',
+            'textarea',
+            'textarea-readonly',
+            'button',
+            'tabindex-div',
+            'negative-select',
+            'displaynone-textarea',
+            'visibilityhidden-button',
+            'hiddenParent-button',
+            'hiddenParentVisible-button',
+            'displaycontents',
+            'displaycontents-child',
+            'displaycontents-child-displaynone',
+            'audio-control',
+            'audio-control-NaN-tabindex',
+            'video-control',
+            'video-control-NaN-tabindex',
+          ];
+        }
 
-      const container = document.createElement('div');
-      container.innerHTML = fixtures.basic;
-      document.body.append(container);
+        const container = document.createElement('div');
+        container.innerHTML = fixtures.basic;
 
-      const focusableElements = focusable(container);
+        if (inDocument) {
+          document.body.append(container);
+        }
 
-      expect(getIdsFromElementsArray(focusableElements)).to.eql(
-        expectedFocusableIds
-      );
+        const focusableElements = focusable(container);
+
+        expect(getIdsFromElementsArray(focusableElements)).to.eql(
+          expectedFocusableIds
+        );
+      });
     });
 
     it('correctly identifies focusable elements in the "nested" example', () => {

diff --git a/test/e2e/isFocusable.e2e.js b/test/e2e/isFocusable.e2e.js
@@ -439,10 +439,15 @@ describe('isFocusable', () => {
         <div data-testid="nested-under-displayed-contents" tabindex="0"></div>
       </div>
     `;
-    function setupDisplayCheck() {
+
+    function setupDisplayCheck(inDocument = true) {
       const container = document.createElement('div');
       container.innerHTML = fixture;
-      document.body.append(container);
+
+      if (inDocument) {
+        document.body.append(container);
+      }
+
       return {
         displayedTop: getByTestId(container, 'displayed-top'),
         displayedNested: getByTestId(container, 'displayed-nested'),
@@ -510,25 +515,30 @@ describe('isFocusable', () => {
       expect(isFocusable(displayedContentsTop, options)).to.eql(false);
       expect(isFocusable(nestedUnderDisplayedContents, options)).to.eql(true);
     });
-    it('return elements without checking display ("none" option)', () => {
-      const {
-        displayedTop,
-        displayedNested,
-        displayedZeroSize,
-        displayedNoneTop,
-        nestedUnderDisplayedNone,
-        displayedContentsTop,
-        nestedUnderDisplayedContents,
-      } = setupDisplayCheck();
 
-      const options = { displayCheck: 'none' };
-      expect(isFocusable(displayedTop, options)).to.eql(true);
-      expect(isFocusable(displayedNested, options)).to.eql(true);
-      expect(isFocusable(displayedZeroSize, options)).to.eql(true);
-      expect(isFocusable(displayedNoneTop, options)).to.eql(true);
-      expect(isFocusable(nestedUnderDisplayedNone, options)).to.eql(true);
-      expect(isFocusable(displayedContentsTop, options)).to.eql(true);
-      expect(isFocusable(nestedUnderDisplayedContents, options)).to.eql(true);
+    [true, false].forEach((inDocument) => {
+      it(`return elements without checking display ("${
+        inDocument ? 'none' : 'full'
+      }" option, container ${inDocument ? 'IN doc' : 'NOT in doc'})`, () => {
+        const {
+          displayedTop,
+          displayedNested,
+          displayedZeroSize,
+          displayedNoneTop,
+          nestedUnderDisplayedNone,
+          displayedContentsTop,
+          nestedUnderDisplayedContents,
+        } = setupDisplayCheck(inDocument);
+
+        const options = { displayCheck: 'none' };
+        expect(isFocusable(displayedTop, options)).to.eql(true);
+        expect(isFocusable(displayedNested, options)).to.eql(true);
+        expect(isFocusable(displayedZeroSize, options)).to.eql(true);
+        expect(isFocusable(displayedNoneTop, options)).to.eql(true);
+        expect(isFocusable(nestedUnderDisplayedNone, options)).to.eql(true);
+        expect(isFocusable(displayedContentsTop, options)).to.eql(true);
+        expect(isFocusable(nestedUnderDisplayedContents, options)).to.eql(true);
+      });
     });
   });
 });