fix(confirm-dialog): add reject-on-close flag

[Bock92]

Fixes: #17512

Changes made to fix the Issue:

• Add rejectOnClose flag:
  • true: the reject event will be emitted when the dialog is closed
  • false: the dialog will close without triggering the reject event

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Updated (UTC)
primeng	⬜️ Ignored (Inspect)	Visit Preview	Feb 7, 2025 1:57pm
primeng-v18	⬜️ Ignored (Inspect)	Visit Preview	Feb 7, 2025 1:57pm

[vercel]

Someone is attempting to deploy a commit to the primetek Team on Vercel.

A member of the Team first needs to authorize it.

[majkers]

So If I set rejectOnClose to true and use Cancel button that normally fires an event ConfirmEventType.REJECT and hides dialog (so indeed sets visible to false by onHide method) I will now get two events? ConfirmEventType.CANCEL and ConfirmEventType.REJECT?

[Bock92]

So If I set rejectOnClose to true and use Cancel button that normally fires an event ConfirmEventType.REJECT and hides dialog (so indeed sets visible to false by onHide method) I will now get two events? ConfirmEventType.CANCEL and ConfirmEventType.REJECT?

If you have rejectOnClose=trueand you click the close icon, the component will emit ConfirmEventType.CANCEL only, and the same happens when you press the Esc button.
Instead the Cancel button only fire the ConfirmEventType.REJECT.

Of course at the end the dialog will be hidden as always happen

[majkers]

So If I set rejectOnClose to true and use Cancel button that normally fires an event ConfirmEventType.REJECT and hides dialog (so indeed sets visible to false by onHide method) I will now get two events? ConfirmEventType.CANCEL and ConfirmEventType.REJECT?

If you have rejectOnClose=trueand you click the close icon, the component will emit ConfirmEventType.CANCEL only, and the same happens when you press the Esc button. Instead the Cancel button only fire the ConfirmEventType.REJECT.

Of course at the end the dialog will be hidden as always happen

Ok that part is OK. But what happens when somebody click reject button that is usually labelled as Cancel button? Then onReject method will be fired:

   onReject() {
        if (this.confirmation && this.confirmation.rejectEvent) {
            this.confirmation.rejectEvent.emit(ConfirmEventType.REJECT);
        }

        this.hide(ConfirmEventType.REJECT);
    }

so ConfirmEventType.REJECT will be emitted and in hide method we have:

   hide(type?: ConfirmEventType) {
        this.onHide.emit(type);
        this.visible = false;
        this.confirmation = null;
    }

and this line this.visible = false; will trigger code changed by You:

   set visible(value: any) {
        this._visible = value;

        if (this._visible && !this.maskVisible) {
            this.maskVisible = true;
        }

        if (!this._visible && this.confirmation?.rejectOnClose) {
            this._rejectOnClose();
        }

        this.cd.markForCheck();
    }

and one more event, this time ConfirmEventType.CANCEL will be fired

  _rejectOnClose() {
        if (this.confirmation?.rejectEvent) {
            this.confirmation.rejectEvent.emit(ConfirmEventType.CANCEL);
        }
    }

So two events and one button click... That is not OK.

[Bock92]

So If I set rejectOnClose to true and use Cancel button that normally fires an event ConfirmEventType.REJECT and hides dialog (so indeed sets visible to false by onHide method) I will now get two events? ConfirmEventType.CANCEL and ConfirmEventType.REJECT?

If you have rejectOnClose=trueand you click the close icon, the component will emit ConfirmEventType.CANCEL only, and the same happens when you press the Esc button. Instead the Cancel button only fire the ConfirmEventType.REJECT.

Of course at the end the dialog will be hidden as always happen

Ok that part is OK. But what happens when somebody click reject button that is usually labelled as Cancel button? Then onReject method will be fired:

   onReject() {
        if (this.confirmation && this.confirmation.rejectEvent) {
            this.confirmation.rejectEvent.emit(ConfirmEventType.REJECT);
        }

        this.hide(ConfirmEventType.REJECT);
    }

so ConfirmEventType.REJECT will be emitted and in hide method we have:

   hide(type?: ConfirmEventType) {
        this.onHide.emit(type);
        this.visible = false;
        this.confirmation = null;
    }

