GoogleChrome / dialog-polyfill

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

Issue with IE9 and IE10 - not shown as an overlay #150

Closed craigfrancis closed 7 years ago

craigfrancis commented 7 years ago

If the dialog contains a <h1>, and you change it's contents before dialog.showModal(), it causes IE9 and IE10 to incorrectly display the dialog (and the backdrop).

I've attached a zip containing a basic HTML and JS file, with the latest polyfill.js and polyfill.css files.

For some reason, it starts working as soon as you comment out the line:

ref_heading.textContent = 'Test';

example.zip


Broken, with the content shown inline, and no backdrop...

Before


Working, without the heading content changing...

After

craigfrancis commented 7 years ago

This happens because downgradeModal() is called from dialog-polyfill.js (line 126).

var removed = false;
var cb = function() {
    removed ? this.downgradeModal() : this.maybeHideModal();
    removed = false;
}.bind(this);
var timeout;
var delayModel = function(ev) {
    var cand = 'DOMNodeRemoved';
    removed |= (ev.type.substr(0, cand.length) === cand);
    window.clearTimeout(timeout);
    timeout = window.setTimeout(cb, 0);
};
['DOMAttrModified', 'DOMNodeRemoved', 'DOMNodeRemovedFromDocument'].forEach(function(name) {
    dialog.addEventListener(name, delayModel);
});

Where a DOMNodeRemoved event fires when changing the textContent twice, or simply removing an element from the <dialog> element (e.g. when changing it's content):

<div id="dialog"><h1 id="title"></h1></div>

<script type="text/javascript">

    var dialog = document.getElementById('dialog'),
        title = document.getElementById('title');

    dialog.addEventListener('DOMNodeRemoved', function(e) {
            console.log(e.type);
        });

    title.textContent = 'A';
    title.textContent = 'B'; // DOMNodeRemoved 1

    title.parentNode.removeChild(title); // DOMNodeRemoved 2

</script>

Do you know why downgradeModal() is used instead of maybeHideModal() when a DOMNodeRemoved or DOMNodeRemovedFromDocument event is fired?

Maybe you are looking for the <dialog> being removed from the DOM, rather than one of it's children? In which case, would it be worth checking the ev.target?

var delayModel = function(ev) {
    if (ev.target !== dialog) return;
    var cand = 'DOMNodeRemoved';
...
samthor commented 7 years ago

Yup, it seems like the bug is that I assumed DOMNodeRemoved would only fire for the target element. I only vaguely remember writing this code and machines running IE10 are hard to come by :)

I'm not sure if this is clear, but I will clarify: the reason downgradeModal is called is because if a modally shown dialog is removed from the document, it's still technically open, just now not in a modal way.

craigfrancis commented 7 years ago

Thanks @samthor, in which case I think you just need to add the following to delayModel:

if (ev.target !== dialog) return;

And yes, IE9/10 is being used less, but unfortunately I still need to support them (I've still got some people using IE7, but they can sort them themselves out).

samthor commented 7 years ago

If your PR fixes the issue, I'll do a release. Let me know.

craigfrancis commented 7 years ago

It fixes my issue, and works in the 3 locations I'm using it...

But I'm not 100% sure that it won't break something else (as in, that code shouldn't be needed for any of my projects, as I don't remove the <dialog> element from the DOM).