Skip to content

Commit

Permalink
remove disconnected autoPopovers during topMostPopoverAncestor algo
Browse files Browse the repository at this point in the history
Prior to this change, a popover removed from the document would still be
in the autoPopovers set, and subsequently `topMostPopverAncestor` would
return disconnected elements, causing `closeAllPopovers` to hit an
infinite loop:

1. `hidePopover` is called
2. `closeAllPopovers` is called
3. `topMostPopoverAncestor` is called and returns disconnected element.
4. `hidePopover(topMost)` is called but returns false, as the element is
   disconnected.
5. `topMostPopoverAncestor` is called, but it returns the same element.
   Repeat step 4, infinite loop.

This commit alters `topMostAutoPopover` to check if the element is
disconnected, and remove it from the set if so, only returning the first
connected element.

1. `hidePopover` is called
2. `closeAllPopovers` is called
3. `topMostPopoverAncestor` is called, the first element is
   disconnected, so it skips and returns the first connected element.
4. `hidePopover(topMost)` is called and returns true.
5. No infinite loop.
  • Loading branch information
keithamus committed May 17, 2023
1 parent a35096c commit b997ec3
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 1 deletion.
10 changes: 9 additions & 1 deletion src/popover-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,15 @@ function topMostClickedPopover(target: HTMLElement) {

// https://html.spec.whatwg.org/#topmost-auto-popover
function topMostAutoPopover(document: Document): HTMLElement | null {
return Array.from(autoPopoverList.get(document) || []).pop() || null;
const documentPopovers = autoPopoverList.get(document);
for (const popover of documentPopovers || []) {
if (!popover.isConnected) {
documentPopovers.delete(popover);
} else {
return popover;
}
}
return null;
}

// https://html.spec.whatwg.org/#nearest-inclusive-open-popover
Expand Down
22 changes: 22 additions & 0 deletions tests/edge-cases.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { expect, test } from '@playwright/test';

test.beforeEach(async ({ page }) => {
await page.goto('/');
});

test('removing an autoPopover from document doesnt crash page', async ({
page,
}) => {
const popover = (await page.locator('#popover1')).nth(0);
await expect(popover).toBeHidden();
await expect(
await popover.evaluate((node) => node.showPopover()),
).toBeUndefined();
await expect(popover).toBeVisible();
await expect(await popover.evaluate((node) => node.remove())).toBeUndefined();
const popover2 = (await page.locator('#popover2')).nth(0);
await expect(
await popover2.evaluate((node) => node.showPopover()),
).toBeUndefined();
await expect(popover2).toBeVisible();
});

0 comments on commit b997ec3

Please sign in to comment.