Skip to content

Commit bb51e8b

Browse files
mdodgelookermdodgelooker
authored
Focus Trap: fix return focus & Popover > Dialog > Select (#1157)
1 parent 624e27a commit bb51e8b

File tree

4 files changed

+107
-32
lines changed

4 files changed

+107
-32
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1414
### Fixed
1515

1616
- `DialogContent` with `borderBottom` prop CSS output error (no border, no flex: 8)
17+
- `Dialog` focus not returning to trigger when closed
18+
- `Select` not opening when rendered in a `Dialog` opened from a `Popover`
1719

1820
## [0.9.4] - 2020-06-29
1921

‎packages/components/src/Popover/Popover.tsx‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,8 +429,13 @@ export function usePopover({
429429
// need to manually disable any parent focus trap
430430
parentFocusTrapRef &&
431431
parentFocusTrapRef.current &&
432-
parentFocusTrapRef.current.deactivate({ returnFocus: false })
432+
parentFocusTrapRef.current.pause()
433433
}
434+
} else if (!focusTrap) {
435+
// need to manually re-enable any parent focus trap
436+
parentFocusTrapRef &&
437+
parentFocusTrapRef.current &&
438+
parentFocusTrapRef.current.unpause()
434439
}
435440
}, [focusTrap, parentFocusTrapRef, isOpen, enableFocusTrap])
436441

