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

michael-yx-wu · 2021-09-09T20:28:12Z

Fixes #3445

Checklist

Includes tests
Update documentation (n/a)

Changes proposed in this pull request:

When enforceFocus is enabled, surround Overlay content with dummy div
elements with focus event listeners that keep focus inside the Overlay.

Reviewers should focus on:

I want to flag that I've removed the tabindex attribute from the backdrop to prevent it from being keyboard focusable.

This change also fixes an issue where multiple open overlays will compete with each for focus. As an example, go to https://blueprintjs.com/docs/#core/components/overlay and open the overlay and then hit shift+s to open the omnibar search (also powered by Overlay). The search input will not get autofocused. Repeat the same workflow on https://46971-71939872-gh.circle-artifacts.com/0/packages/docs-app/dist/index.html#core/components/overlay and note that the omnibar search does get focus correctly.

Screenshot

Before

When using a portal, the portal is the last element on the page so tabbing will eventually cause focus to leave the document and none of the existing event handlers run before that happens. Note that shift+tabbing is not closing the overlay, the gif is just looping.

After

Tab/shift+tab will never allow focus to leave the overlay if enforceFocus.

When enforceFocus is enabled, surround Overlay content with dummy div elements with focus event listeners that keep focus inside the Overlay.

michael-yx-wu · 2021-09-09T23:26:36Z

packages/core/src/components/overlay/overlay.tsx

-    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();


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

michael-yx-wu · 2021-09-09T23:28:06Z

packages/core/src/components/overlay/overlay.tsx

@@ -415,7 +501,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();


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

packages/core/test/multistep-dialog/multistepDialogTests.tsx

michael-yx-wu · 2021-09-09T23:52:36Z

packages/core/test/overlay/overlayTests.tsx

+    after(() => {
+        document.documentElement.removeChild(testsContainerElement);
+    });


not really important, but seems good to clean up?

blueprint-bot · 2021-09-09T23:54:09Z

Fix tests by unmounting properly

Previews: documentation | landing | table

michael-yx-wu · 2021-09-10T00:14:11Z

packages/core/src/components/overlay/overlay.tsx

@@ -319,11 +335,11 @@ 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;


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

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

Filed #4908

adidahiya

overall looks pretty good!

packages/core/src/components/overlay/overlay.tsx

This allows bringFocusInsideOverlay to work even when there are no keyboard-focusable elements.

michael-yx-wu · 2021-09-14T17:55:58Z

I found a bug where this doesn't properly enforce focus if focus is coming back to the document from another part of the browser (e.g. the URL or bookmarks bar). I will restore the focus event handler logic, but I will listen instead for focusin instead of focus since the latter isn't supposed to bubble

update: as suggested, I'll address this in a flup

blueprint-bot · 2021-09-14T20:08:11Z

Fix selector

Previews: documentation | landing | table

adidahiya

nice 👍

ro-savage · 2021-09-21T01:17:06Z

@michael-yx-wu - The appears to be a bug or unintentional behaviour with this change on dialogs with scrolls.

See: #4920

michael-yx-wu force-pushed the mw/dialog-enforceFocus branch from eb5846f to 4e74ba7 Compare September 9, 2021 20:37

palantir deleted a comment from blueprint-bot Sep 9, 2021

Use focus traps to fix Overlay enforceFocus

a8b0b4b

When enforceFocus is enabled, surround Overlay content with dummy div elements with focus event listeners that keep focus inside the Overlay.

palantir deleted a comment from blueprint-bot Sep 9, 2021

michael-yx-wu force-pushed the mw/dialog-enforceFocus branch from 4e74ba7 to a8b0b4b Compare September 9, 2021 20:50

This comment has been minimized.

Sign in to view

Make it work without portals

07a6b30

This comment has been minimized.

Sign in to view

Fix tests

2ec903d

michael-yx-wu commented Sep 9, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

michael-yx-wu commented Sep 9, 2021

View reviewed changes

