Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

calcite-alert: queue corruption after removal of visible alert #6616

Closed
timmorey opened this issue Mar 20, 2023 · 3 comments
Closed

calcite-alert: queue corruption after removal of visible alert #6616

timmorey opened this issue Mar 20, 2023 · 3 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 3 A day or two of work, likely requires updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library

Comments

@timmorey
Copy link
Contributor

Actual Behavior

If there is a queue of <calcite-alert>s, and the first alert is forcibly removed from the DOM without being properly dismissed, the alert queue will become corrupt and future alerts will not be rendered.

Expected Behavior

The alert queue should handle removal of alerts from the DOM.

With dynamic rendering in modern apps, it is quite possible that an alert may become severed from the document before it has been properly closed by the user. While we may recommend that alerts be properly closed and allowed to animate out before they are removed from the DOM, we should be tolerant of cases where they are not, and we should attempt to keep internal state consistent to allow future alerts to render.

Reproduction Sample

https://codepen.io/timmorey/pen/yLxqdoE

Reproduction Steps

  1. Click "Add alert" twice
  2. Observe that "alert 0" is visible, and that there is one more alert queued.
  3. Click "Remove alert" once
  4. Observe that no alerts are visible.

The expected behavior is that "alert 1" should be visible at step 4. If you inspect the dom at this time, you'll see that the second alert remains queued.

Reproduction Version

1.1.0

Relevant Info

No response

Regression?

No response

Priority impact

p3 - want for upcoming milestone

Impact

We are building components to be shared with other teams, and sometimes these components want to show alerts to the user to indicate an error condition or report the success of an operation. We would like to declaratively render these alerts in the components that trigger them, rather than insist that the parent application manage the alerts. However, we cannot guarantee that all alerts will have been gracefully dismissed by the time our components are destroyed, and we don't want to break the alert queue for the parent application.

Esri team

ArcGIS Field Apps

@timmorey timmorey added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Mar 20, 2023
@github-actions github-actions bot added p3 - want for upcoming milestone ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. labels Mar 20, 2023
@nwhittaker
Copy link
Contributor

This also has impact on modals that create alerts. The alert is added to the "content" slot so it layers above its corresponding modal and is dismissed when the modal is closed.

This is nice behavior, but can require convoluted workarounds to avoid this queue bug by making sure the alert is closed before the modal closes.

@geospatialem geospatialem added p - medium Issue is non core or affecting less that 60% of people using the library estimate - 3 A day or two of work, likely requires updates to tests. labels May 2, 2023
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label May 2, 2023
@macandcheese macandcheese removed their assignment Jun 9, 2023
@driskull driskull self-assigned this Jun 16, 2023
@driskull driskull added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Jun 16, 2023
driskull added a commit that referenced this issue Jun 21, 2023
…7189)

**Related Issue:** #6616

## Summary

- Adds new internal event `calciteInternalAlertUnregister` to dispatch
when an alert is removed from the DOM.
- Event is dispatched on the window since that is where the listeners
are and dispatching on the element won't reach the window since the
element would no longer be in the DOM.
- When the event is received, the queue is updated to remove the removed
alert element.
- queue is kept n'sync.


![giphy](https://github.com/Esri/calcite-components/assets/1231455/691e1744-6045-48b2-8e55-97479a2b8a5b)
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Jun 21, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jun 26, 2023
@geospatialem
Copy link
Member

Verified with 1.5.0-next.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 3 A day or two of work, likely requires updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library
Projects
None yet
Development

No branches or pull requests

5 participants