Issues with multiple dialog boxes using showDialog

[amyy54]

I was doing some work with importing comparisons other than the Personal Best comparison (LiveSplit/livesplit-core#889) and came across an issue when attempting to display multiple dialog boxes with showDialog right after each other.

In src/ui/views/RunEditor.tsx, I attempted to add another dialog box to the importComparison function like so:

const [dialogResult, runComparisonName] = await showDialog({
    title: "Import Comparison",
    description: "Specify the name of the comparison you want to import:",
    textInput: true,
    buttons: ["Ok", "Cancel"],
    defaultText: file.name.replace(/\.[^/.]+$/, ""),
});
if (dialogResult !== 0) {
    return;
}
const [dialogResult2, comparisonName] = await showDialog({
    title: "Import Comparison",
    description: "Specify the name the comparison should be saved as:",
    textInput: true,
    buttons: ["Import", "Cancel"],
    defaultText: file.name.replace(/\.[^/.]+$/, ""),
});
if (dialogResult2 !== 0) {
    return;
}

After doing this, I noticed that—while both dialog boxes would appear one after the other as expected—the second dialog box would handle its result before it was closed. That is, dialogResult2 would have the value true and hit the if (dialogResult2 !== 0) { line as soon as the first dialog box was completed.

Out of curiosity about it being a possible race condition, I added setTimeout around the second dialog box (and all subsequent code) and found that it would work perfectly fine as expected. See the code below:

const [dialogResult, runComparisonName] = await showDialog({
    title: "Import Comparison",
    description: "Specify the name of the comparison you want to import:",
    textInput: true,
    buttons: ["Ok", "Cancel"],
    defaultText: file.name.replace(/\.[^/.]+$/, ""),
});
if (dialogResult !== 0) {
    return;
}
setTimeout(async () => {
    const [dialogResult2, comparisonName] = await showDialog({
        title: "Import Comparison",
        description: "Specify the name the comparison should be saved as:",
        textInput: true,
        buttons: ["Import", "Cancel"],
        defaultText: file.name.replace(/\.[^/.]+$/, ""),
    });
    console.log(dialogResult2);
    if (dialogResult2 !== 0) {
        return;
    }
    const valid = editor.importComparisonAsComparison(run, comparisonName, runComparisonName);
    if (!valid) {
        toast.error(
            "The comparison could not be added. It may be a duplicate or a reserved name.",
        );
    } else {
        update();
    }
}, 0)

I couldn't make anything of the showDialog function when trying to identify the issue, so unfortunately could not continue without adding setTimeout, which should not be the fix I go with for what I'm trying to do.

[hallcristobal]

Explanation of the Problem

This is an issue regarding how scheduling works in Javascript, because we're calling dialogElement?.close(); in handleClose, this enqueues the handler for dialogElement.onclose that is set up in the showDialog setup function.

This effectively makes the scheduler queue (heavily simplified) at that point, when the button is pressed:

resolveFn
showDialog
dialog.onclose

Meaning that by the time the dialogElement.onclose handler is called, we've already set up the singleton-dialog element to reference the next stuff - thus instantly calling close on the second one.

Example

You can see/run a very simplified example of how this works in the following code snippet, and looking at the console log:

let resolveFn;
let alreadyClosed = false;

const dialog = document.createElement("dialog");
document.body.append(dialog);
const button1 = document.createElement("button");

button1.onclick = (ev) => {
  console.log("bclick");
  alreadyClosed = true;
  dialog?.close();
  resolveFn?.(`button resolve called - ${button1.innerText}`);
};
dialog.append(button1);

function showButton(text) {
  dialog.showModal();
  alreadyClosed = false;
  dialog.setAttribute("disabled", "");
  dialog.onclose = () => {
    if (!alreadyClosed) {
      alreadyClosed = true;
      console.log("dclose");
      resolveFn?.(`dialog resolve called - ${text}`);
    }
  }
  button1.innerText = text;
  return new Promise((resolve) => {
    console.log("setresolve");
    resolveFn = resolve;
  });
}

(async () => {
  let button1 = await showButton("Button 1");
  console.log(button1);
  let button2 = await showButton("Button 2");
  console.log(button2);
  // let button3 = await showButton("Button 3");
  // console.log(button3);
})()
.then(() => console.log("Done."))
.catch(console.error);

For extra fun, you can notice if you uncomment the button 3 i added there, the script will just not even show the button2, and skip straight from 1 to 3!

So how do we fix this?

Basically, we have to move the functionality out of dialogElement.onclose and make it a method that can be called immediately from the DialogContainer's handleClose method.

This will probably look something like

1. changing handleClose from:

    const handleClose = (i: number) => {
        alreadyClosed = true;
        dialogElement?.close();
        resolveFn?.([i, input]);
        onClose();
    };

to

    const handleClose = (i: number) => {
        alreadyClosed = true;
        closeDialogElement();
        resolveFn?.([i, input]);
        onClose();
    };

2. Removing setting the handler in the showDialog function:

        dialogElement.onclose = () => {
            if (!alreadyClosed) {
                resolveFn?.([closeWith, ""]);
                onCloseFn?.();
            }
        };

3. Adding a top level closeDialogElement() function that can be queued correctly:

    const closeDialogElement = () => {
        dialogElement?.close();
        if (!alreadyClosed) {
            resolveFn?.([closeWith, ""]);
            onCloseFn?.();
        }
    }

This was really fun to diagnose, and I love random javascript quirks like this. Though, I kind of knew this is what was happening as soon as you said adding a setTimeout of 0 "fixed" it