fix(cdk/overlay): ensure error isn't thrown when many overlay items are closed from handler #20377
Conversation
googlebot
added
the
cla: yes
| const backgroundElement = document.createElement('div'); | ||
| document.body.appendChild(backgroundElement); | ||
|
|
||
| expect(fakeAsync(() => backgroundElement.click())).not.toThrowError(); |
There was a problem hiding this comment.
Isn't the fakeAsync supposed to be around the entire test?
| // We copy the array because the original may be modified asynchronously if the | ||
| // outsidePointerEvents listener decides to detach overlays resulting in index errors inside | ||
| // the for loop. | ||
| const overlays = this._attachedOverlays.slice(); |
There was a problem hiding this comment.
This would prevent the error, but doesn't it have the potential of firing off the event for an overlay that's already closed? It may not be a problem in problem in practice since the Subject gets completed, but it still seems smelly.
Looking through the loop again, shouldn't we be breaking off as soon as we find an overlay to emit to? @jelbourn I was under the impression that we want to emit to the top overlay that has subscribers since that's how the keyboard dispatcher works too, but this will emit to multiple overlays. I think the OverlayKeyboardDispatcher is doing it correctly.
It isn't something changed by this PR, but we may have to address it at some point before people start depending on it.
There was a problem hiding this comment.
It may fire off on an already closed overlay. An alternative to this would be to also check overlayRef.hasAttached() and continue to the next one if it has nothing. Or modifying the loop to consider mutations to the list when decrementing the counter and not copying the list - but this didn't seem ideal either.
There was a problem hiding this comment.
I'm not sure about stopping at the first overlay (worth a follow-up discussion), but it does seem that we should check that the overlay is not already closed before emitting.
There was a problem hiding this comment.
IMO we should be doing it, because the primary use case for this dispatcher is closing when the user clicks outside of an overlay. If we were to dispatch to all overlays, it would mean that when the user has multiple dialogs open at the same time, clicking on the top one will close everything else.
There was a problem hiding this comment.
I suspect different use-cases would call for one or the other
andy9775
force-pushed
the
cdk-overlay-fix-for-loop
branch
from
5040480 to
7bad7a3
Compare
|
|
||
| expect(spy).toHaveBeenCalled(); | ||
|
|
||
| backgroundElement.parentNode!.removeChild(backgroundElement); |
There was a problem hiding this comment.
Should this go in an afterEach block in a separate describe block, if it needs to be run even if the test fails?
There was a problem hiding this comment.
The jasmine tests don't exit on a failed expect so the closing logic will run regardless.
jelbourn
added
action: merge
andy9775
force-pushed
the
cdk-overlay-fix-for-loop
branch
from
7bad7a3 to
80e6254
Compare
…re closed from handler When an event handler listening to the `outsidePointerEvents` observable closes out any open overlays this removes them from the OverlayOutsideClickDispatcher's array of overlay elements. This results in an error being thrown since the overlays array in the for-loop is modified (elements are removed) and the counter not accounting for this. This fix ensures that if the array of overlay refs is modified we don't get an off by 1 error when fetching an element from the array on subsequent iterations of the loop.
andy9775
force-pushed
the
cdk-overlay-fix-for-loop
branch
from
80e6254 to
6bf0c04
Compare
…re closed from handler (#20377) When an event handler listening to the `outsidePointerEvents` observable closes out any open overlays this removes them from the OverlayOutsideClickDispatcher's array of overlay elements. This results in an error being thrown since the overlays array in the for-loop is modified (elements are removed) and the counter not accounting for this. This fix ensures that if the array of overlay refs is modified we don't get an off by 1 error when fetching an element from the array on subsequent iterations of the loop. (cherry picked from commit 1d0e3c7)
…re closed from handler (angular#20377) When an event handler listening to the `outsidePointerEvents` observable closes out any open overlays this removes them from the OverlayOutsideClickDispatcher's array of overlay elements. This results in an error being thrown since the overlays array in the for-loop is modified (elements are removed) and the counter not accounting for this. This fix ensures that if the array of overlay refs is modified we don't get an off by 1 error when fetching an element from the array on subsequent iterations of the loop.
…re closed from handler (#20377) When an event handler listening to the `outsidePointerEvents` observable closes out any open overlays this removes them from the OverlayOutsideClickDispatcher's array of overlay elements. This results in an error being thrown since the overlays array in the for-loop is modified (elements are removed) and the counter not accounting for this. This fix ensures that if the array of overlay refs is modified we don't get an off by 1 error when fetching an element from the array on subsequent iterations of the loop.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When an event handler listening to the
outsidePointerEventsobservable closes out any openoverlays this removes them from the OverlayOutsideClickDispatcher's array of overlay elements. This
results in an error being thrown since the overlays array in the for-loop is modified (elements are
removed) and the counter not accounting for this. This fix ensures that if the array of overlay
refs is modified we don't get an off by 1 error when fetching an element from the array on
subsequent iterations of the loop.