Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Commit

Permalink
Protect against infinite recursion in focusOverlay.
Browse files Browse the repository at this point in the history
An easy way to repro the infinite recursion is to create multiple _opened_
overlays, and dismiss them at the same time (toasts).

Here's what's happening. Say there are two overlays (A, and B), that were
dismissed at the same time - and that their transitions end within the same
run loop tick:

 * `A.transitionend`:
   * Removed from `overlays`.
   * Calls `applyFocus`, which calls `focusOverlay`.
 * `focusOverlay` calls `B.applyFocus`:
   * `B.closed == false`, so `focusOverlay` is called again.
   * ...recurse.

The main problem is that the overlay's state (`opened`) is set before the
transition completes; but `focusOverlay` relies on it to determine what
to do next. So, we defer work until after transitions are complete.
  • Loading branch information
Ian MacLeod committed Jun 16, 2014
1 parent 316c491 commit 2e88d17
Showing 1 changed file with 16 additions and 2 deletions.
18 changes: 16 additions & 2 deletions core-overlay.html
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ <h2>Dialog</h2>
},

openedChanged: function() {
this.transitioning = true;
this.ensureTargetSetup();
this.prepareRenderOpened();
// continue styling after delay so display state can change
Expand Down Expand Up @@ -371,6 +372,7 @@ <h2>Dialog</h2>
if (e && e.target !== this.target) {
return;
}
this.transitioning = false;
if (!this.opened) {
this.resetTargetDimensions();
this.target.style.display = 'none';
Expand Down Expand Up @@ -437,7 +439,11 @@ <h2>Dialog</h2>
focusNode.focus();
} else {
focusNode.blur();
focusOverlay();
if (currentOverlay() == this) {
console.warn('Current core-overlay is attempting to focus itself as next! (bug)');
} else {
focusOverlay();
}
}
},

Expand Down Expand Up @@ -637,7 +643,15 @@ <h2>Dialog</h2>

function focusOverlay() {
var current = currentOverlay();
if (current) {
// We have to be careful to focus the next overlay _after_ any current
// transitions are complete (due to the state being toggled prior to the
// transition). Otherwise, we risk infinite recursion when a transitioning
// (closed) overlay becomes the current overlay.
//
// NOTE: We make the assumption that any overlay that completes a transition
// will call into focusOverlay to kick the process back off. Currently:
// transitionend -> applyFocus -> focusOverlay.
if (current && !current.transitioning) {
current.applyFocus();
}
}
Expand Down

0 comments on commit 2e88d17

Please sign in to comment.