feat(dialog): expose close attempts

[fxck]

Should close #3251

Discussion why is it needed #2853 (comment)

I went with just backdrop and escape attempt types, there could be api as well, but I don't really see a use case for that (the reason for this whole closeAttempts thing is to be able to catch events you can't really affect, which you definitely can with .close().. also I'd require more refactoring).

@crisbeto can you review, pretty please.

also cc @jelbourn

[crisbeto]

In general, I'm not sure whether we should consider successful closes as "attempts". We could emit something along the lines of { type: "backdrop", success: true } which may come in handy.

[crisbeto]

Would be better if this were called something like MdDialogCloseAttempt.

[crisbeto]

Add a trailing comma here.

[fxck]

In general, I'm not sure whether we should consider successful closes as "attempts". We could emit something along the lines of { type: "backdrop", success: true } which may come in handy.

@crisbeto not sure about this, truly successful close is only after animation is done, and this needs to emit at the beginning of the animation. And honestly the only factor that would affect the success is whether disableClose is true or not, which is something the user explicitly sets.

Other than that I'm all for better naming, but attempt felt fitting.

[crisbeto]

Whenever I read "attemp", I assume that it didn't go through, but these obviously can. Perhaps @jelbourn can weigh in?

[fxck]

@jelbourn can you please please comment, I don't want this to be stuck on just naming for weeks. It would help a ton if it would be merged.

[fxck]

@crisbeto I consulted with couple of native speakers, they said attempt is alright, that it doesn't at all imply failure

[victornoel]

@crisbeto any news on this PR? Would be great to have it in the next release :)

[crisbeto]

I think @jelbourn should weight in on this.

[fxck]

Another week has gone by..

[victornoel]

@fxck until then, I think you can rebase this PR, there is some conflicts now :)

[ggranum]

@crisbeto , @jelbourn: I commented on #3251 already, but wanted to raise this ticket up also, as the lack of this hook is causing me some minor annoyance. I would like to allow users to close the dialog however they want, but I need to know when it's about to close in order to prevent bad input.

Consider a text input field - the onBlur event is commensurate with the onFocus event that will dismiss the dialog if the click is outside the boundary of my Dialog Component. There is nothing I can do to conditionally prevent the close.

If it's the syntax of closeAttempt, I would suggest either beforeClose in alignment with ExtJS, as I mentioned on #3251, or closeRequest, as I've seen the use of *Request previously in either the Angular or the AngularMaterial codebase - I don't recall which.

@crisbeto, To respond to your comment

I'm not sure whether we should consider successful closes as "attempts"

I'm not certain of course, but I think you're looking at this from the same point of view as I prefer - I would prefer to get a beforeClose event, with the option of canceling the close. I'm not super picky however, so this slightly different paradigm of preventing all non-programmatically driven close operations and then signing up for the closeAttempt hook and closing it programmatically as appropriate is fine for me as well. The difference being, from the point of view of developers using this new event, ALL attempts are not 'successful', because the target developer using this event will have disabled all the click/tap/keyboard driven close options. Assuming I understood my quick scan of the code changes correctly, which is far from certain.

All that said, a closeAttempt with type api also does make sense, as it could save users who are just looking for a general "the user would like the dialog to close but it hasn't yet" hook from having to hook up a second listener just to deal with the api driven close events. In this case I would agree that attempt does slightly confuse the meaning, and request (or ???) is perhaps more desirable.

If it's just not going to happen, please let us know.

Many thanks.

[fxck]

Any chance of review, now that you are back from ng-conf @jelbourn ?

[alexw10]

@fxck Will this help with the use case of being able to prevent the dialog from closing on an escape or when using the vdl-dialog-close when the dialog has been opened.
Basically the use case I am wondering is being able to set the disableClose to true when a dialog is already opened. Reason for this is by default we want the disable close to be false, but if certain things happen within the Dialog we want to set disableClose to true so the dialog cannot be closed.

Maybe this is different but if so do you know how to accomplish the use case I am saying?

[fxck]

@alexw10 Yes, this is exactly what this will allow you to do.

[alexw10]

@fxck Let's get this in than! @jelbourn 💯

