ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

fix(overlays): dismissed overlays will resolve in the next frame

Open sean-perkins opened this issue 1 year ago • 0 comments

Pull request checklist

Please check if your PR fulfills the following requirements:

  • [ ] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • [x] Build (npm run build) was run locally and any changes were pushed
  • [x] Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • [x] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Documentation content changes
  • [ ] Other (please describe):

What is the current behavior?

With a very specific configuration of presenting and dismissing overlays, occasionally getTop() will return the overlay that was just dismissed. When used with recursion (where you are trying to dismiss all overlays until there is no more), this creates problems/infinite loops.

Issue URL: #25447

What is the new behavior?

  • The dismiss() API will resolve in the next frame, after the element has completed the transition and been removed from the DOM.
  • This improves the consistency of getTop() to return the active visible, top overlay.

Does this introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

You can verify this behavior in this reproduction app: https://github.com/sean-perkins/gh-25447-overlay-get-top it is a fork of the original reproduction, with safe guards to avoid an infinite loop locking up the browser tab.

The main branch is on 6.2.0 and will highlight the problem in console (the infinite loop occurs, but bails with an error).

You can install the dev-build and see this behavior does not happen and the loading indicator properly dismisses.

Dev-build: 6.2.2-dev.11659644639.1b018cb5

Writing a test case around this problem was unsuccessful (without copying all the bolierplate from the reproduction app into our test app). I do not believe the value of covering this test condition is worth maintaining all the boilerplate.

sean-perkins avatar Aug 04 '22 20:08 sean-perkins