fix(tab): exclude hidden descendants (#579)

* Don't allow tabbing to elements inside hidden parents

* fix: use isVisible helper to exclude hidden elements

Hidden elements should be excluded from tab order.
Hidden attribute can be overwritten by stylesheets.

Co-authored-by: Ben Styles <[email protected]>

Author: ph-fritsche

src/__tests__/tab.js

@@ -17,7 +17,7 @@ test('fires events when tabbing between two elements', () => {
   userEvent.tab()
   expect(getEventSnapshot()).toMatchInlineSnapshot(`
     Events fired on: div
-    
+
     input#a[value=""] - keydown: Tab (9)
     input#a[value=""] - focusout
     input#b[value=""] - focusin
@@ -45,7 +45,7 @@ test('does not change focus if default prevented on keydown', () => {
   userEvent.tab()
   expect(getEventSnapshot()).toMatchInlineSnapshot(`
     Events fired on: div
-    
+
     input#a[value=""] - keydown: Tab (9)
     input#a[value=""] - keyup: Tab (9)
   `)
@@ -418,6 +418,26 @@ test('should not focus disabled elements', () => {
   expect(five).toHaveFocus()
 })
 
+test('should not focus elements inside a hidden parent', () => {
+  setup(`
+    <div>
+      <input data-testid="one" />
+      <div hidden="">
+        <button>click</button>
+      </div>
+      <input data-testid="three" />
+    </div>`)
+
+  const one = document.querySelector('[data-testid="one"]')
+  const three = document.querySelector('[data-testid="three"]')
+
+  userEvent.tab()
+  expect(one).toHaveFocus()
+
+  userEvent.tab()
+  expect(three).toHaveFocus()
+})
+
 test('should keep focus on the document if there are no enabled, focusable elements', () => {
   setup(`<button disabled>no clicky</button>`)
   userEvent.tab()

src/__tests__/utils.js

@@ -1,4 +1,5 @@
-import {isInstanceOfElement} from '../utils'
+import { screen } from '@testing-library/dom'
+import {isInstanceOfElement, isVisible} from '../utils'
 import {setup} from './helpers/utils'
 
 // isInstanceOfElement can be removed once the peerDependency for @testing-library/dom is bumped to a version that includes https://github.com/testing-library/dom-testing-library/pull/885
@@ -71,3 +72,19 @@ describe('check element type per isInstanceOfElement', () => {
     expect(() => isInstanceOfElement(element, 'HTMLSpanElement')).toThrow()
   })
 })
+
+test('check if element is visible', () => {
+  setup(`
+    <input data-testid="visibleInput"/>
+    <input data-testid="hiddenInput" hidden/>
+    <input data-testid="styledHiddenInput" style="display: none">
+    <input data-testid="styledDisplayedInput" hidden style="display: block"/>
+    <div style="display: none"><input data-testid="childInput" /></div>
+  `)
+
+  expect(isVisible(screen.getByTestId('visibleInput'))).toBe(true)
+  expect(isVisible(screen.getByTestId('styledDisplayedInput'))).toBe(true)
+  expect(isVisible(screen.getByTestId('styledHiddenInput'))).toBe(false)
+  expect(isVisible(screen.getByTestId('childInput'))).toBe(false)
+  expect(isVisible(screen.getByTestId('hiddenInput'))).toBe(false)
+})

src/tab.js

@@ -1,5 +1,5 @@
 import {fireEvent} from '@testing-library/dom'
-import {getActiveElement, FOCUSABLE_SELECTOR} from './utils'
+import {getActiveElement, FOCUSABLE_SELECTOR, isVisible} from './utils'
 import {focus} from './focus'
 import {blur} from './blur'
 
@@ -31,7 +31,11 @@ function tab({shift = false, focusTrap} = {}) {
   const enabledElements = [...focusableElements].filter(
     el =>
       el === previousElement ||
-      (el.getAttribute('tabindex') !== '-1' && !el.disabled),
+      (el.getAttribute('tabindex') !== '-1' &&
+        !el.disabled &&
+        // Hidden elements are not tabable
+        isVisible(el)
+      ),
   )
 
   if (enabledElements.length === 0) return

src/utils.js

@@ -293,6 +293,19 @@ function isClickableInput(element) {
   )
 }
 
+function isVisible(element) {
+  const getComputedStyle = getWindowFromNode(element).getComputedStyle
+
+  for(; element && element.ownerDocument; element = element.parentNode) {
+    const display = getComputedStyle(element).display
+    if (display === 'none') {
+      return false
+    }
+  }
+
+  return true
+}
+
 function eventWrapper(cb) {
   let result
   getConfig().eventWrapper(() => {
@@ -367,4 +380,5 @@ export {
   getSelectionRange,
   isContentEditable,
   isInstanceOfElement,
+  isVisible,
 }