Modal may run openModal after component is already disconnected leaving state in the DOM

[jkieboom]

Actual Behavior

Because calling openModal is async, it is possible for the component to become disconnected before openModal is actually run. In this case openModal will leave side effects in the DOM that are not cleaned up. In particular it leaves the overflow-hidden CSS class on the html node.

Expected Behavior

The modal component does not produce any side effects once disconnected.

Reproduction Sample

https://codepen.io/jkieboom/pen/NWLYgRe

Reproduction Steps

1. Let codepen load
2. Message in document should indicate that html element no longer has overflow-hidden CSS class (but does)
3. Can also be inspected through devtools on the html element

Reproduction Version

@latest

Relevant Info

No response

Regression?

No response

Priority impact

p3 - not time sensitive

Impact

This was uncovered in our CI because it left state that affected other tests.

Esri team

ArcGIS Maps SDK for JavaScript

[driskull]

Trying to get a fix for this but I'm seeing a bug with stencil. ionic-team/stencil#4070

If a component is removed right after its added it doesn't seem to call the disconnectedCallback. This is kind of a bad bug because it leaves listeners connected. cc @jcfranco

This fix can benefit from use of an AbortController to abort opening the modal after requestAnimationFrame if the controller was aborted in disconnectedCallback. But since the disconnectedCallback bug exists we can't really clean stuff up on disconnectedCallback.

I was thinking we should move the initial open logic into connectedCallback as well because it seems like openModal should be called if the component is moved in the DOM too, not just on first load.

[jcfranco]

@driskull Thanks for digging. That bug is quite unfortunate. I'm afraid we can't do anything regarding that issue until it's fixed and we bump Stencil.

Regarding the issue at hand, I did notice that the repro case behaves differently depending on the output target used:

• dist (lazy-loaded) - does not call the overridden componentDidLoad, but does remove the util class after the initial message is logged (see https://codepen.io/jcfranco/pen/JjeNpBL).
• components – the overridden componentDidLoad is called, but throws errors due to it clobbering the original lifecycle method and the util class is also removed from the document after the initial message is logged (same as the codepen above).

@jkieboom Could you check if this is also happening in your testing environment? If not, would it be possible to get an updated repro case to better reflect the issue? LMK if we can help here.

I was thinking we should move the initial open logic into connectedCallback as well because it seems like openModal should be called if the component is moved in the DOM too, not just on first load.

IIRC, after initial load, openModal is then called by the open watcher, so this may not be necessary. I could be missing something, so please let me know if you're noticing inconsistent behavior here.

[jkieboom]

We don't actually override componentDidLoad in our testing environment. It was just a way to get kind of the same timing behavior in the repro. You can just chain up the original componentDidLoad in the repro to avoid clobbering the lifecycle? It's hard to give a robust repro case for this because it's a race condition and depends on timing.

[jcfranco]

Gotcha. I'll reach out to get a closer look at the test failures.

Hope we can find a repro case. Otherwise, we'll have to put this in the back burner until the Stencil lifecycle issue is fixed.

[jcfranco]

[belated] @jkieboom provided an updated (internal) repro case for this. He also confirmed it's not critical, so we'll be moving this to a later milestone (after the Stencil v4 update as the disconnectedCallback was recently fixed).

[jcfranco]

Tested locally, and this is still an issue w/ [email protected] (without any specific fixes for this).

[jcfranco]

Narrowed down the issue to this line. We store the initial overflow style to restore it when modal is closed, which is only correct for the first modal that opens. Subsequent ones will store the style that the modal applies, hence why overflow: hidden lingers after all modals are removed in a different order. I managed to create a repro case showing this.

[driskull]

Makes total sense! nice find

[driskull]

Sounds like a registry of modals could handle this.

[github-actions]

Installed and assigned for verification.

[DitwanP]

🍡 Verified locally on main