[maxime1992]

@frankspin89 I'm also waiting this PR to be merged for that use case .... 👍

[jelbourn]

Took a look at how this works now and the proposed changes. I find the addition of closeAttempts along with having a CloseAttemptType to be more complicated than this needs to be.

Is there any reason that exposing the overlay's backdropClick wouldn't be enough? You can already add your own event listener for escape, so doing this would be be a matter of setting disableClose to true and then combining the backdropClick and your own escape key handler.

The only other thing in the library that closes a dialog is the MdDialogClose directive, which I imagine you just wouldn't use in this situation.

[fxck]

You can certainly catch escape key by yourself, but you can't quite expect everyone who'd utilise this to know how to do it properly and you could get it easily here for "free", not to mention I think it kinda makes sense. But honestly I'll do anything you tell me to do to get this merged.

[frankspin89]

@fxck I think it's important to include caching escape key. Would prevent lot's of new questions related to this topic. But if it's a no go for the material team, please provided detailed documentation to implement caching escape key triggers by yourself.

[jelbourn]

@fxck so would changing the PR (or opening a new one) that just exposes the backdropClick from OverlayRef do what you need?

[fxck]

Yeah, but it will be pain in the ass to have to set up my own escape key handler with all the logic about whether there are open dialogs etc. It will generate unnecessary issues, I really don't like it, but better than nothing I guess.

[maxime1992]

25 days since the last comment and I badly need this once again. Any news ?

[willshowell]

@jelbourn, up to now I've been indifferent about the approach taken by this PR, but with menus getting a disableClose option soon #4842, (and a potentially valid feature for datepicker) I'd like to also put a vote in favor of @fxck's original approach.

I recognize that it's possible and not too difficult for developers to handle the escape handler themselves, but I think whatever approach is chosen here must have a matching API across components. And as the behavior for closing overlays is being defined/refined at a decent clip (#4666, #4843), the value for developers to get this and future refinements for free is huge when compared against the real, but small, cost in complexity.

This value is only increased when different components have unique closing requirements (e.g. datepicker selections, menu tabbing out). Further, as shown in this use-case and in #2612, a user may want to disable closing via one method (menu panel click) while retaining other methods (escape key and backdrop click). This would be amazingly easy by just filtering closeAttempts.

(I realize none of my points are really about dialogs, but I feel strongly about consistent behavior across other overlay-based components)

[fxck]

want to take this over @willshowell ?

[willshowell]

@fxck I'd be happy to, though since my last comment, the disableClose PR for menu was cancelled - which makes sense, but invalidates my argument.

@jelbourn Autocomplete has panelClosingActions

  /**
   * A stream of actions that should close the autocomplete panel, including
   * when an option is selected, on blur, and when TAB is pressed.
   */
  get panelClosingActions(): Observable<MdOptionSelectionChange> {
    return merge(
      this.optionSelections,
      this.autocomplete._keyManager.tabOut,
      this._outsideClickStream
    );
  }

which turns out to be very helpful for #3334 (comment), and is not entirely different from the use cases expressed in this thread. What if this PR were organized differently?

  // dialog.ts

  closeAll(): void {
    let i = this._openDialogs.length;

    while (i--) {
      this._openDialogs[i].emitCloseEvent(true);
    }
  }

  private _attachDialogContent<T>(...): MdDialogRef<T> {
    // ...
    if (config.hasBackdrop) {
      overlayRef.backdropClick().subscribe(() => {
        dialogRef.emitCloseEvent();
      });
    }
    // ...
  }

  private _handleKeydown(event: KeyboardEvent): void {
    let topDialog = this._openDialogs[this._openDialogs.length - 1];

    if (event.keyCode === ESCAPE) {
      topDialog.emitCloseEvent();
    }
  }

  // dialog-ref.ts

  _closeEvent = new Subject<boolean>();

  get dialogClosingActions() {
    return this._closeEvent.asObservable();
  }

  emitCloseEvent(forceClose = false) {
    this._closeEvent.next(forceClose);
  }

  ngOnInit() {
    this.dialogClosingActions
      .filter(forceClose => !this.disableClose || forceClose)
      .subscribe(() => this.close());
  }

