Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

Commit

Permalink
fix(Popup|Dialog): ESC should close opened Popup/Dialog if focus is l…
Browse files Browse the repository at this point in the history
…ost and moved to body (#1807)

* fix: ESC should close opened Popup/Dialog if focus is lost and moved to body

* Add e2e test for popup

* fix for nested Dialogs

* add test for nested dialogs

* add test case fo dialog in popup

* update changelog

* improvements after CR comments

* Update packages/react/src/components/Dialog/Dialog.tsx

* small improvement to trigger build
  • Loading branch information
sophieH29 authored Aug 19, 2019
1 parent 11e0ba4 commit a26fba7
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 7 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix bidirectional `FocusZone` to land focus correctly on DOWN key press after series of UP arrow keys @sophieH29 ([#1794](https://github.com/stardust-ui/react/pull/1794))
- Fix `hand` icon in Teams theme @lucivpav ([#1782](https://github.com/stardust-ui/react/pull/1782))
- Use `target` from `Provider` in `ReactDOM.createPortal()` calls @layershifter ([#1810](https://github.com/stardust-ui/react/pull/1810))
- ESC key should close the last opened `Popup` or `Dialog` if body has focus @sophieH29 ([#1807](https://github.com/stardust-ui/react/pull/1807))

### Features
- Add `overwrite` prop to `Provider` @layershifter ([#1780](https://github.com/stardust-ui/react/pull/1780))
Expand Down
27 changes: 27 additions & 0 deletions e2e/tests/dialogInDialog-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,31 @@ describe('Dialog in Dialog', () => {
expect(await e2e.exists(outerHeader)).toBe(false)
expect(await e2e.exists(innerHeader)).toBe(false)
})

it('A click on content and pressing ESC button should close the last opened dialog', async () => {
await e2e.clickOn(outerTrigger) // opens dialog
expect(await e2e.exists(outerHeader)).toBe(true)

await e2e.clickOn(innerTrigger) // opens nested dialog
expect(await e2e.exists(innerHeader)).toBe(true)

await e2e.clickOn(innerHeader)

// check that focus moved to body after clicking on Dialog content
expect(await e2e.isFocused('body')).toBe(true)

// press ESC and check if nested dialog is closed and focus is on nested trigger
await e2e.pressKey('Escape')
expect(await e2e.exists(innerHeader)).toBe(false)
expect(await e2e.isFocused(innerTrigger)).toBe(true)

// click on dialog content to move focus to body
await e2e.clickOn(outerHeader)
expect(await e2e.isFocused('body')).toBe(true)

// press ESC again and check if the last dialog is closed and focus is on trigger
await e2e.pressKey('Escape')
expect(await e2e.exists(outerHeader)).toBe(false)
expect(await e2e.isFocused(outerTrigger)).toBe(true)
})
})
15 changes: 9 additions & 6 deletions e2e/tests/dialogInPopup-example.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,21 @@ export const selectors = {
dialogHeader: Dialog.slotClassNames.header,
dialogOverlay: Dialog.slotClassNames.overlay,
dialogTrigger: 'dialog-trigger',
popupContent: Popup.Content.className,
popupContent: 'popup-content',
popupTrigger: 'popup-trigger',
}

const DialogInPopupExample = () => (
<Popup
content={
<Dialog
cancelButton={{ content: 'Close', id: selectors.dialogCancel }}
header="A dialog"
trigger={<Button id={selectors.dialogTrigger} content="Open a dialog" />}
/>
<>
<div id={selectors.popupContent}>Popup content</div>
<Dialog
cancelButton={{ content: 'Close', id: selectors.dialogCancel }}
header="A dialog"
trigger={<Button id={selectors.dialogTrigger} content="Open a dialog" />}
/>
</>
}
trigger={<Button id={selectors.popupTrigger} content="Open a popup" />}
/>
Expand Down
29 changes: 28 additions & 1 deletion e2e/tests/dialogInPopup-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const dialogCancel = `#${selectors.dialogCancel}`
const dialogHeader = `.${selectors.dialogHeader}`
const dialogOverlay = `.${selectors.dialogOverlay}`
const dialogTrigger = `#${selectors.dialogTrigger}`
const popupContent = `.${selectors.popupContent}`
const popupContent = `#${selectors.popupContent}`
const popupTrigger = `#${selectors.popupTrigger}`

// https://github.com/stardust-ui/react/issues/1674
Expand Down Expand Up @@ -48,4 +48,31 @@ describe('Dialog in Popup', () => {
expect(await e2e.exists(popupContent)).toBe(true)
expect(await e2e.exists(dialogHeader)).toBe(false)
})

it('A click on content and pressing ESC button should close the first opened dialog', async () => {
await e2e.clickOn(popupTrigger) // opens popup
await e2e.clickOn(dialogTrigger) // opens dialog

expect(await e2e.exists(popupContent)).toBe(true)
expect(await e2e.exists(dialogHeader)).toBe(true)

await e2e.clickOn(dialogHeader)

// check that focus moved to body after clicking on Dialog content
expect(await e2e.isFocused('body')).toBe(true)

// press ESC and check if nested popup is closed and focus is on nested trigger
await e2e.pressKey('Escape')
expect(await e2e.exists(dialogHeader)).toBe(false)
expect(await e2e.isFocused(dialogTrigger)).toBe(true)

// click on popup content to move focus to body
await e2e.clickOn(popupContent)
expect(await e2e.isFocused('body')).toBe(true)

// press ESC again and check if the last popup is closed and focus is on trigger
await e2e.pressKey('Escape')
expect(await e2e.exists(popupContent)).toBe(false)
expect(await e2e.isFocused(popupTrigger)).toBe(true)
})
})
26 changes: 26 additions & 0 deletions e2e/tests/popupInPopup-example.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import * as React from 'react'
import { Button, Popup } from '@stardust-ui/react'

export const selectors = {
popupContentId: 'popup-content-id',
popupTriggerId: 'popup-trigger-id',
popupContentNestedId: 'popup-content-nested-id',
popupTriggerNestedId: 'popup-trigger-nested-id',
}

const PopupInPopupExample = () => (
<Popup
content={
<>
<div id={selectors.popupContentId}>content</div>
<Popup
content={<div id={selectors.popupContentNestedId}>nested content</div>}
trigger={<Button id={selectors.popupTriggerNestedId} content="Open a nested popup" />}
/>
</>
}
trigger={<Button id={selectors.popupTriggerId} content="Open a popup" />}
/>
)

export default PopupInPopupExample
39 changes: 39 additions & 0 deletions e2e/tests/popupInPopup-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { selectors } from './popupInPopup-example'

const popupTrigger = `#${selectors.popupTriggerId}`
const popupContent = `#${selectors.popupContentId}`
const popupTriggerNested = `#${selectors.popupTriggerNestedId}`
const popupContentNested = `#${selectors.popupContentNestedId}`

describe('Popup in Popup', () => {
beforeEach(async () => {
await e2e.gotoTestCase(__filename, popupTrigger)
})

it('A click on content and pressing ESC button should close the last opened popup', async () => {
await e2e.clickOn(popupTrigger) // opens popup
expect(await e2e.exists(popupContent)).toBe(true)

await e2e.clickOn(popupTriggerNested) // opens nested popup
expect(await e2e.exists(popupContentNested)).toBe(true)

await e2e.clickOn(popupContentNested)

// check that focus moved to body after clicking on Popup content
expect(await e2e.isFocused('body')).toBe(true)

// press ESC and check if nested popup is closed and focus is on nested trigger
await e2e.pressKey('Escape')
expect(await e2e.exists(popupContentNested)).toBe(false)
expect(await e2e.isFocused(popupTriggerNested)).toBe(true)

// click on popup content to move focus to body
await e2e.clickOn(popupContent)
expect(await e2e.isFocused('body')).toBe(true)

// press ESC again and check if the last popup is closed and focus is on trigger
await e2e.pressKey('Escape')
expect(await e2e.exists(popupContent)).toBe(false)
expect(await e2e.isFocused(popupTrigger)).toBe(true)
})
})
20 changes: 20 additions & 0 deletions packages/react/src/components/Dialog/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as customPropTypes from '@stardust-ui/react-proptypes'
import * as _ from 'lodash'
import * as PropTypes from 'prop-types'
import * as React from 'react'
import * as keyboardKey from 'keyboard-key'

import {
UIComponentProps,
Expand Down Expand Up @@ -203,6 +204,19 @@ class Dialog extends AutoControlledComponent<WithAsProp<DialogProps>, DialogStat
}
}

handleDocumentKeydown = (getRefs: Function) => (e: KeyboardEvent) => {
// if focus was lost from Dialog, for e.g. when click on Dialog's content
// and ESC is pressed, the opened Dialog should get closed and the trigger should get focus
const lastOverlayRef = getRefs().pop()
const isLastOpenedDialog: boolean =
lastOverlayRef && lastOverlayRef.current === this.overlayRef.current

if (keyboardKey.getCode(e) === keyboardKey.Escape && isLastOpenedDialog) {
this.handleDialogCancel(e)
_.invoke(this.triggerRef, 'current.focus')
}
}

renderComponent({ accessibility, classes, ElementType, styles, unhandledProps }) {
const {
actions,
Expand Down Expand Up @@ -312,6 +326,12 @@ class Dialog extends AutoControlledComponent<WithAsProp<DialogProps>, DialogStat
type="click"
capture
/>
<EventListener
listener={this.handleDocumentKeydown(getRefs)}
targetRef={targetRef}
type="keydown"
capture
/>
</>
)}
</Unstable_NestingAuto>
Expand Down
11 changes: 11 additions & 0 deletions packages/react/src/components/Popup/Popup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,17 @@ export default class Popup extends AutoControlledComponent<PopupProps, PopupStat
if (isMatchingKey && this.isOutsidePopupElementAndOutsideTriggerElement(getRefs(), e)) {
this.trySetOpen(false, e)
}

// if focus was lost from Popup and moved to body, for e.g. when click on popup content
// and ESC is pressed, the last opened Popup should get closed and the trigger should get focus
const lastContentRef = getRefs().pop()
const isLastOpenedPopup: boolean =
lastContentRef && lastContentRef.current === this.popupDomElement
const bodyHasFocus: boolean = document.activeElement === document.body

if (keyCode === keyboardKey.Escape && bodyHasFocus && isLastOpenedPopup) {
this.close(e, () => _.invoke(this.triggerFocusableDomElement, 'focus'))
}
}

isOutsidePopupElementAndOutsideTriggerElement(refs: NodeRef[], e) {
Expand Down

0 comments on commit a26fba7

Please sign in to comment.