remove disconnected autoPopovers during topMostPopoverAncestor algo #99
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for popover-polyfill ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
keithamus
force-pushed
the
remove-disconnected-autopopovers-during-topmostpopoverancestor-algo
branch
from
b997ec3 to
478e000
Compare
|
@jgerigmeyer could we get this one out as a |
…ostpopoverancestor-algo * main: chore(deps): Automated dependency upgrades
jgerigmeyer
deleted the
remove-disconnected-autopopovers-during-topmostpopoverancestor-algo
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this change, a popover removed from the document would still be in the autoPopovers set, and subsequently
topMostPopverAncestorwould return disconnected elements, causingcloseAllPopoversto hit an infinite loop:hidePopoveris calledcloseAllPopoversis calledtopMostPopoverAncestoris called and returns disconnected element.hidePopover(topMost)is called but returns false, as the element is disconnected.topMostPopoverAncestoris called, but it returns the same element. Repeat step 4, infinite loop.This commit alters
topMostAutoPopoverto check if the element is disconnected, and remove it from the set if so, only returning the first connected element.hidePopoveris calledcloseAllPopoversis calledtopMostPopoverAncestoris called, the first element is disconnected, so it skips and returns the first connected element.hidePopover(topMost)is called and returns true.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!