GoogleChrome / dialog-polyfill

Polyfill for the HTML dialog element
BSD 3-Clause "New" or "Revised" License
2.45k stars 245 forks source link

Closing a showModal with the Escape key throws an error #149

Closed fstorr closed 7 years ago

fstorr commented 7 years ago

This bug appears when showModal(); is used rather than show();.

Adding a keydown event listener to listen for the escape key causes an error when the Escape key is pressed and the dialog closes. For example:

document.addEventListener('keydown', keydownHandler);

function keydownHandler(event) {
  if (event.keyCode === 27) {
    dialog.close();
  }
}

causes Safari and Firefox (both latest on macOS) to error with the message:

"Error: Failed to execute 'close' on dialog: The element does not have an 'open' attribute, and therefore cannot be closed."

I've checked dev tools when the dialog is open and there's definitely an open attribute present when the dialog is opened.

marcysutton commented 7 years ago

Maybe it's a timing issue? Thanks for filing this @fstorr.

samthor commented 7 years ago

So it took me a second to understand this bug—because a dialog already closes itself when you hit Esc.

This is actually a bug in your code—sort of. Our global 'esc' handler runs before yours does (because it's added presumably before yours, although it's not clear why), so your call to .close is actually after the dialog is closed.

Chrome's implementation throws the error, so we match that. You could also use .setOpen(false), but it wouldn't stop that the polyfill already closes the dialog for you.

I hope that helps!

samthor commented 7 years ago

Closing as this is not an issue with the polyfill. :)

marcysutton commented 7 years ago

Ah, that's easy enough then. Just remove the call to dialog.close(). Thanks!

samthor commented 7 years ago

You could also set dialog.open = false, which will be safe if the dialog is already closed (although it won't emit a 'close' event that way).