Skip to content

Commit

Permalink
Modal: Fix infinite focus loop (#649)
Browse files Browse the repository at this point in the history
The TrapFocusBehavior component listens to all document focus events. If
a target of a FocusEvent does not match any children of the trapped
component, the event is blocked and a new one is dispatched for the
first child of the trapped component. This works well for a single trap.

The infinite loop occurs when there is more than one TrapFocusBehaviour
trying to trap the focus for its first child. Calling .focus() for it
triggers a new FocusEvent, which is then captured by all Trap components,
calling .focus() again and filling all call stack up.

The solution is to check whether the event has been triggered by a trap
component or not. If it has, then let the event run its course. Since
the first child is actually the event target, and not the trap
component, Element.closest is used to identify if the target has a trap
ancestor.

In short,

* 'trap-focus' name added to identify TrapFocusBehavior components
* Element.closest() used to identify trapped components
* class component migrated to hook

Impact: both Modal and Sheet use TrapFocusBehavior.
  • Loading branch information
leoschmitz committed Mar 30, 2021
1 parent 1ab8fd0 commit 024b979
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 43 deletions.
8 changes: 6 additions & 2 deletions packages/gestalt/src/__snapshots__/Modal.jsdom.test.js.snap
Expand Up @@ -7,7 +7,9 @@ Object {
style="overflow: hidden;"
>
<div>
<div>
<div
name="trap-focus"
>
<div
aria-label="Test modal"
class="container"
Expand Down Expand Up @@ -37,7 +39,9 @@ Object {
</div>
</body>,
"container": <div>
<div>
<div
name="trap-focus"
>
<div
aria-label="Test modal"
class="container"
Expand Down
16 changes: 12 additions & 4 deletions packages/gestalt/src/__snapshots__/Sheet.jsdom.test.js.snap
Expand Up @@ -2,7 +2,9 @@

exports[`Sheet should render all props with nodes 1`] = `
<div>
<div>
<div
name="trap-focus"
>
<div
class="container"
>
Expand Down Expand Up @@ -94,7 +96,9 @@ exports[`Sheet should render all props with nodes 1`] = `

exports[`Sheet should render all props with render props 1`] = `
<div>
<div>
<div
name="trap-focus"
>
<div
class="container"
>
Expand Down Expand Up @@ -192,7 +196,9 @@ exports[`Sheet should render all props with render props 1`] = `

exports[`Sheet should render animation in 1`] = `
<div>
<div>
<div
name="trap-focus"
>
<div
class="container"
>
Expand Down Expand Up @@ -257,7 +263,9 @@ exports[`Sheet should render animation in 1`] = `

exports[`Sheet should render animation out 1`] = `
<div>
<div>
<div
name="trap-focus"
>
<div
class="container"
>
Expand Down
79 changes: 42 additions & 37 deletions packages/gestalt/src/behaviors/TrapFocusBehavior.js
@@ -1,5 +1,5 @@
// @flow strict
import React, { Component, type Node as ReactNode } from 'react';
import React, { useEffect, useRef, type Node as ReactNode } from 'react';

type Props = {|
children?: ReactNode,
Expand Down Expand Up @@ -32,48 +32,53 @@ const focusElement = (el: HTMLElement) => {
}
};

export default class TrapFocusBehavior extends Component<Props> {
el: ?HTMLDivElement;
export default function TrapFocusBehavior({ children }: Props): ReactNode {
const elRef = useRef<?HTMLDivElement>(null);
const previouslyFocusedElRef = useRef<?HTMLElement>(null);

previouslyFocusedEl: ?HTMLElement;

componentDidMount() {
this.previouslyFocusedEl = document.activeElement;
this.focusFirstChild();
document.addEventListener('focus', this.handleFocus, true);
}

componentWillUnmount() {
document.removeEventListener('focus', this.handleFocus, true);
if (this.previouslyFocusedEl) {
focusElement(this.previouslyFocusedEl);
}
}

setElRef: (el: ?HTMLDivElement) => void = (el: ?HTMLDivElement) => {
const setElRef: (el: ?HTMLDivElement) => void = (el: ?HTMLDivElement) => {
if (el) {
this.el = el;
elRef.current = el;
}
};

handleFocus: (event: FocusEvent) => void = (event: FocusEvent) => {
if (!this.el || (event.target instanceof Node && this.el.contains(event.target))) {
return;
}
useEffect(() => {
const { current: element } = elRef;
const focusFirstChild = () => {
if (element) {
focusElement(queryFocusableAll(element)[0]);
}
};

event.stopPropagation();
event.preventDefault();
this.focusFirstChild();
};
const handleFocus: (event: FocusEvent) => void = (event: FocusEvent) => {
if (!element || (event.target instanceof Node && element.contains(event.target))) {
return;
}

focusFirstChild() {
const { el } = this;
if (el) {
focusElement(queryFocusableAll(el)[0]);
}
}
if (event.target instanceof Element && event.target.closest('[name="trap-focus"]') !== null) {
return;
}

render(): ReactNode {
return <div ref={this.setElRef}>{this.props.children}</div>;
}
event.stopPropagation();
event.preventDefault();
focusFirstChild();
};

previouslyFocusedElRef.current = document.activeElement;
focusFirstChild();
document.addEventListener('focus', handleFocus, true);
return function cleanup() {
const { current: previouslyFocusedEl } = previouslyFocusedElRef;
document.removeEventListener('focus', handleFocus, true);
if (previouslyFocusedEl) {
focusElement(previouslyFocusedEl);
}
};
}, [elRef, previouslyFocusedElRef]);

return (
<div name="trap-focus" ref={setElRef}>
{children}
</div>
);
}

0 comments on commit 024b979

Please sign in to comment.