fix(modal): handle removal of open attribute and prevent multiple beforeClose calls

packages/calcite-components/src/components/modal/modal.e2e.ts

@@ -109,7 +109,7 @@ describe("calcite-modal properties", () => {
     expect(styleH).toEqual("800px");
   });
 
-  it("calls the beforeClose method prior to closing", async () => {
+  it("calls the beforeClose method prior to closing via click", async () => {
     const page = await newE2EPage();
     const mockCallBack = jest.fn();
     await page.exposeFunction("beforeClose", mockCallBack);
@@ -123,14 +123,40 @@ describe("calcite-modal properties", () => {
         (elm.beforeClose = (window as typeof window & Pick<typeof elm, "beforeClose">).beforeClose)
     );
     await page.waitForChanges();
-    await modal.setProperty("open", true);
+    modal.setProperty("open", true);
     await page.waitForChanges();
-    await modal.setProperty("open", false);
+    expect(await modal.getProperty("opened")).toBe(true);
+    const closeButton = await page.find(`calcite-modal >>> .${CSS.close}`);
+    await closeButton.click();
     await page.waitForChanges();
-    expect(mockCallBack).toHaveBeenCalled();
+    expect(mockCallBack).toHaveBeenCalledTimes(1);
+    expect(await modal.getProperty("opened")).toBe(false);
   });
 });
 
+it("calls the beforeClose method prior to closing via attribute", async () => {
+  const page = await newE2EPage();
+  const mockCallBack = jest.fn();
+  await page.exposeFunction("beforeClose", mockCallBack);
+  await page.setContent(`
+    <calcite-modal open></calcite-modal>
+  `);
+  const modal = await page.find("calcite-modal");
+  await page.$eval(
+    "calcite-modal",
+    (elm: HTMLCalciteModalElement) =>
+      (elm.beforeClose = (window as typeof window & Pick<typeof elm, "beforeClose">).beforeClose)
+  );
+  await page.waitForChanges();
+  modal.setProperty("open", true);
+  await page.waitForChanges();
+  expect(await modal.getProperty("opened")).toBe(true);
+  modal.removeAttribute("open");
+  await page.waitForChanges();
+  expect(mockCallBack).toHaveBeenCalledTimes(1);
+  expect(await modal.getProperty("opened")).toBe(false);
+});
+
 describe("opening and closing behavior", () => {
   it("opens and closes", async () => {
     const page = await newE2EPage();

packages/calcite-components/src/components/modal/modal.scss

@@ -108,7 +108,7 @@
   }
 }
 
-:host([open]) {
+:host([opened]) {
   @apply opacity-100;
   visibility: visible !important;
   transition-delay: 0ms;
@@ -310,7 +310,7 @@ slot[name="primary"] {
   }
 }
 
-:host([open][fullscreen]) {
+:host([opened][fullscreen]) {
   .header,
   .footer,
   .content-top,

packages/calcite-components/src/components/modal/modal.tsx

@@ -87,8 +87,7 @@ export class Modal
   @Prop({ mutable: true, reflect: true }) open = false;
 
   /** Passes a function to run before the component closes. */
-  @Prop()
-  beforeClose: (el: HTMLElement) => Promise<void> = () => Promise.resolve();
+  @Prop() beforeClose: (el: HTMLCalciteModalElement) => Promise<void>;
 
   /** When `true`, disables the component's close button. */
   @Prop({ reflect: true }) closeButtonDisabled = false;
@@ -208,7 +207,7 @@ export class Modal
         <div
           class={{
             [CSS.container]: true,
-            [CSS.containerOpen]: this.isOpen,
+            [CSS.containerOpen]: this.opened,
             [CSS.slottedInShell]: this.slottedInShell,
           }}
         >
@@ -284,7 +283,7 @@ export class Modal
         aria-label={this.messages.close}
         class={CSS.close}
         key="button"
-        onClick={this.close}
+        onClick={this.closeModal}
         title={this.messages.close}
         // eslint-disable-next-line react/jsx-sort-props
         ref={(el) => (this.closeButtonEl = el)}
@@ -343,6 +342,15 @@ export class Modal
   //
   //--------------------------------------------------------------------------
 
+  /**
+   * We use an internal property to handle styles for when a modal is actually opened, not just when the open attribute is applied. This is a property because we need to apply styles to the host element and to keep the styles present while beforeClose is .
+   *
+   * @internal.
+   */
+  @Prop({ mutable: true, reflect: true }) opened = false;
+
+  ignoreOpenChange = false;
+
   @Element() el: HTMLCalciteModalElement;
 
   modalContent: HTMLDivElement;
@@ -379,13 +387,6 @@ export class Modal
 
   @State() hasContentBottom = false;
 
-  /**
-   * We use internal variable to make sure initially open modal can transition from closed state when rendered
-   *
-   * @private
-   */
-  @State() isOpen = false;
-
   @State() effectiveLocale: string;
 
   @Watch("effectiveLocale")
@@ -404,7 +405,7 @@ export class Modal
   @Listen("keydown", { target: "window" })
   handleEscape(event: KeyboardEvent): void {
     if (this.open && !this.escapeDisabled && event.key === "Escape" && !event.defaultPrevented) {
-      this.close();
+      this.closeModal();
       event.preventDefault();
     }
   }
@@ -508,7 +509,7 @@ export class Modal
       this.openModal();
     } else {
       this.transitionEl?.classList.add(CSS.closingIdle);
-      this.close();
+      this.closeModal();
     }
   }
 
@@ -519,9 +520,14 @@ export class Modal
 
   /** Open the modal */
   private openModal() {
+    if (this.ignoreOpenChange) {
+      return;
+    }
+
+    this.ignoreOpenChange = true;
     this.el.addEventListener("calciteModalOpen", this.openEnd);
     this.open = true;
-    this.isOpen = true;
+    this.opened = true;
     const titleEl = getSlotted(this.el, SLOTS.header);
     const contentEl = getSlotted(this.el, SLOTS.content);
 
@@ -533,23 +539,32 @@ export class Modal
       // use an inline style instead of a utility class to avoid global class declarations.
       document.documentElement.style.setProperty("overflow", "hidden");
     }
+    this.ignoreOpenChange = false;
   }
 
-  handleOutsideClose = (): void => {
+  private handleOutsideClose = (): void => {
     if (this.outsideCloseDisabled) {
       return;
     }
 
-    this.close();
+    this.closeModal();
   };
 
   /** Close the modal, first running the `beforeClose` method */
-  close = (): Promise<void> => {
-    return this.beforeClose(this.el).then(() => {
-      this.open = false;
-      this.isOpen = false;
-      this.removeOverflowHiddenClass();
-    });
+  closeModal = async (): Promise<void> => {
+    if (this.ignoreOpenChange) {
+      return;
+    }
+
+    if (this.beforeClose) {
+      await this.beforeClose(this.el);
+    }
+
+    this.ignoreOpenChange = true;
+    this.open = false;
+    this.opened = false;
+    this.removeOverflowHiddenClass();
+    this.ignoreOpenChange = false;
   };
 
   private removeOverflowHiddenClass(): void {