Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

ESC that cancels a modal: Stop event propagation #1692

Closed
eekboom opened this issue Jan 29, 2014 · 7 comments
Closed

ESC that cancels a modal: Stop event propagation #1692

eekboom opened this issue Jan 29, 2014 · 7 comments

Comments

@eekboom
Copy link

eekboom commented Jan 29, 2014

If I press ESCAPE while a modal is open, in my app two things happen:
The modal closes (fine).
My app navigates to the parent state (speaking "angular-ui-router"-wise), which is due to a custom ESC handler I added.

angular-bootstrap should stop the keydown event propagation when a modal is closed with ESC, so that it does not trigger any further actions.

@mvhecke
Copy link
Contributor

mvhecke commented Jan 29, 2014

It seems to be a duplicate of #1643 and that fix isn't released yet, it will be included in the next release.

@eekboom
Copy link
Author

eekboom commented Jan 29, 2014

I think #1643 is unrelated.

I am talking about the need to call evt.stopPropagation() on the keydown event in $modalStack.
That hasn't really anything to do with destroying the scope.

@eekboom eekboom closed this as completed Jan 29, 2014
@eekboom eekboom reopened this Jan 29, 2014
@mvhecke
Copy link
Contributor

mvhecke commented Jan 29, 2014

Sorry, should have read better. We could check if there is anything in the modalStack just by checking the length. Would changing the following line fix your problem?

https://github.com/angular-ui/bootstrap/blob/master/src/modal/modal.js#L199

Change this

if (evt.which === 27) {

To this

if (evt.which === 27 && $$stackedMap.length !== 0) {

@eekboom
Copy link
Author

eekboom commented Jan 30, 2014

No, sorry for not making myself clear enough.
What would fix my problem is to change the code like this:

if (evt.which === 27) {
  modal = openedWindows.top();
  if (modal && modal.value.keyboard) {
    evt.stopPropagation(); // <<--- ADDED LINE

If the ESC closes a modal window, it should be considered "consumed" and should not bubble up and also trigger an action somewhere up the DOM.

@mvhecke
Copy link
Contributor

mvhecke commented Jan 30, 2014

It's an interesting case, in your case you would want a global option for the modals where you can disable the event listing for ESC?

@dliebscher
Copy link

I'd guess, eekboom's point was the following:
If the modal closes due to an escape-keypress, the event gets bubbled to parent elements even though it is formally consumed (the action appropriate to the event has happened -> modal has closed).
In his parent form, there's a custom event handler, that closes the current (non-modal) form on ESCAPE by navigating to the parent scope.

From user's point of view, the following happens:
1 User opens a view
2 User opens a modal dialog upon that view
3 User presses escape (to cancel the modal dialog)
--> modal dialog gets closed (perfect)
--> view gets closed (not expected)

I'm experiencing exactly such an issue. So please consume the event if you take an action.

@bekos
Copy link
Contributor

bekos commented Apr 19, 2014

@eekboom This is a valid point, but the correct fix would just preventDefault() (stopPropagtion causes some weird, difficult to debug issues) and your custom handler should check if isDefaultPrevented() in order to execute. Will submit a PR soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants