Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[core] fix(Overlay): use focus traps to fix enforceFocus #4894

Merged
merged 8 commits into from
Sep 14, 2021
Merged

[core] fix(Overlay): use focus traps to fix enforceFocus #4894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a8b0b4b
Use focus traps to fix Overlay enforceFocus
michael-yx-wu Sep 9, 2021
07a6b30
Make it work without portals
michael-yx-wu Sep 9, 2021
2ec903d
Fix tests
michael-yx-wu Sep 9, 2021
4fe0b21
Fix tests by unmounting properly
michael-yx-wu Sep 9, 2021
ea6b246
Add focus trap element comments
michael-yx-wu Sep 14, 2021
6f15212
Ignore tabindex="-1" elements
michael-yx-wu Sep 14, 2021
6f011ac
Fallback to focus on startFocusTrapElement
michael-yx-wu Sep 14, 2021
2243a19
Fix selector
michael-yx-wu Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 123 additions & 38 deletions packages/core/src/components/overlay/overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,24 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
// an HTMLElement that contains the backdrop and any children, to query for focus target
public containerElement: HTMLElement | null = null;

// An empty, keyboard-focusable div at the beginning of the Overlay content
private startFocusTrapElement: HTMLDivElement | null = null;
michael-yx-wu marked this conversation as resolved.
Show resolved Hide resolved

// An empty, keyboard-focusable div at the end of the Overlay content
private endFocusTrapElement: HTMLDivElement | null = null;

private refHandlers = {
// HACKHACK: see https://github.com/palantir/blueprint/issues/3979
/* eslint-disable-next-line react/no-find-dom-node */
container: (ref: TransitionGroup) => (this.containerElement = findDOMNode(ref) as HTMLElement),
firstFocusable: (ref: HTMLDivElement | null) => {
this.startFocusTrapElement = ref;
ref?.addEventListener("focusin", this.handleStartFocusTrapElementFocusIn);
},
lastFocusable: (ref: HTMLDivElement | null) => {
this.endFocusTrapElement = ref;
ref?.addEventListener("focusin", this.handleEndFocusTrapElementFocusIn);
},
};

public render() {
Expand All @@ -241,7 +255,7 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
return null;
}

const { children, className, usePortal, isOpen } = this.props;
const { children, className, enforceFocus, usePortal, isOpen } = this.props;

// TransitionGroup types require single array of children; does not support nested arrays.
// So we must collapse backdrop and children into one array, and every item must be wrapped in a
Expand All @@ -252,6 +266,10 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
if (maybeBackdrop !== null) {
childrenWithTransitions.unshift(maybeBackdrop);
}
if (isOpen && enforceFocus && childrenWithTransitions.length > 0) {
childrenWithTransitions.unshift(this.renderDummyElement(this.refHandlers.firstFocusable, "__first"));
childrenWithTransitions.push(this.renderDummyElement(this.refHandlers.lastFocusable, "__last"));
}

const containerClasses = classNames(
Classes.OVERLAY,
Expand Down Expand Up @@ -319,11 +337,13 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
if (isFocusOutsideModal) {
// element marked autofocus has higher priority than the other clowns
const autofocusElement = this.containerElement.querySelector("[autofocus]") as HTMLElement;
Copy link
Contributor Author

@michael-yx-wu michael-yx-wu Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that InputGroup's autoFocus prop is passed to the input element as is. React doesn't actually render the autofocus attribute to the DOM (see facebook/react#11851 (comment)), so this querySelector is unlikely to work unless the consumer explicitly sets the DOM autofocus (no caps) attribute instead. Not sure if we want to do anything about this though, since React will warn if you set the DOM attribute directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should do something about this in a FLUP. not a high priority though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #4908

const wrapperElement = this.containerElement.querySelector("[tabindex]") as HTMLElement;
const firstKeyboardFocusableElement = this.getKeyboardFocusableElements().shift();
if (autofocusElement != null) {
autofocusElement.focus();
} else if (wrapperElement != null) {
wrapperElement.focus();
} else if (firstKeyboardFocusableElement != null) {
firstKeyboardFocusableElement.focus();
} else {
this.startFocusTrapElement?.focus();
}
}
});
Expand Down Expand Up @@ -395,7 +415,6 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
{...backdropProps}
className={classNames(Classes.OVERLAY_BACKDROP, backdropClassName, backdropProps?.className)}
onMouseDown={this.handleBackdropMouseDown}
tabIndex={this.props.canOutsideClickClose ? 0 : undefined}
/>
</CSSTransition>
);
Expand All @@ -404,9 +423,94 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
}
}

private renderDummyElement(ref: (element: HTMLDivElement) => void, key: string) {
const { transitionDuration, transitionName } = this.props;
return (
<CSSTransition
classNames={transitionName}
key={key}
addEndListener={this.handleTransitionAddEnd}
timeout={transitionDuration}
unmountOnExit={true}
>
<div ref={ref} tabIndex={0} />
</CSSTransition>
);
}

