-
Notifications
You must be signed in to change notification settings - Fork 53
fix(Popup): Stop event propagation when press Escape on the popup #515
Conversation
Codecov Report
@@ Coverage Diff @@
## master #515 +/- ##
=========================================
- Coverage 88.42% 88.3% -0.13%
=========================================
Files 42 42
Lines 1452 1454 +2
Branches 211 186 -25
=========================================
Hits 1284 1284
- Misses 163 165 +2
Partials 5 5
Continue to review full report at Codecov.
|
src/components/Popup/Popup.tsx
Outdated
@@ -129,6 +129,7 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps | |||
this.trySetOpen(!this.state.open, e, true) | |||
}, | |||
closeAndFocusTrigger: e => this.closeAndFocusTrigger(e), | |||
stopPropagation: e => e.stopPropagation(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this concept is part of accessibility behavior, isn't it - otherwise we will need to decide the same way for toggle
as well. We had already similar discussion with Juraj and had decided to leave event propagation aspects to the client.
More than that, looking on the implementation here (https://github.com/stardust-ui/react/pull/515/files#diff-1942cc0b79094f8768e5b6847613a941R28) I have concerns about the fact that multiple actions will be called for the same event without any prescribed order suggested by the API - I would rather avoid to introduce this from happening.
I would rather suggest to make explicit e.stopPropagation()
call in the closeAndFocusTrigger
processing logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to inline this into the already existing handler but thought having it in behavior can bring more flexibility for us.
Ok, then will move it to closeAndFocusTrigger
action handler for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would rather suggest to avoid modifications made for popupBehavior
- as this will save us from additional indirection level being introduced, - and just inline necessary logic to the closeAndFocusTrigger
action handler
closeAndFocusTrigger: e => this.closeAndFocusTrigger(e), | ||
closeAndFocusTrigger: e => { | ||
this.closeAndFocusTrigger(e) | ||
e.stopPropagation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
please, don't forget to update CHANGELOG :) |
TODO:
After using Popup in different prototypes, I encountered an issue. When pressing
Esc
button on popup, popup closes and trigger gets focus (that's expected). But this event is propagated, so it can be handled also by other layers and the focus can be removed from the trigger, what is not expected behavior.Expected behavior
Focus should remain on trigger element when clicking Esc, and the event shouldn't propagate.
To fix it I've added additional key action in
popupBehavior
, so we can be quite flexible here. For e.g, if for some case we won't need it, or we need it also for other keys - appropriate behavior can be created for that, whilePopup
will always have it as a separate actionstopPropagation