search

oddbird / popover-polyfill

Polyfills the HTML popover attribute and showPopover/hidePopover/togglePopover methods onto HTMLElement, as well as the popovertarget and popovertargetaction attributes on <button> elements.
https://popover.oddbird.net
BSD 3-Clause "New" or "Revised" License
269 stars 14 forks source link

remove disconnected autoPopovers during topMostPopoverAncestor algo #99

Closed keithamus closed 1 year ago

keithamus commented 1 year ago

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.

Steps to test/reproduce

Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review). I've added a new test file "edge-cases.spec". This includes a test which fails on the current build (times out due to the browser hanging), but passes with the changes.

Show me

Provide screenshots/animated gifs/videos if necessary.

Special thanks to @hmaurer for giving me an easy repro to this one!

netlify[bot] commented 1 year ago

Deploy Preview for popover-polyfill ready!

Name Link
Latest commit 5a4486feeea0e3b195ab8c09a1c82eb60bbd1065
Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/64658cbff9ad58000810a7fa
Deploy Preview https://deploy-preview-99--popover-polyfill.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

keithamus commented 1 year ago

@jgerigmeyer could we get this one out as a 0.2.1 release please?