/**
* Ensures repeatedly pressing shift+tab keeps focus inside the Overlay. Moves focus to
* the `endFocusTrapElement` or the first keyboard-focusable element in the Overlay (excluding
* the `startFocusTrapElement`), depending on whether the element losing focus is inside the
* Overlay.
*/
private handleStartFocusTrapElementFocusIn = (e: FocusEvent) => {
e.preventDefault();
e.stopImmediatePropagation();
if (
e.relatedTarget != null &&
this.containerElement!.contains(e.relatedTarget as Element) &&
e.relatedTarget !== this.endFocusTrapElement
) {
this.endFocusTrapElement?.focus();
} else {
this.getKeyboardFocusableElements().shift()?.focus();
}
};

/**
* Ensures repeatedly pressing tab keeps focus inside the Overlay. Moves focus to the
* `startFocusTrapElement` or the last keyboard-focusable element in the Overlay (excluding the
* `startFocusTrapElement`), depending on whether the element losing focus is inside the
* Overlay.
*/
private handleEndFocusTrapElementFocusIn = (e: FocusEvent) => {
e.preventDefault();
e.stopImmediatePropagation();
if (
e.relatedTarget != null &&
this.containerElement!.contains(e.relatedTarget as Element) &&
e.relatedTarget !== this.startFocusTrapElement
) {
this.startFocusTrapElement?.focus();
} else {
this.getKeyboardFocusableElements().pop()?.focus();
}
};

private getKeyboardFocusableElements() {
const focusableElements: HTMLElement[] =
this.containerElement !== null
? Array.from(
// Order may not be correct if children elements use tabindex values > 0.
// Selectors derived from this SO question:
// https://stackoverflow.com/questions/1599660/which-html-elements-can-receive-focus
this.containerElement.querySelectorAll(
[
adidahiya marked this conversation as resolved.
Show resolved Hide resolved
'a[href]:not([tabindex="-1"])',
'button:not([disabled]):not([tabindex="-1"])',
'details:not([tabindex="-1"])',
'input:not([disabled]):not([tabindex="-1"])',
'select:not([disabled]):not([tabindex="-1"])',
'textarea:not([disabled]):not([tabindex="-1"])',
'[tabindex]:not([tabindex="-1"])',
].join(","),
),
)
: [];
if (this.props.enforceFocus) {
// The first and last elements are dummy elements that help trap focus when enforceFocus
// is enabled
focusableElements.shift();
focusableElements.pop();
}
return focusableElements;
}

private overlayWillClose() {
document.removeEventListener("focus", this.handleDocumentFocus, /* useCapture */ true);
document.removeEventListener("mousedown", this.handleDocumentClick);
this.startFocusTrapElement?.removeEventListener("focusin", this.handleStartFocusTrapElementFocusIn);
this.endFocusTrapElement?.removeEventListener("focusin", this.handleEndFocusTrapElementFocusIn);

const { openStack } = Overlay;
const stackIndex = openStack.indexOf(this);
Expand All @@ -415,7 +519,7 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
if (openStack.length > 0) {
const lastOpenedOverlay = Overlay.getLastOpened();
if (lastOpenedOverlay.props.enforceFocus) {
document.addEventListener("focus", lastOpenedOverlay.handleDocumentFocus, /* useCapture */ true);
lastOpenedOverlay.bringFocusInsideOverlay();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this handler was a roundabout way to bring focus back to the last opened overlay. we can just call bringFocusInsideOverlay directly.

}
}

Expand All @@ -426,20 +530,13 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
}

private overlayWillOpen() {
const { openStack } = Overlay;
if (openStack.length > 0) {
document.removeEventListener("focus", Overlay.getLastOpened().handleDocumentFocus, /* useCapture */ true);
}
openStack.push(this);
Overlay.openStack.push(this);

if (this.props.autoFocus) {
this.bringFocusInsideOverlay();
}
if (this.props.enforceFocus) {
document.addEventListener("focus", this.handleDocumentFocus, /* useCapture */ true);
}

if (this.props.canOutsideClickClose && !this.props.hasBackdrop) {
if (this.props.canOutsideClickClose || this.props.enforceFocus) {
document.addEventListener("mousedown", this.handleDocumentClick);
}

Expand All @@ -455,15 +552,14 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
onClose?.(e);
}
if (enforceFocus) {
// make sure document.activeElement is updated before bringing the focus back
this.bringFocusInsideOverlay();
}
backdropProps?.onMouseDown?.(e);
};

private handleDocumentClick = (e: MouseEvent) => {
const { canOutsideClickClose, isOpen, onClose } = this.props;
// get the actually target even if we are in an open mode Shadow DOM
const { canOutsideClickClose, enforceFocus, isOpen, onClose } = this.props;
// get the actual target even if we are in an open mode Shadow DOM
const eventTarget = (e.composed ? e.composedPath()[0] : e.target) as HTMLElement;

const stackIndex = Overlay.openStack.indexOf(this);
Expand All @@ -475,25 +571,14 @@ export class Overlay extends AbstractPureComponent2<OverlayProps, IOverlayState>
return elem && elem.contains(eventTarget) && !elem.isSameNode(eventTarget);
});

