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

fix(FocusTrapZone): do not propagate any keyboard events #1180

Merged

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Apr 5, 2019

closes #1156

Issues caused:

Because of Portal events propagating to the parent components in React element tree, it caused post-effects: keyboard event was propagated outside Portal, and was handled by another component too. (Issue example #1156)

Why:

FocusTrapZone should not only trap focus but totally block user interaction outside of it - i.e. stop propagating keyboard events.

@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #1180 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
- Coverage   82.37%   82.36%   -0.01%     
==========================================
  Files         733      733              
  Lines        8720     8716       -4     
  Branches     1165     1231      +66     
==========================================
- Hits         7183     7179       -4     
  Misses       1522     1522              
  Partials       15       15
Impacted Files Coverage Δ
packages/react/src/components/Popup/Popup.tsx 75.54% <ø> (-0.4%) ⬇️
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx 78.19% <100%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6029804...3bde189. Read the comment docs.

@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Fixes
- Truncate `content` and `header` of `ListItem` when used from `DropdownSelectedItem` @silviuavram ([#1161](https://github.com/stardust-ui/react/pull/1161))
- `FocusTrapZone` - Do not propagate any keyboard events @sophieH29 ([#1180](https://github.com/stardust-ui/react/pull/1180))
Copy link
Contributor

Choose a reason for hiding this comment

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

could you, please, provide some notes about resolution strategy in this PR's description - I mean, specifically

  • what was the cause of the issue
  • why this was considered as a fix

This description will help a lot to better understand why provided changes are considered to be a fix for the issue, as well as to retrospect this case if we will experience similar problem in future. Thank you!

@sophieH29 sophieH29 merged commit 5835d10 into master Apr 5, 2019
@sophieH29 sophieH29 deleted the fix/focus-trap-zone-do-not-propagate-keyboard-events branch April 5, 2019 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard event propagates from Dialog component to ListItem
3 participants