(Also, maybe instead of forceClose, the Subject could pass the actual events and those are filtered out as needed)

[jelbourn]

My main issue is that I want to avoid any kind of cancellation APIs throughout the library (APIs like preventDefault). Reasons for this:

• It's hard to draw a line between which actions should be cancellable and which can't
• It makes the event's control-flow more complicated, especially if you want to support async cancellation (which is the logical progression).
• I haven't seen a strong case where you couldn't accomplish the desired end result by disabling the action in the first place.

For dialogs specifically, I can see how something like dialogClosingActions would be useful, but still wouldn't block programmatic calls to close() based on disableClose. The property is more meant to be disableDefaultCloseActions

[willshowell]

@jelbourn Understood. What if each OverlayRef had an escape listener? I'm sure autocomplete/menu could benefit from it, and it would solve annoyances like escaping out of an autocomplete within a dialog closing the dialog.

I see that adding the listener to the document solved #3009, and I don't fully understand the caveats, but maybe there is an even better solution that could ultimately provide DialogRef with backdropClick and overlayEscape streams (which the user could merge on their own).

[jelbourn]

I think anything escape-related is too specific for Overlay since it also captures things like snackbar and tooltip which aren't interactive surfaces. (In hindsight the backdrop may also have been too specific as well vs. treating it as a second overlay).

Is there currently an issue w/ escape closing the dialog if your focus is on a menu/autocomplete/etc.?

[willshowell]

@jelbourn yes, tracking #6223.

Is there any reason that exposing the overlay's backdropClick wouldn't be enough? You can already add your own event listener for escape, so doing this would be be a matter of setting disableClose to true and then combining the backdropClick and your own escape key handler.

Admittedly, replicating an escape listener is easy

Observable.fromEvent(document, 'keydown')
  .filter(e => e.code === 'Escape')
  .takeUntil(this._onDestroy)
  .subscribe(() => this.doSomething());

But when you have multiple dialogs, something like this will affect all open dialogs, not just the topmost one.

In order to address the original purpose of this PR, I think one of the following must happen:

1. DialogRef gets an escape stream (preferred)

- if (event.keyCode === ESCAPE && canClose) {
-   topDialog.close();
- }
+ if (event.keyCode === ESCAPE) {
+   topDialog.dialogEscape.next(event);
+ }

2. _openDialogs gets exposed as public so that users can reimplement MdDialog#_handleKeydown for each of their dialogs

EDIT: or 3 which could be a full blown EscapeHandler cdk utility 😄

EDIT 2: I'm wondering if backdrop and escape behavior could actually be more coupled since esc seems to be a natural response when something is blocking your interaction with the page. Is refactoring backdrop into its own overlay something you're considering or that just a pipe dream?

[jelbourn]

At this point I wouldn't bother with the backdrop since it would be too much of a breaking change.

Would it be simplest to just expose the DOM element on MdDialogRef? I think not exposing it ultimately just makes people query for it anyway.

[willshowell]

I don't think that would help though. If the dialog has no focusable elements, or the user has clicked away from the focusable elements in the dialog, keydown events target the last focused item or the body, respectively, instead of the dialog. So they can't be caught by a DOM element ref... unless I'm missing something?

See #3009 and https://plnkr.co/edit/puYeeRCta8A7moZOPRWI?p=preview

[jelbourn]

Ah, the escape handling changed while I was on vacation back in February. In my head the event listener was still on the dialog container.

It feels like there needs to be a more involved keyboard event dispatcher for overlays. Probably one listener on the body that dispatches the event to the appropriate overlay based on some combination of event origin and open order (maybe with a separate priority mechanism). I don't think special-casing escape specifically is a good idea since we could just get into the same situation with another key.

[willshowell]

Cool! So to summarize:

1. Overlays get their own KeyboardEvent dispatcher
2. MdDialog no longer listens to document keydown events
3. MdDialogRef publicly exposes backdropClick and the keyboard dispatcher stream from OverlayRef
4. User can manually filter and merge streams to accomplish initial goal of this PR

@fxck thoughts?

[fxck]

Sounds good.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}