if (isOpen && canOutsideClickClose && !isClickInThisOverlayOrDescendant) {
// casting to any because this is a native event
onClose?.(e as any);
}
};

private handleDocumentFocus = (e: FocusEvent) => {
// get the actually target even if we are in an open mode Shadow DOM
const eventTarget = e.composed ? e.composedPath()[0] : e.target;
if (
this.props.enforceFocus &&
this.containerElement != null &&
eventTarget instanceof Node &&
!this.containerElement.contains(eventTarget as HTMLElement)
) {
// prevent default focus behavior (sometimes auto-scrolls the page)
e.preventDefault();
e.stopImmediatePropagation();
this.bringFocusInsideOverlay();
Comment on lines -484 to -496
Copy link
Contributor Author

@michael-yx-wu michael-yx-wu Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when enforceFocus=true, focus will start on the overlay and will only leave if another overlay with autoFocus or enforceFocus is opened. because we take care to focus the last opened overlay when closing the current overlay and we call bringFocusInsideOverlay in the mousedown event handler, we don't need this focus event handler anymore

if (isOpen && !isClickInThisOverlayOrDescendant) {
if (canOutsideClickClose) {
// casting to any because this is a native event
onClose?.(e as any);
}
if (enforceFocus) {
this.bringFocusInsideOverlay();
}
}
};

Expand Down
14 changes: 9 additions & 5 deletions packages/core/test/multistep-dialog/multistepDialogTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,24 @@ describe("<MultistepDialog>", () => {
});

it("gets by without children", () => {
assert.doesNotThrow(() => mount(<MultistepDialog isOpen={true} />));
assert.doesNotThrow(() => {
const dialog = mount(<MultistepDialog isOpen={true} />);
dialog.unmount();
});
});

it("supports non-existent children", () => {
assert.doesNotThrow(() =>
mount(
assert.doesNotThrow(() => {
const dialog = mount(
<MultistepDialog>
{null}
<DialogStep id="one" panel={<Panel />} />
{undefined}
<DialogStep id="two" panel={<Panel />} />
</MultistepDialog>,
),
);
);
dialog.unmount();
});
});

it("enables next by default", () => {
Expand Down
36 changes: 28 additions & 8 deletions packages/core/test/overlay/overlayTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ describe("<Overlay>", () => {
}
});

after(() => {
document.documentElement.removeChild(testsContainerElement);
});
Comment on lines +63 to +65
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really important, but seems good to clean up?


it("renders its content correctly", () => {
const overlay = shallow(
<Overlay isOpen={true} usePortal={false}>
Expand Down Expand Up @@ -247,16 +251,16 @@ describe("<Overlay>", () => {
</Overlay>,
);
assertFocus(() => {
const backdrops = Array.from(document.querySelectorAll("." + Classes.OVERLAY_BACKDROP));
assert.include(backdrops, document.activeElement);
const contents = Array.from(document.querySelectorAll("." + Classes.OVERLAY_CONTENT));
assert.include(contents, document.activeElement);
}, done);
});

it("does not bring focus to overlay if autoFocus=false", done => {
it("does not bring focus to overlay if autoFocus=false and enforceFocus=false", done => {
mountWrapper(
<div>
<button>something outside overlay for browser to focus on</button>
<Overlay autoFocus={false} isOpen={true} usePortal={true}>
<Overlay autoFocus={false} enforceFocus={false} isOpen={true} usePortal={true}>
<input type="text" />
</Overlay>
</div>,
Expand Down Expand Up @@ -290,10 +294,7 @@ describe("<Overlay>", () => {
buttonRef!.focus();
assertFocus(() => {
assert.notStrictEqual(document.activeElement, buttonRef);
assert.isTrue(
document.activeElement?.classList.contains(Classes.OVERLAY_BACKDROP),
"focus on backdrop",
);
assert.isTrue(document.activeElement?.classList.contains(Classes.OVERLAY_CONTENT), "focus on content");
}, done);
});

Expand All @@ -307,6 +308,25 @@ describe("<Overlay>", () => {
assertFocus(`strong.${Classes.OVERLAY_CONTENT}`, done);
});

it("returns focus to overlay after clicking an outside element if enforceFocus=true", done => {
mountWrapper(
<div>
<Overlay
enforceFocus={true}
canOutsideClickClose={false}
isOpen={true}
usePortal={false}
hasBackdrop={false}
>
{createOverlayContents()}
</Overlay>
<button id="buttonId" />
</div>,
);
wrapper.find("#buttonId").simulate("click");
assertFocus(`strong.${Classes.OVERLAY_CONTENT}`, done);
});

it("does not result in maximum call stack if two overlays open with enforceFocus=true", () => {
const anotherContainer = document.createElement("div");
document.documentElement.appendChild(anotherContainer);
Expand Down