search

GoogleChrome / dialog-polyfill

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

Infinite loop when shift-tabbing in a modal dialog in Internet Explorer #166

Closed jessevanassen closed 4 years ago

jessevanassen commented 6 years ago

When shift-tab is being pressed from within a modal dialog, the polyfill gets into an infinite loop, which completely freezes the Internet Explorer and requiring a browser restart.

Reproduction recipe:

  1. In Internet Explorer (11), navigate to https://demo.agektmr.com/dialog/#showmodal. Because IE11 doesn't support <dialog> natively, the polyfill will be used.
  2. Click the "Open modal dialog" button in the ".showModal() API" section.
  3. Press Shift + TAB. The browser will scroll to the top of the document, and become unresponsive.

The culprit is the handleFocus_ function. If this.forwardTab_ is false (which it is in case of shift-tab), it will call document.documentElement.focus(). This is in turn being picked up by document.documentElement.addEventListener('focus', this.handleFocus_, true);, which calls this.handleFocus_, resulting in an infinite loop.

Edge seems to be unaffected though, because document.documentElement.addEventListener('focus', this.handleFocus_, true); won't actually fire when shift tabbing, I suspect that's because shift-tabbing might put the focus outside of the document.documentElement.

To show this in action, you can replace document.documentElement.focus() by 

        setTimeout(function() {
          console.log("Focusing the documentElement");
          document.documentElement.focus();
        }, 0);

This will fill up the console log without crashing the browser.

A possible solution could be to insert this.forwardTab_ = undefined; right before calling document.documentElement.focus(). This is already done in the keydown listener or when clicking on the overlay. This will result in the focus listener only firing twice: once originating from the focus shift from actually pressing the tab key, and once from the call to document.documentElement.focus(). In the second call, if (this.forwardTab_ === undefined) { return; } // move focus only from a tab key will kill the loop.

samthor commented 6 years ago

Thanks for the detailed writeup. I'll look at this soon.

LoganDark commented 6 years ago

29 days ago

samthor commented 6 years ago

I don't think setting forwardTab_ = undefined makes sense, because a user might be shift-tabbing through the page, although I suppose the dialog blocks anything else from being tabbed.

FYI we can reproduce this in Safari by adding tabindex="0" to the <html> element. IE must just have that behavior by default (and I don't have a Windows machine that handy). I'll have a fix soon.

samthor commented 6 years ago

Can you test the solution I've pushed in this commit? I'll do a release if it works for you.