packages/core/test/multistep-dialog/multistepDialogTests.tsx Outdated Show resolved Hide resolved

Fix tests by unmounting properly

4fe0b21

michael-yx-wu commented Sep 9, 2021

View reviewed changes

michael-yx-wu marked this pull request as ready for review September 9, 2021 23:54

michael-yx-wu requested a review from adidahiya September 9, 2021 23:54

michael-yx-wu commented Sep 10, 2021

View reviewed changes

adidahiya reviewed Sep 13, 2021

View reviewed changes

packages/core/src/components/overlay/overlay.tsx Show resolved Hide resolved

packages/core/src/components/overlay/overlay.tsx Outdated Show resolved Hide resolved

packages/core/src/components/overlay/overlay.tsx Show resolved Hide resolved

michael-yx-wu added 3 commits September 14, 2021 13:18

Add focus trap element comments

ea6b246

Ignore tabindex="-1" elements

6f15212

Fallback to focus on startFocusTrapElement

6f011ac

This allows bringFocusInsideOverlay to work even when there are no keyboard-focusable elements.

This comment has been minimized.

Sign in to view

Fix selector

2243a19

adidahiya changed the title ~~Use focus traps to fix Overlay enforceFocus~~ [core] fix(Overlay): use focus traps to fix enforceFocus behavior Sep 14, 2021

adidahiya approved these changes Sep 14, 2021

View reviewed changes

adidahiya changed the title ~~[core] fix(Overlay): use focus traps to fix enforceFocus behavior~~ [core] fix(Overlay): use focus traps to fix enforceFocus Sep 14, 2021

adidahiya merged commit 9a4debd into develop Sep 14, 2021

adidahiya deleted the mw/dialog-enforceFocus branch September 14, 2021 21:52

This was referenced Sep 15, 2021

[core] fix(Overlay): capture focus when it returns to document #4907

Merged

Overlay cannot focus autofocus elements due to React behavior #4908

Open

ro-savage mentioned this pull request Sep 21, 2021

New EnforceFocus on overlay causes dialogs to jump #4920

Closed

adidahiya mentioned this pull request Oct 14, 2021

[core] fix(Overlay): improve focus management #4963

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michael-yx-wu commented Sep 9, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

michael-yx-wu Sep 9, 2021 •

edited

Loading

This comment has been minimized.

michael-yx-wu Sep 9, 2021

michael-yx-wu Sep 9, 2021

blueprint-bot commented Sep 9, 2021

michael-yx-wu Sep 10, 2021 •

edited

Loading

adidahiya Sep 14, 2021

michael-yx-wu Sep 15, 2021

adidahiya left a comment

michael-yx-wu commented Sep 14, 2021 •

edited

Loading

This comment has been minimized.

blueprint-bot commented Sep 14, 2021

adidahiya left a comment

ro-savage commented Sep 21, 2021

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

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

Conversation

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

Fixes #3445

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

Before

After

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

This comment has been minimized.

michael-yx-wu Sep 9, 2021

Choose a reason for hiding this comment

michael-yx-wu Sep 9, 2021

Choose a reason for hiding this comment

blueprint-bot commented Sep 9, 2021

Fix tests by unmounting properly

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

Choose a reason for hiding this comment

adidahiya Sep 14, 2021

Choose a reason for hiding this comment

michael-yx-wu Sep 15, 2021

Choose a reason for hiding this comment

adidahiya left a comment

Choose a reason for hiding this comment

michael-yx-wu commented Sep 14, 2021 • edited Loading

This comment has been minimized.

blueprint-bot commented Sep 14, 2021

Fix selector

adidahiya left a comment

Choose a reason for hiding this comment

ro-savage commented Sep 21, 2021

michael-yx-wu commented Sep 9, 2021 •

edited

Loading

michael-yx-wu Sep 9, 2021 •

edited

Loading

michael-yx-wu Sep 10, 2021 •

edited

Loading

michael-yx-wu commented Sep 14, 2021 •

edited

Loading