Dialog visible attr change not triggers any hide events

[luchillo17]

So i want to bind a callback to onBeforeHide, and when closing the p-dialog with the x button in the header it triggers the callback, so far all green.

The issue comes when i close it programatically by setting the visible binding to false, it closes as expected, however the event isn't triggered, that's an issue when you have custom templates with buttons that performs an action and then closes the dialog using the variable binding.

In current state my workaround is to call the close function both in the button with click binding, and in the onBeforeHide event, in that function i set the visible binding to false and then execute some post processing.

I know it's redundant to set it to false when using the event binding but at least i don't have duplicated code, however it should in deed trigger the event when the visible boolean binding changes to false.

[sliekens]

I have the same problem. The example code from the documentation is buggy.

<p-dialog (visible)]="display">
  <p-footer>
    <div class="ui-dialog-buttonpane ui-widget-content ui-helper-clearfix">
      <button type="button" pButton icon="fa-close" (click)="display=false" label="No"></button>
      <button type="button" pButton icon="fa-check" (click)="display=false" label="Yes"></button>
    </div>
  </p-footer>
</p-dialog>

Setting display=false in the buttons' click handlers correctly hides the dialog, but it doesn't trigger the onBeforeHide or onAfterHide events.

Instead I had to write custom code that calls the Dialog.hide(event) method.

<p-dialog #dialog (visible)]="display">
  <p-footer>
    <div class="ui-dialog-buttonpane ui-widget-content ui-helper-clearfix">
      <button type="button" pButton icon="fa-close" (click)="dialog.hide($event)" label="No"></button>
      <button type="button" pButton icon="fa-check" (click)="confirm();dialog.hide($event)" label="Yes"></button>
    </div>
  </p-footer>
</p-dialog>

[luchillo17]

@StevenLiekens I think you miss-typo in the visible binding, lacks left bracket, is that in your code as well or just bad copy-paste?

[sliekens]

Bad copy-paste indeed. Example code is taken from here: http://www.primefaces.org/primeng/#/dialog

I modified the example to make the important bits stand out. I didn't mean to delete the left bracket.

[sliekens]

I'll try to make it absolutely clear for everyone.

Here is the minimal repro for a dialog + event handler for onAfterHide. The dialog has a button to hide the dialog. Clicking this button correctly hides the dialog, but the onAfterHide event is not triggered and the alert is never shown.

<p-dialog [(visible)]="display" (onAfterHide)="alert('Dialog hidden!')" >
  <button (click)="display=false">Close</button>
</p-dialog>

The workaround that I found is to forward the button's click event to the Dialog's hide method.

<p-dialog #dialog [(visible)]="display" (onAfterHide)="alert('Dialog hidden!')" >
  <button (click)="dialog.hide($event)">Close</button>
</p-dialog>

This code is the only code that behaves as expected.

[huineng]

Was this ever solved, i still cannot get onAfterHide of onBeforeHide to work

[luchillo17]

It seems not, it seems an easy one to fix with Getter/Setter, so it should be fixed ages ago.

[huineng]

ok i got it working now just by adding the event
(onAfterHide)="onAfterHide($event)"

[luchillo17]

@StevenLiekens I'm sorry, i didn't read your post thoroughly enough to notice that it works if the event is forwarded to the dialog's hide method, anyways it's weird that we need to do such thing to make it work, what if we call the hide method from code and we don't have any event to pass to it? would it need to create a dummy event only to get this to work?

@cagataycivici I still think this issue is still unresolved, i think @StevenLiekens hack is a workaround, not a solution.

[sliekens]

I don't think that passing the click event was necessary to make my example work. I don't remember why I added that.

[luchillo17]

Ok so it works if we call the hide method, am i right? what about if we just change the variable, does it still triggers the onAfterHide event?

[sliekens]

Changing the variable does not trigger the event.

[luchillo17]

That's the issue i want fixed, that variable is a binding, an input & output for showing/hiding the dialog, so in theory it should as well trigger such events (onBeforeHide & onAfterHide event, and any other that relates to hide).

[sliekens]

Some cleanup is in order to make this work. The logic for showing/hiding the dialog and triggering the events looks like a mess. No wonder it doesn't work.

These blocks of code look smelly to me:
https://github.com/primefaces/primeng/blob/master/components/dialog/dialog.ts#L136-L147
https://github.com/primefaces/primeng/blob/master/components/dialog/dialog.ts#L149-L160
https://github.com/primefaces/primeng/blob/master/components/dialog/dialog.ts#L257-L263
https://github.com/primefaces/primeng/blob/master/components/dialog/dialog.ts#L208-L214

My first thought is that show() and hide() do way too much. They should do little more than toggle the visible status. In fact, just remove show and hide. Only allow changing the dialog's visibility by using the visible property getter and setter.

The visible setter should raise onBeforeShow or onBeforeHide before assigning the new value to _visible and that's it.

The onAfterShow and onAfterHide events can be raised in a round of ngOnChanges which I believe is the only correct place for that.

I don't know what the shown property is for, but it looks like it was added to check if the dialog is in a valid state. That's amusing because a user can assign any value to it and mess up the dialog's internal state. I'm sure that this property is not really needed and can be removed after this code cleanup,

I don't know what disableModality method is but remove it anyway. Modal dialogs in HTML are a complete lie. You can't prevent a user from focusing background elements, so don't even pretend that you can. Even clicking on the dialog's overlay itself will cause a click event to bubble up to the document root and then all bets are off.

[huineng]

thanks , if you would have removed in the same effort the lines

 //  if(!this.positionInitialized) {
            this.center();
   //         this.positionInitialized = true;
   //     }

my ticket #2310 could have been closed too :)

[lincolnluiz]

Thanks @StevenLiekens!!!