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

fix: escape behavior of popup and dropdown #1183

Merged
merged 6 commits into from
Apr 8, 2019
Merged

fix: escape behavior of popup and dropdown #1183

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ebf5ec1
introduce fix
kuzhelov-ms Apr 7, 2019
ef37ab7
Merge branch 'master' into fix/escape-behavior-of-popup-and-dropdown
kuzhelov-ms Apr 7, 2019
de7e787
update changelog
kuzhelov-ms Apr 7, 2019
97ac0c5
Merge branch 'master' into fix/escape-behavior-of-popup-and-dropdown
kuzhelov Apr 8, 2019
3b2b65f
Merge branch 'master' into fix/escape-behavior-of-popup-and-dropdown
kuzhelov Apr 8, 2019
ddf2bae
Merge branch 'master' into fix/escape-behavior-of-popup-and-dropdown
kuzhelov Apr 8, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Truncate `content` and `header` of `ListItem` when used from `DropdownSelectedItem` @silviuavram ([#1161](https://github.com/stardust-ui/react/pull/1161))
- Fix `rotate` prop on `Icon` not working in `rtl` @mnajdova ([#1179](https://github.com/stardust-ui/react/pull/1179))
- `FocusTrapZone` - Do not propagate any keyboard events @sophieH29 ([#1180](https://github.com/stardust-ui/react/pull/1180))
- Capture effect of `Esc` key down event within component for `Popup` and `Dropdown` @kuzhelov ([#1183](https://github.com/stardust-ui/react/pull/1183))

<!--------------------------------[ v0.26.0 ]------------------------------- -->
## [v0.26.0](https://github.com/stardust-ui/react/tree/v0.26.0) (2019-04-03)
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/components/Dropdown/Dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,7 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo
case keyboardKey.Escape:
accessibilityInputPropsKeyDown(e)
this.tryFocusTriggerButton()
e.stopPropagation()
return
default:
accessibilityInputPropsKeyDown(e)
Expand Down
7 changes: 5 additions & 2 deletions packages/react/src/components/Popup/Popup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,13 @@ export default class Popup extends AutoControlledComponent<ReactProps<PopupProps

protected actionHandlers: AccessibilityActionHandlers = {
closeAndFocusTrigger: e => {
e.stopPropagation()
this.close(e, () => _.invoke(this.triggerFocusableDomElement, 'focus'))
e.stopPropagation()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed to stop propagation in the popup? As far as understood, the problem was caused in Dropdown which was propagating keyboard event to Popup. Seems like Dropdown changes should fix it, isn't it?

Copy link
Contributor

@sophieH29 sophieH29 Apr 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I believe Dropdown in Popup will be usually used with focus trap behavior. This PR #1180 stop propagation for any keyboard events within FocusTrapZone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sophieH29 why is it needed to stop propagation in the popup? As far as understood, the problem was caused in Dropdown which was propagating keyboard event to Popup

Actually, this makes just one aspect of the problem, the one that is replicated with the codesandbox example in the issue description: #1079 . If it would be the only problem, than yes, changes to the Dropdown would be enough.

However, as it is mentioned in the issue, the same problem applies to Popup -> Popup case, and this behavior is not correct (from issue's description):

The same result will be for multiple Popups.

@sophieH29 I believe Dropdown in Popup will be usually used with focus trap behavior. This PR #1180 stop propagation for any keyboard events within FocusTrapZone

Yes, it should - however, in the meanwhile this fix is necessary

},
close: e => {
this.close(e)
e.stopPropagation()
},
close: e => this.close(e),
toggle: e => {
e.preventDefault()
this.trySetOpen(!this.state.open, e)
Expand Down