fix(modal, popover): fix focus-trap from preventing first click (#6769)

**Related Issue:** #6581 ## Summary - Uses the component's host element for the focus trap. This seems to fix the issue in the related PR. I guess we should always use the host element in order to not run into any issues. Previously, it was using an element inside the shadowRoot. - Sets up a mutation observer to update focusable elements when new elements are slotted - Remove setting tabIndex on an element as a fallback
Esri · Apr 12, 2023 · be4a63a · be4a63a
1 parent 198018c
commit be4a63a
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 42 deletions.
diff --git a/src/components/modal/modal.tsx b/src/components/modal/modal.tsx
@@ -190,6 +190,7 @@ export class Modal
     connectConditionalSlotComponent(this);
     connectLocalized(this);
     connectMessages(this);
+    connectFocusTrap(this);
   }
 
   disconnectedCallback(): void {
@@ -351,9 +352,9 @@ export class Modal
 
   modalContent: HTMLDivElement;
 
-  private mutationObserver: MutationObserver = createObserver("mutation", () => {
-    this.updateFooterVisibility();
-  });
+  private mutationObserver: MutationObserver = createObserver("mutation", () =>
+    this.handleMutationObserver()
+  );
 
   private cssVarObserver: MutationObserver = createObserver("mutation", () => {
     this.updateSizeCssVars();
@@ -367,8 +368,6 @@ export class Modal
 
   focusTrap: FocusTrap;
 
-  focusTrapEl: HTMLDivElement;
-
   closeButtonEl: HTMLButtonElement;
 
   contentId: string;
@@ -443,7 +442,7 @@ export class Modal
   @Method()
   async setFocus(): Promise<void> {
     await componentLoaded(this);
-    focusFirstTabbable(this.focusTrapEl);
+    focusFirstTabbable(this.el);
   }
 
   /**
@@ -480,8 +479,6 @@ export class Modal
 
   private setTransitionEl = (el: HTMLDivElement): void => {
     this.transitionEl = el;
-    this.focusTrapEl = el;
-    connectFocusTrap(this);
   };
 
   onBeforeOpen(): void {
@@ -560,6 +557,11 @@ export class Modal
     document.documentElement.classList.remove(CSS.overflowHidden);
   }
 
+  private handleMutationObserver = (): void => {
+    this.updateFooterVisibility();
+    this.updateFocusTrapElements();
+  };
+
   private updateFooterVisibility = (): void => {
     this.hasFooter = !!getSlotted(this.el, [SLOTS.back, SLOTS.primary, SLOTS.secondary]);
   };

diff --git a/src/components/popover/popover.tsx b/src/components/popover/popover.tsx
@@ -63,6 +63,7 @@ import {
   setComponentLoaded,
   setUpLoadableComponent
 } from "../../utils/loadable";
+import { createObserver } from "../../utils/observers";
 
 const manager = new PopoverManager();
 
@@ -255,6 +256,10 @@ export class Popover
   //
   // --------------------------------------------------------------------------
 
+  mutationObserver: MutationObserver = createObserver("mutation", () =>
+    this.updateFocusTrapElements()
+  );
+
   filteredFlipPlacements: EffectivePlacement[];
 
   @Element() el: HTMLCalcitePopoverElement;
@@ -284,8 +289,6 @@ export class Popover
 
   focusTrap: FocusTrap;
 
-  focusTrapEl: HTMLDivElement;
-
   // --------------------------------------------------------------------------
   //
   //  Lifecycle
@@ -298,6 +301,7 @@ export class Popover
     connectMessages(this);
     connectOpenCloseComponent(this);
     this.setUpReferenceElement(this.hasLoaded);
+    connectFocusTrap(this);
   }
 
   async componentWillLoad(): Promise<void> {
@@ -391,7 +395,7 @@ export class Popover
   async setFocus(): Promise<void> {
     await componentLoaded(this);
     forceUpdate(this.el);
-    focusFirstTabbable(this.focusTrapEl);
+    focusFirstTabbable(this.el);
   }
 
   /**
@@ -411,8 +415,6 @@ export class Popover
   private setTransitionEl = (el: HTMLDivElement): void => {
     this.transitionEl = el;
     connectOpenCloseComponent(this);
-    this.focusTrapEl = el;
-    connectFocusTrap(this);
   };
 
   setFilteredPlacements = (): void => {

diff --git a/src/utils/focusTrapComponent.spec.ts b/src/utils/focusTrapComponent.spec.ts
@@ -8,12 +8,11 @@ import {
 describe("focusTrapComponent", () => {
   it("focusTrapComponent lifecycle", () => {
     const fakeComponent = {} as any;
-    fakeComponent.focusTrapEl = document.createElement("div");
+    fakeComponent.el = document.createElement("div");
 
     connectFocusTrap(fakeComponent);
 
-    expect(fakeComponent.focusTrapEl.tabIndex).toBe(-1);
-    expect(fakeComponent.focusTrap).toBeDefined();
+    expect(fakeComponent.el).toBeDefined();
     expect(fakeComponent.focusTrap.active).toBe(false);
 
     const activateSpy = jest.fn();
@@ -34,14 +33,4 @@ describe("focusTrapComponent", () => {
     deactivateFocusTrap(fakeComponent);
     expect(deactivateSpy).toHaveBeenCalledTimes(1);
   });
-
-  it("focusTrapEl with tabIndex`", () => {
-    const fakeComponent = {} as any;
-    fakeComponent.focusTrapEl = document.createElement("div");
-    fakeComponent.focusTrapEl.tabIndex = 0;
-
-    connectFocusTrap(fakeComponent);
-    expect(fakeComponent.focusTrapEl.tabIndex).toBe(0);
-    expect(fakeComponent.focusTrap).toBeDefined();
-  });
 });
diff --git a/src/utils/focusTrapComponent.ts b/src/utils/focusTrapComponent.ts
@@ -4,9 +4,14 @@ import { FocusableElement, focusElement, tabbableOptions } from "./dom";
 const trapStack: _FocusTrap[] = [];
 
 /**
- * Defines interface for components with a focus trap.
+ * Defines interface for components with a focus trap. Focusable content is required for components implementing focus trapping with this interface.
  */
 export interface FocusTrapComponent {
+  /**
+   * The focus trap element.
+   */
+  el: HTMLElement;
+
   /**
    * When `true`, prevents focus trapping.
    */
@@ -17,11 +22,6 @@ export interface FocusTrapComponent {
    */
   focusTrap: FocusTrap;
 
-  /**
-   * The focus trap element.
-   */
-  focusTrapEl: HTMLElement;
-
   /**
    * Method to update the element(s) that are used within the FocusTrap component.
    */
@@ -36,21 +36,17 @@ export type FocusTrap = _FocusTrap;
  * @param {FocusTrapComponent} component The FocusTrap component.
  */
 export function connectFocusTrap(component: FocusTrapComponent): void {
-  const { focusTrapEl } = component;
+  const { el } = component;
 
-  if (!focusTrapEl) {
+  if (!el) {
     return;
   }
 
-  if (focusTrapEl.tabIndex == null) {
-    focusTrapEl.tabIndex = -1;
-  }
-
   const focusTrapOptions: FocusTrapOptions = {
     clickOutsideDeactivates: true,
-    document: focusTrapEl.ownerDocument,
+    document: el.ownerDocument,
     escapeDeactivates: false,
-    fallbackFocus: focusTrapEl,
+    fallbackFocus: el,
     setReturnFocus: (el) => {
       focusElement(el as FocusableElement);
       return false;
@@ -59,7 +55,7 @@ export function connectFocusTrap(component: FocusTrapComponent): void {
     trapStack
   };
 
-  component.focusTrap = createFocusTrap(focusTrapEl, focusTrapOptions);
+  component.focusTrap = createFocusTrap(el, focusTrapOptions);
 }
 
 /**
@@ -95,5 +91,5 @@ export function deactivateFocusTrap(component: FocusTrapComponent): void {
  * requestAnimationFrame(() => input.setFocus());
  */
 export function updateFocusTrapElements(component: FocusTrapComponent): void {
-  component.focusTrap?.updateContainerElements(component.focusTrapEl);
+  component.focusTrap?.updateContainerElements(component.el);
 }