and this line this.visible = false; will trigger code changed by You:

   set visible(value: any) {
        this._visible = value;

        if (this._visible && !this.maskVisible) {
            this.maskVisible = true;
        }

        if (!this._visible && this.confirmation?.rejectOnClose) {
            this._rejectOnClose();
        }

        this.cd.markForCheck();
    }

and one more event, this time ConfirmEventType.CANCEL will be fired

  _rejectOnClose() {
        if (this.confirmation?.rejectEvent) {
            this.confirmation.rejectEvent.emit(ConfirmEventType.CANCEL);
        }
    }

So two events and one button click... That is not OK.

This Is true, but when the component Is hidden It will be destroyed and ngOnDestroy() will unsubscribe the events, so we don't have the double events.
I test this case. Did you reproduce this issue?

[majkers]

No I did not but visible = false thatactually hides dialog is being fired after onReject() with first event being emitted.

[Bock92]

No I did not but visible = false thatactually hides dialog is being fired after onReject() with first event being emitted.

I tested it and decided on a different solution. I'm now using the visibleChange event to trigger _rejectOnClose() when the dialog closes. This ensure the rejection logic is executed only once.

@majkers

[majkers]

I don't have time to test. But I really doubt it is any difference. IMHO it is still not the best idea to rely on visible since it might be set/changed in many places. IMHO the best approach would to overwrite dialog's header template, and set click method for Close/X button to close()

EDIT. Doing something like this IMHO is also not the best idea. Banana box together with change event

[(visible)]="visible"
...
(visibleChange)="_rejectOnClose($event)"

[Bock92]

I don't have time to test. But I really doubt it is any difference. IMHO it is still not the best idea to rely on visible since it might be set/changed in many places. IMHO the best approach would to overwrite dialog's header template, and set click method for Close/X button to close()

EDIT. Doing something like this IMHO is also not the best idea. Banana box together with change event

[(visible)]="visible"
...
(visibleChange)="_rejectOnClose($event)"

We could add a new close or afterClose property to the Confirmation interface to hold a function that executes when the Close (X) icon is clicked. What do you think ?

[majkers]

Confirmation

But you will still need info from p-dialog that close button was clicked. Relying on visible isn't the best way. So maybe You should add output for close button in p-dialog and then simply "subscribe" to it in confirmdialog and fire "old" (before Your renaming to _rejectOnClose) close method from p-confirmdialog. No need to change Confirmation IMHO and it will work as it used to in primeNg 17. Ie. Accept and Reject methods only. Reject with eventType.
BTW I looked at ConfirmationService.....it is really nasty. One method is callled onAccept with 'on' prefix others are Close and Confirm without this prefix. One observable has $ in its name and other doesn't .... :/

[Bock92]

Confirmation

But you will still need info from p-dialog that close button was clicked. Relying on visible isn't the best way. So maybe You should add output for close button in p-dialog and then simply "subscribe" to it in confirmdialog and fire "old" (before Your renaming to _rejectOnClose) close method from p-confirmdialog. No need to change Confirmation IMHO and it will work as it used to in primeNg 17. Ie. Accept and Reject methods only. Reject with eventType. BTW I looked at ConfirmationService.....it is really nasty. One method is callled onAccept with 'on' prefix others are Close and Confirm without this prefix. One observable has $ in its name and other doesn't .... :/

Confirmation

But you will still need info from p-dialog that close button was clicked. Relying on visible isn't the best way. So maybe You should add output for close button in p-dialog and then simply "subscribe" to it in confirmdialog and fire "old" (before Your renaming to _rejectOnClose) close method from p-confirmdialog. No need to change Confirmation IMHO and it will work as it used to in primeNg 17. Ie. Accept and Reject methods only. Reject with eventType. BTW I looked at ConfirmationService.....it is really nasty. One method is callled onAccept with 'on' prefix others are Close and Confirm without this prefix. One observable has $ in its name and other doesn't .... :/

I will review the implementation in PrimeNG 17, as I started using it from version 18. Once I have more details, I will update the Pull Request accordingly.

I understand that the code may be a bit complex, but having some example snippets would have been helpful.

[majkers]

I will tell You that it might be a hard task because confirmDialog didn't use dialog internally in v17 I guess. Like I told You to make it transparent for > v17 users and for those who migrated from v17 it would be the best to add eventEmitter to close button from dialog and use it in confirmDialog