-
Notifications
You must be signed in to change notification settings - Fork 50
[🧪] Testing possible fix for Modal
issues
#2961
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
41b900e
to
86439c9
Compare
super(owner, args); | ||
|
||
registerDestructor(this, (): void => { | ||
document.removeEventListener('click', this._clickHandler, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow all of the changes here, but why not do the style cleanup here and remove the use of will-destroy
render modifier?
(the destructor will run when the component is cleaned up / unrendered)
This can be closed now. We may use it as reference for the refactoring of the |
📌 Summary
This is a branch that is used to test a possible fix for the issue of a
Modal
being closed not by using theF.close
method, but by simply removing it from the DOM.With the current logic, there's no cleanup (the
willDestroyNode
is invoked but the cleanup is done in theonDismiss
function).This PR does multiple things:
Modal
backing class, so we can see the order or execution and which functions are called (and their nested invocation)onDeactivate=this.onManualDismiss
from thefocus-trap
options, because it was invoking the "manual" dismiss even with a non-manual closure, or otherwise it caused the onDismiss to be called twice (see below)The idea is to:
📸 Screenshots
Difference in Atlas, between when the modal is closed via dismiss vs submit

Video of the fix working:
https://github.com/user-attachments/assets/f81e2e6f-7806-4731-b518-91e29eaabcb8
Reason why the

onDeactivate=this.onManualDismiss
has been removed from thefocus-trap
options (see red areas):🔗 External links
Jira ticket: https://hashicorp.atlassian.net/browse/HDS-4975
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.