From be4a63a661b2d7ac698649acf0e7769ea39c0694 Mon Sep 17 00:00:00 2001 From: Matt Driscoll Date: Wed, 12 Apr 2023 13:37:07 -0700 Subject: [PATCH] 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 --- src/components/modal/modal.tsx | 18 ++++++++++-------- src/components/popover/popover.tsx | 12 +++++++----- src/utils/focusTrapComponent.spec.ts | 15 ++------------- src/utils/focusTrapComponent.ts | 28 ++++++++++++---------------- 4 files changed, 31 insertions(+), 42 deletions(-) diff --git a/src/components/modal/modal.tsx b/src/components/modal/modal.tsx index 4f6ba02e29c..f285709cfb6 100644 --- 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 { 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 index d74811677c4..b5f47fa37bc 100644 --- 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 { @@ -391,7 +395,7 @@ export class Popover async setFocus(): Promise { 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 index d9f6fe9f826..267962c33c2 100644 --- 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 index 841ddd28a78..200bf94253d 100644 --- 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); }