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

fix(Popup): Stop event propagation when press Escape on the popup #515

Merged
merged 3 commits into from
Nov 22, 2018

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Nov 22, 2018

TODO:

  • Update CHANGELOG.md

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, while Popup will always have it as a separate action stopPropagation

 stopPropagation: {
        keyCombinations: [{ keyCode: keyboardKey.Escape }],
      },

@sophieH29 sophieH29 self-assigned this Nov 22, 2018
@sophieH29 sophieH29 changed the title Stop event propagation when press Escape on the popup fix(Popup): Stop event propagation when press Escape on the popup Nov 22, 2018
@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

Merging #515 into master will decrease coverage by 0.12%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/components/Popup/Popup.tsx 30.09% <0%> (-0.6%) ⬇️

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 ef38731...da31dcc. Read the comment docs.

@@ -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(),
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@kuzhelov kuzhelov left a 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

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 22, 2018
closeAndFocusTrigger: e => this.closeAndFocusTrigger(e),
closeAndFocusTrigger: e => {
this.closeAndFocusTrigger(e)
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.

👍

@kuzhelov
Copy link
Contributor

please, don't forget to update CHANGELOG :)

@sophieH29 sophieH29 merged commit c7bef52 into master Nov 22, 2018
@sophieH29 sophieH29 deleted the fix/popup-do-not-propagate-event-on-esc branch November 22, 2018 14:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants