From f8ca9e1c6507396b9e36e1395f71a26dd28cb11c Mon Sep 17 00:00:00 2001 From: Sofiya Huts Date: Thu, 7 Mar 2019 13:01:44 +0100 Subject: [PATCH 1/4] fix(Popup): Do not propagate keyboard event only when focus trap behavior is used --- packages/react/src/components/Popup/Popup.tsx | 14 +++--- .../specs/components/Popup/Popup-test.tsx | 49 ++++++++++++++++--- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/packages/react/src/components/Popup/Popup.tsx b/packages/react/src/components/Popup/Popup.tsx index fe1b57fd10..f8f7f93a65 100644 --- a/packages/react/src/components/Popup/Popup.tsx +++ b/packages/react/src/components/Popup/Popup.tsx @@ -469,12 +469,7 @@ export default class Popup extends AutoControlledComponent { - // No need to propagate keydown events outside Popup - // allow only keyboard actions to execute - _.invoke(accessibility.keyHandlers.popup, 'onKeyDown', e) - e.stopPropagation() - }, + ...accessibility.keyHandlers.popup, className: popupPositionClasses, style: popupPlacementStyles, ...this.getContentProps(), @@ -483,6 +478,13 @@ export default class Popup extends AutoControlledComponent { + // No need to propagate keydown events outside Popup + // when focus trap behavior is used + // allow only keyboard actions to execute + _.invoke(accessibility.keyHandlers.popup, 'onKeyDown', e) + e.stopPropagation() + }, } as FocusTrapZoneProps const autoFocusProps = { diff --git a/packages/react/test/specs/components/Popup/Popup-test.tsx b/packages/react/test/specs/components/Popup/Popup-test.tsx index d953ce3fc4..1660a94d50 100644 --- a/packages/react/test/specs/components/Popup/Popup-test.tsx +++ b/packages/react/test/specs/components/Popup/Popup-test.tsx @@ -8,6 +8,9 @@ import { Alignment, } from 'src/components/Popup/positioningHelper' import Popup, { PopupEvents } from 'src/components/Popup/Popup' +import { Accessibility } from 'src/lib/accessibility/types' +import { popupFocusTrapBehavior, popupBehavior, dialogBehavior } from 'src/lib/accessibility/index' + import { domEvent, mountWithProvider } from '../../../utils' import * as keyboardKey from 'keyboard-key' import { ReactWrapper } from 'enzyme' @@ -68,12 +71,6 @@ describe('Popup', () => { expect(getPopupContent(popup).exists()).toBe(true) - // when popup open, check that stopPropagation is called when keyboard events are invoked - const stopPropagation = jest.fn() - const popupContentElement = getPopupContent(popup) - popupContentElement.simulate('keyDown', { stopPropagation }) - expect(stopPropagation).toHaveBeenCalledTimes(1) - // check popup closes on Esc popupTriggerElement.simulate('keydown', { keyCode: keyboardKeyToClose }) expect(getPopupContent(popup).exists()).toBe(false) @@ -266,4 +263,44 @@ describe('Popup', () => { expect(contentElement.classList.contains(Popup.Content.className)).toEqual(true) }) }) + + describe('keyboard event propagation', () => { + const expectPopupToStopPropagation = ( + behavior: Accessibility, + shouldStopPropagation: boolean, + ) => { + const popup = mountWithProvider( + text to trigger popup } + content={} + accessibility={behavior} + />, + ) + + // check popup open on key press + const popupTriggerElement = popup.find(`#${triggerId}`) + popupTriggerElement.simulate('keydown', { keyCode: keyboardKey.Enter }) + + expect(getPopupContent(popup).exists()).toBe(true) + + // when popup open, check that stopPropagation is called when keyboard events are invoked + const stopPropagation = jest.fn() + const popupContentElement = getPopupContent(popup) + popupContentElement.simulate('keyDown', { stopPropagation }) + if (shouldStopPropagation) { + expect(stopPropagation).toHaveBeenCalledTimes(1) + } else { + expect(stopPropagation).not.toBeCalled() + } + } + test('stop when focus trap behavior is used', () => { + expectPopupToStopPropagation(popupFocusTrapBehavior, true) + }) + test('stop when dialog behavior is used', () => { + expectPopupToStopPropagation(dialogBehavior, true) + }) + test('does not stop when default behavior is used', () => { + expectPopupToStopPropagation(popupBehavior, false) + }) + }) }) From daacc9cb45aed84d6420909b951981d884c14dbd Mon Sep 17 00:00:00 2001 From: Sofiya Huts Date: Thu, 7 Mar 2019 13:03:53 +0100 Subject: [PATCH 2/4] Update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d8c6e3156..20bd3a0d5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixes +- Do not propagate keyboard events outside `Popup`'s content only when focus trap is used @sophieH29 ([#1028](https://github.com/stardust-ui/react/pull/1028)) + ### Features - Add `inline` prop in the `Popup` for rendering the content next to the trigger element @mnajdova ([#1017](https://github.com/stardust-ui/react/pull/1017)) From 3a3df0b63202dbd867cced5bb62c876ea3f483b4 Mon Sep 17 00:00:00 2001 From: Sofiya Huts Date: Thu, 7 Mar 2019 13:06:27 +0100 Subject: [PATCH 3/4] Small improvements --- packages/react/test/specs/components/Popup/Popup-test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/test/specs/components/Popup/Popup-test.tsx b/packages/react/test/specs/components/Popup/Popup-test.tsx index 1660a94d50..6d61b1fa25 100644 --- a/packages/react/test/specs/components/Popup/Popup-test.tsx +++ b/packages/react/test/specs/components/Popup/Popup-test.tsx @@ -293,10 +293,10 @@ describe('Popup', () => { expect(stopPropagation).not.toBeCalled() } } - test('stop when focus trap behavior is used', () => { + test('stops when focus trap behavior is used', () => { expectPopupToStopPropagation(popupFocusTrapBehavior, true) }) - test('stop when dialog behavior is used', () => { + test('stops when dialog behavior is used', () => { expectPopupToStopPropagation(dialogBehavior, true) }) test('does not stop when default behavior is used', () => { From b8f902def48296ae6a464ddc447858cf43d4f37a Mon Sep 17 00:00:00 2001 From: Sofiya Huts Date: Thu, 7 Mar 2019 13:13:28 +0100 Subject: [PATCH 4/4] Improvements after CR comments --- .../test/specs/components/Popup/Popup-test.tsx | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/react/test/specs/components/Popup/Popup-test.tsx b/packages/react/test/specs/components/Popup/Popup-test.tsx index 6d61b1fa25..b682f22acf 100644 --- a/packages/react/test/specs/components/Popup/Popup-test.tsx +++ b/packages/react/test/specs/components/Popup/Popup-test.tsx @@ -265,7 +265,7 @@ describe('Popup', () => { }) describe('keyboard event propagation', () => { - const expectPopupToStopPropagation = ( + const expectPopupToHandleStopPropagation = ( behavior: Accessibility, shouldStopPropagation: boolean, ) => { @@ -277,30 +277,24 @@ describe('Popup', () => { />, ) - // check popup open on key press + // open popup const popupTriggerElement = popup.find(`#${triggerId}`) popupTriggerElement.simulate('keydown', { keyCode: keyboardKey.Enter }) - expect(getPopupContent(popup).exists()).toBe(true) - // when popup open, check that stopPropagation is called when keyboard events are invoked const stopPropagation = jest.fn() const popupContentElement = getPopupContent(popup) popupContentElement.simulate('keyDown', { stopPropagation }) - if (shouldStopPropagation) { - expect(stopPropagation).toHaveBeenCalledTimes(1) - } else { - expect(stopPropagation).not.toBeCalled() - } + expect(stopPropagation).toHaveBeenCalledTimes(shouldStopPropagation ? 1 : 0) } test('stops when focus trap behavior is used', () => { - expectPopupToStopPropagation(popupFocusTrapBehavior, true) + expectPopupToHandleStopPropagation(popupFocusTrapBehavior, true) }) test('stops when dialog behavior is used', () => { - expectPopupToStopPropagation(dialogBehavior, true) + expectPopupToHandleStopPropagation(dialogBehavior, true) }) test('does not stop when default behavior is used', () => { - expectPopupToStopPropagation(popupBehavior, false) + expectPopupToHandleStopPropagation(popupBehavior, false) }) }) })