fix: exclude focusable elements hidden indirectly

vaadin · Feb 9, 2022 · 6b4ad03 · 6b4ad03
1 parent ad822bb
commit 6b4ad03
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 3 deletions.
diff --git a/packages/component-base/src/focus-trap-controller.js b/packages/component-base/src/focus-trap-controller.js
@@ -3,7 +3,7 @@
  * Copyright (c) 2021 - 2022 Vaadin Ltd.
  * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
  */
-import { getFocusableElements, isElementFocused } from './focus-utils.js';
+import { getFocusableElements, isElementFocused, isElementHidden } from './focus-utils.js';
 
 /**
  * A controller for trapping focus within a DOM node.
@@ -110,7 +110,10 @@ export class FocusTrapController {
    * @private
    */
   __focusNextElement(backward = false) {
-    const focusableElements = this.__focusableElements;
+    // Filter out hidden elements before detecting which element to focus next.
+    // We can't do that during initialization, because by the time `trapFocus()`
+    // is called in case of overlay, focusable items might not yet be visible.
+    const focusableElements = this.__focusableElements.filter((el) => !isElementHidden(el));
     const step = backward ? -1 : 1;
     const currentIndex = this.__focusedElementIndex;
     const nextIndex = (focusableElements.length + currentIndex + step) % focusableElements.length;

diff --git a/packages/component-base/test/focus-trap-controller.test.js b/packages/component-base/test/focus-trap-controller.test.js
@@ -21,7 +21,9 @@ customElements.define(
           <div id="trap">
             <input id="trap-input-1" />
             <input id="trap-input-2" />
-            <input id="trap-input-3" />
+            <div id="parent">
+              <input id="trap-input-3" />
+            </div>
           </div>
 
           <input id="outside-input-2" />
@@ -202,6 +204,51 @@ describe('focus-trap-controller', () => {
           ]);
         });
       });
+
+      describe('hidden elements', () => {
+        beforeEach(() => {
+          controller.trapFocus(trap);
+        });
+
+        it('should exclude elements hidden with display: none', async () => {
+          trapInput3.style.display = 'none';
+
+          const activeElement1 = await tab();
+          const activeElement2 = await tab();
+          const activeElement3 = await tab();
+          expect([activeElement1.id, activeElement2.id, activeElement3.id]).to.deep.equal([
+            'trap-input-2',
+            'trap-input-1',
+            'trap-input-2'
+          ]);
+        });
+
+        it('should exclude elements hidden with visibility: hidden', async () => {
+          trapInput3.style.visibility = 'hidden';
+
+          const activeElement1 = await tab();
+          const activeElement2 = await tab();
+          const activeElement3 = await tab();
+          expect([activeElement1.id, activeElement2.id, activeElement3.id]).to.deep.equal([
+            'trap-input-2',
+            'trap-input-1',
+            'trap-input-2'
+          ]);
+        });
+
+        it('should exclude elements whose parent node is not visible', async () => {
+          trapInput3.parentNode.style.display = 'none';
+
+          const activeElement1 = await tab();
+          const activeElement2 = await tab();
+          const activeElement3 = await tab();
+          expect([activeElement1.id, activeElement2.id, activeElement3.id]).to.deep.equal([
+            'trap-input-2',
+            'trap-input-1',
+            'trap-input-2'
+          ]);
+        });
+      });
     });
 
     describe('trap is deactivated', () => {