Merge pull request #2520 from SillyTavern/popup-onclosing-fix Fix popup onClosing handling not working correctly
Signed| @@ -135,6 +135,7 @@ export class Popup { | ||
| 135 | 135 | |
| 136 | 136 | /** @type {Promise<any>} */ #promise; |
| 137 | 137 | /** @type {(result: any) => any} */ #resolver; |
| 138 | + /** @type {boolean} */ #isClosingPrevented; | |
| 138 | 139 | |
| 139 | 140 | /** |
| 140 | 141 | * Constructs a new Popup object with the given text content, type, inputValue, and options |
| @@ -334,10 +335,21 @@ export class Popup { | ||
| 334 | 335 | evt.preventDefault(); |
| 335 | 336 | evt.stopPropagation(); |
| 336 | 337 | await this.complete(POPUP_RESULT.CANCELLED); |
| 337 | - window.removeEventListener('cancel', cancelListenerBound); | |
| 338 | 338 | }; |
| 339 | 339 | const cancelListenerBound =this.dlg.addEventListener('cancel', cancelListener.bind(this)); |
| 340 | - this.dlg.addEventListener('cancel', cancelListenerBound); | |
| 340 | + | |
| 341 | + // Don't ask me why this is needed. I don't get it. But we have to keep it. | |
| 342 | + // We make sure that the modal on it's own doesn't hide. Dunno why, if onClosing is triggered multiple times through the cancel event, and stopped | |
| 343 | + // It seems to just call 'close' on the dialog even if the 'cancel' event was prevented. | |
| 344 | + // Here, we just say that close should not happen if the dalog has no result. | |
| 345 | + const closeListener = async (evt) => { | |
| 346 | + if (this.#isClosingPrevented) { | |
| 347 | + evt.preventDefault(); | |
| 348 | + evt.stopPropagation(); | |
| 349 | + this.dlg.showModal(); | |
| 350 | + } | |
| 351 | + }; | |
| 352 | + this.dlg.addEventListener('close', closeListener.bind(this)); | |
| 341 | 353 | |
| 342 | 354 | const keyListener = async (evt) => { |
| 343 | 355 | switch (evt.key) { |
| @@ -366,16 +378,16 @@ export class Popup { | ||
| 366 | 378 | evt.preventDefault(); |
| 367 | 379 | evt.stopPropagation(); |
| 368 | 380 | const result = Number(document.activeElement.getAttribute('data-result') ?? this.defaultResult); |
| 381 | + | |
| 382 | + // Call complete on the popup. Make sure that we handle `onClosing` cancels correctly and don't remove the listener then. | |
| 369 | 383 | await this.complete(result); |
| 370 | - window.removeEventListener('keydown', keyListenerBound); | |
| 371 | 384 | |
| 372 | 385 | break; |
| 373 | 386 | } |
| 374 | 387 | } |
| 375 | 388 | |
| 376 | 389 | }; |
| 377 | 390 | const keyListenerBound =this.dlg.addEventListener('keydown', keyListener.bind(this)); |
| 378 | - this.dlg.addEventListener('keydown', keyListenerBound); | |
| 379 | 391 | } |
| 380 | 392 | |
| 381 | 393 | /** |
| @@ -445,9 +457,11 @@ export class Popup { | ||
| 445 | 457 | * - popup with `POPUP_TYPE.INPUT` will return the input value - or `false` on negative and `null` on cancelled |
| 446 | 458 | * - All other will return the result value as provided as `POPUP_RESULT` or a custom number value |
| 447 | 459 | * |
| 460 | + * <b>IMPORTANT:</b> If the popup closing was cancelled via the `onClosing` handler, the return value will be `Promise<undefined>`. | |
| 461 | + * | |
| 448 | 462 | * @param {POPUP_RESULT|number} result - The result of the popup (either an existing `POPUP_RESULT` or a custom result value) |
| 449 | 463 | * |
| 450 | 464 | * @returns {Promise<string|number|boolean|undefined?>} A promise that resolves with the value of the popup when it is completed. <b>Returns `undefined` if the closing action was cancelled.</b> |
| 451 | 465 | */ |
| 452 | 466 | async complete(result) { |
| 453 | 467 | // In all cases besides INPUT the popup value should be the result |
| @@ -481,8 +495,16 @@ export class Popup { | ||
| 481 | 495 | |
| 482 | 496 | if (this.onClosing) { |
| 483 | 497 | const shouldClose = this.onClosing(this); |
| 484 | 498 | if (!shouldClose) return;{ |
| 499 | + this.#isClosingPrevented = true; | |
| 500 | + // Set values back if we cancel out of closing the popup | |
| 501 | + this.value = undefined; | |
| 502 | + this.result = undefined; | |
| 503 | + this.inputResults = undefined; | |
| 504 | + return undefined; | |
| 505 | + } | |
| 485 | 506 | } |
| 507 | + this.#isClosingPrevented = false; | |
| 486 | 508 | |
| 487 | 509 | Popup.util.lastResult = { value, result, inputResults: this.inputResults }; |
| 488 | 510 | this.#hide(); |