• 
      

    Simplify dialog actions, support cancelation, and add close reasons.

    Review Request #14684 — Created Nov. 11, 2025 and submitted

    Information

    Ink
    master

    Reviewers

    DialogView lacked support for handling a cancel request from the
    browser, such as when the user hit the Escape key. The dialog would
    close with no feedback. This was a problem for the Confirm dialog,
    which never went through the cancel response flow.

    There's now explicit support for handling this and direct closing using
    the cancel and close events, along with new Cancel actions.

    When closing a dialog, or when receiving a cancel/close request, a Close
    Reason will be set. This contains a flag indicating if the dialog was
    canceled, along with optional fields for the action triggering the
    closure and any state returned from the action's click handler.

    To support that on the action side, we needed a Cancel action type. To
    avoid introducing yet type requiring a Callback variant, we're
    deprecating the *_CALLBACK versions of the types. These weren't really
    necessary, as they just determined whether to invoke the provided
    callback= argument.

    We now support CLOSE and CANCEL types, with CALLBACK and
    CALLBACK_AND_CLOSE being deprecated. A default of null is equivalent
    to CALLBACK, which invokes an action but does not close the dialog.
    The old types will convert to the new on load, but are not formally
    deprecated for now.

    If the cancel event is triggered on the element, and there's a
    Cancel action, it will be invoked directly, allowing any callback to
    be run.

    Tested each action type, new and old, and verified the right behavior.
    Tested both in Review Board and with the new story updates.

    Tested that hitting Escape closed the Confirm dialog, and triggered
    the Cancel dialog behavior.

    Summary ID
    Simplify dialog actions, support cancelation, and add close reasons.
    `DialogView` lacked support for handling a cancel request from the browser, such as when the user hit the Escape key. The dialog would close with no feedback. This was a problem for the Confirm dialog, which never went through the cancel response flow. There's now explicit support for handling this and direct closing using the `cancel` and `close` events, along with new Cancel actions. When closing a dialog, or when receiving a cancel/close request, a Close Reason will be set. This contains a flag indicating if the dialog was canceled, along with optional fields for the action triggering the closure and any state returned from the action's click handler. To support that on the action side, we needed a Cancel action type. To avoid introducing yet type requiring a Callback variant, we're deprecating the `*_CALLBACK` versions of the types. These weren't really necessary, as they just determined whether to invoke the provided `callback=` argument. We now support `CLOSE` and `CANCEL` types, with `CALLBACK` and `CALLBACK_AND_CLOSE` being deprecated. A default of `null` is equivalent to `CALLBACK`, which invokes an action but does not close the dialog. The old types will convert to the new on load, but are not formally deprecated for now.
    0b915bb554d0b7f2b7dd48b248e1ff23254caed8
    Description From Last Updated

    I don't think we gain any performance impact by assigning a new variable in order to use this.#callback once instead …

    daviddavid

    It seems like there's something missing here?

    daviddavid
    david
    1. 
        
    2. src/ink/js/components/views/dialogView.ts (Diff revision 1)
       
       
       
      Show all issues

      I don't think we gain any performance impact by assigning a new variable in order to use this.#callback once instead of twice. Can we just use this.#callback inline in here?

      1. It keeps the code a lot cleaner and mandates the thing we're calling is the thing we're checking.

    3. Oh neat.

    4. src/ink/js/components/views/dialogView.ts (Diff revision 1)
       
       
       
      Show all issues

      It seems like there's something missing here?

      1. It was meant to imply that it'll bubble up, but that's really not apparent from reading this. Reworking it.

    5. 
        
    chipx86
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (4b99f31)