‎packages/components/src/utils/useFocusTrap.ts‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,29 +52,46 @@ export function useFocusTrap(
5252
const { value, setOn, setOff } = useToggle(enabled)
5353

5454
useEffect(() => {
55+
function removeTrap() {
56+
trapRef.current &&
57+
trapRef.current.deactivate({ returnFocus: checkFocusLost() })
58+
trapRef.current = undefined
59+
enableFocusTrap && enableFocusTrap()
60+
}
5561
if (element && value) {
5662
const autoFocusElement = element.querySelector(
5763
'[data-autofocus="true"]'
5864
) as HTMLElement
59-
trapRef.current = createFocusTrap(element, {
60-
clickOutsideDeactivates: true,
61-
escapeDeactivates: false,
62-
fallbackFocus: element,
63-
onDeactivate: () => setOff(),
64-
...(autoFocusElement ? { initialFocus: autoFocusElement } : {}),
65-
})
65+
if (trapRef.current) {
66+
trapRef.current.unpause()
67+
} else {
68+
trapRef.current = createFocusTrap(element, {
69+
clickOutsideDeactivates: true,
70+
escapeDeactivates: false,
71+
fallbackFocus: element,
72+
onDeactivate: () => setOff(),
73+
...(autoFocusElement ? { initialFocus: autoFocusElement } : {}),
74+
})
75+
}
6676
disableFocusTrap && disableFocusTrap()
6777
// Wait for any other focus trap to complete deactivation
6878
window.setTimeout(() => trapRef.current && trapRef.current.activate(), 0)
6979
} else {
70-
trapRef.current &&
71-
trapRef.current.deactivate({ returnFocus: checkFocusLost() })
72-
enableFocusTrap && enableFocusTrap()
80+
if (element) {
81+
// If element is defined but value is false it's because of either of the following
82+
// a) The focus trap is being superseded by another dialog or popover
83+
// b) The dialog is closing and the element will be removed shortly
84+
// Either way we wait till the element is removed to deactivate the trap and return focus to the trigger
85+
trapRef.current && trapRef.current.pause()
86+
} else {
87+
removeTrap()
88+
}
7389
}
7490

7591
return () => {
76-
trapRef.current &&
77-
trapRef.current.deactivate({ returnFocus: checkFocusLost() })
92+
if (!element || document.body.compareDocumentPosition(element) !== 20) {
93+
removeTrap()
94+
}
7895
}
7996
}, [value, element, disableFocusTrap, enableFocusTrap, setOff])
8097

‎storybook/src/Overlays/Popovers/Testing.stories.tsx‎

Lines changed: 70 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import {
3030
Button,
3131
ButtonOutline,
3232
ButtonTransparent,
33-
DialogManager,
33+
Dialog,
3434
FieldSelect,
3535
Heading,
3636
Menu,
@@ -41,6 +41,7 @@ import {
4141
Paragraph,
4242
Popover,
4343
PopoverContent,
44+
SpaceVertical,
4445
usePopover,
4546
useToggle,
4647
} from '@looker/components'
@@ -85,7 +86,7 @@ export const PopoverFocusTrap = () => {
8586
content={
8687
<PopoverContent p="large" width="360px">
8788
<Paragraph>
88-
Does tabbing focus only loop through these 3 buttons?
89+
Does tabbing focus only loop through these 3 buttons &amp; Select?
8990
</Paragraph>
9091
<Paragraph>
9192
Does clicking (or mousedown) each trigger an alert?
@@ -110,6 +111,9 @@ export const PopoverFocusTrap = () => {
110111
aria-label="Fruits"
111112
defaultValue="1"
112113
/>
114+
<Paragraph>
115+
Does it scroll here when the Select is closed?
116+
</Paragraph>
113117
<Paragraph>Long text</Paragraph>
114118
<Paragraph>Long text</Paragraph>
115119
<Paragraph>Long text</Paragraph>
@@ -128,37 +132,84 @@ export const PopoverFocusTrap = () => {
128132
>
129133
<Button mt="medium">Open Focus Trap Test Popover</Button>
130134
</Popover>
135+
<Paragraph>Does it scroll here when the Popover is closed?</Paragraph>
136+
<Paragraph>Long text</Paragraph>
137+
<Paragraph>Long text</Paragraph>
138+
<Paragraph>Long text</Paragraph>
139+
<Paragraph>Long text</Paragraph>
140+
<Paragraph>Long text</Paragraph>
141+
<Paragraph>Long text</Paragraph>
142+
<Paragraph>Long text</Paragraph>
143+
<Paragraph>Long text</Paragraph>
144+
<Paragraph>Long text</Paragraph>
145+
<Paragraph>Long text</Paragraph>
146+
<Paragraph>Long text</Paragraph>
147+
<Paragraph>Long text</Paragraph>
148+
<Paragraph>Long text</Paragraph>
149+
<Paragraph>Long text</Paragraph>
150+
<Paragraph>Long text</Paragraph>
151+
<Paragraph>Long text</Paragraph>
152+
<Paragraph>Long text</Paragraph>
153+
<Paragraph>Long text</Paragraph>
154+
<Paragraph>Long text</Paragraph>
155+
<Paragraph>Long text</Paragraph>
156+
<Paragraph>Long text</Paragraph>
157+
<Paragraph>Long text</Paragraph>
158+
<Paragraph>Long text</Paragraph>
159+
<Paragraph>Long text</Paragraph>
160+
<Paragraph>Long text</Paragraph>
161+
<Paragraph>Long text</Paragraph>
162+
<Paragraph>Long text</Paragraph>
163+
<Paragraph>Long text</Paragraph>
131164
</Box>
132165
)
133166
}
134167

135-
export const MenuOpenDialog = () => {
168+
export const OverlayOpenDialog = () => {
169+
const { value, setOn, setOff } = useToggle()
136170
function openAlert() {
137171
alert(`It's working!`)
138172
}
139173
return (
140-
<Box mt="large">
141-
<Heading>Menu Opening Dialog</Heading>
174+
<SpaceVertical mt="large" align="start">
175+
<Heading>Popover Opening a Dialog</Heading>
176+
<Popover
177+
content={
178+
<Button m="large" onClick={setOn}>
179+
Open Dialog
180+
</Button>
181+
}
182+
>
183+
<Button>Open Popover</Button>
184+
</Popover>
185+
<Dialog isOpen={value} onClose={setOff}>
186+
<DialogContent>
187+
<SpaceVertical align="start">
188+
<Paragraph>Try opening the Select and picking an option:</Paragraph>
189+
<FieldSelect
190+
label="Default Value"
191+
width={300}
192+
options={options}
193+
aria-label="Fruits"
194+
defaultValue="1"
195+
/>
196+
<Paragraph>Try clicking the button:</Paragraph>
197+
<Button onClick={openAlert}>Open Alert</Button>
198+
</SpaceVertical>
199+
</DialogContent>
200+
</Dialog>
201+
<Heading>Menu Opening a Dialog</Heading>
142202
<Menu>
143203
<MenuDisclosure tooltip="Select your favorite kind">
144204
<Button mr="small" mt="medium">
145205
Open Menu
146206
</Button>
147207
</MenuDisclosure>
148-
<DialogManager
149-
content={
150-
<DialogContent>
151-
<Paragraph>Some content inside the Dialog</Paragraph>
152-
<Button onClick={openAlert}>Open Alert</Button>
153-
</DialogContent>
154-
}
155-
>
156-
<MenuList>
157-
<MenuItem>Open Dialog</MenuItem>
158-
</MenuList>
159-
</DialogManager>
208+
<MenuList>
209+
<MenuItem onClick={setOn}>Open Dialog</MenuItem>
210+
</MenuList>
160211
</Menu>
161-
</Box>
212+
</SpaceVertical>
162213
)
163214
}
164215

@@ -177,7 +228,7 @@ export const RenderProps = () => (
177228

178229
export const RenderPropsSpread = () => (
179230
<Popover content={popoverContent}>
180-
{(...props) => <button {...props}>Test</button>}
231+
{(props) => <button {...props}>Test</button>}
181232
</Popover>
182233
)
183234

0 commit comments

Comments
 (0)