fix(modal): handle removal of open attribute and prevent multiple bef…

…oreClose calls (#7470) **Related Issue:** #6407 #6379 ## Summary - Add internal property `opened` to maintain open state when beforeBack is rejected and to handle initial animation. - Refactor `beforeClose` default value. - Rename internal close method name to be consistent with internal open method name - Prevent `beforeClose` from being called twice by adding an internal flag to ignore watch changes when necessary. - Tests
Esri · Aug 23, 2023 · f31588f · f31588f
1 parent 1327cef
commit f31588f
Show file tree

Hide file tree

Showing 3 changed files with 118 additions and 47 deletions.
diff --git a/packages/calcite-components/src/components/modal/modal.e2e.ts b/packages/calcite-components/src/components/modal/modal.e2e.ts
@@ -35,8 +35,8 @@ describe("calcite-modal properties", () => {
     const modal = await page.find("calcite-modal");
     await modal.setProperty("open", true);
     await page.waitForChanges();
-    const style = await page.$eval("calcite-modal", (elm) => {
-      const m = elm.shadowRoot.querySelector(".modal");
+    const style = await page.$eval("calcite-modal", (el) => {
+      const m = el.shadowRoot.querySelector(".modal");
       return window.getComputedStyle(m).getPropertyValue("width");
     });
     expect(style).toEqual("600px");
@@ -50,8 +50,8 @@ describe("calcite-modal properties", () => {
     const modal = await page.find("calcite-modal");
     await modal.setProperty("open", true);
     await page.waitForChanges();
-    const style = await page.$eval("calcite-modal", (elm) => {
-      const m = elm.shadowRoot.querySelector(".modal");
+    const style = await page.$eval("calcite-modal", (el) => {
+      const m = el.shadowRoot.querySelector(".modal");
       return window.getComputedStyle(m).getPropertyValue("height");
     });
     expect(style).toEqual("600px");
@@ -65,8 +65,8 @@ describe("calcite-modal properties", () => {
     const modal = await page.find("calcite-modal");
     await modal.setProperty("open", true);
     await page.waitForChanges();
-    const style = await page.$eval("calcite-modal", (elm) => {
-      const m = elm.shadowRoot.querySelector(".modal");
+    const style = await page.$eval("calcite-modal", (el) => {
+      const m = el.shadowRoot.querySelector(".modal");
       return window.getComputedStyle(m).getPropertyValue("width");
     });
     expect(style).not.toEqual("600px");
@@ -80,8 +80,8 @@ describe("calcite-modal properties", () => {
     const modal = await page.find("calcite-modal");
     await modal.setProperty("open", true);
     await page.waitForChanges();
-    const style = await page.$eval("calcite-modal", (elm) => {
-      const m = elm.shadowRoot.querySelector(".modal");
+    const style = await page.$eval("calcite-modal", (el) => {
+      const m = el.shadowRoot.querySelector(".modal");
       return window.getComputedStyle(m).getPropertyValue("height");
     });
     expect(style).not.toEqual("600px");
@@ -97,19 +97,19 @@ describe("calcite-modal properties", () => {
     const modal = await page.find("calcite-modal");
     await modal.setProperty("open", true);
     await page.waitForChanges();
-    const styleW = await page.$eval("calcite-modal", (elm) => {
-      const m = elm.shadowRoot.querySelector(".modal");
+    const styleW = await page.$eval("calcite-modal", (el) => {
+      const m = el.shadowRoot.querySelector(".modal");
       return window.getComputedStyle(m).getPropertyValue("width");
     });
-    const styleH = await page.$eval("calcite-modal", (elm) => {
-      const m = elm.shadowRoot.querySelector(".modal");
+    const styleH = await page.$eval("calcite-modal", (el) => {
+      const m = el.shadowRoot.querySelector(".modal");
       return window.getComputedStyle(m).getPropertyValue("height");
     });
     expect(styleW).toEqual("800px");
     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);
@@ -119,18 +119,74 @@ describe("calcite-modal properties", () => {
     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)
+      (el: HTMLCalciteModalElement) =>
+        (el.beforeClose = (
+          window as GlobalTestProps<{ beforeClose: HTMLCalciteModalElement["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).toHaveBeenCalledTimes(1);
+    expect(await modal.getProperty("opened")).toBe(false);
+  });
+
+  it("calls the beforeClose method prior to closing via ESC key", 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",
+      (el: HTMLCalciteModalElement) =>
+        (el.beforeClose = (
+          window as GlobalTestProps<{ beforeClose: HTMLCalciteModalElement["beforeClose"] }>
+        ).beforeClose)
+    );
+    await page.waitForChanges();
+    modal.setProperty("open", true);
+    await page.waitForChanges();
+    expect(await modal.getProperty("opened")).toBe(true);
+    await page.keyboard.press("Escape");
     await page.waitForChanges();
-    expect(mockCallBack).toHaveBeenCalled();
+    await page.waitForChanges();
+    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",
+    (el: HTMLCalciteModalElement) =>
+      (el.beforeClose = (
+        window as GlobalTestProps<{ beforeClose: HTMLCalciteModalElement["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();
@@ -462,7 +518,7 @@ describe("calcite-modal accessibility checks", () => {
     modal.setProperty("open", true);
     await page.waitForChanges();
     expect(modal).toHaveAttribute("open");
-    await page.$eval("calcite-modal", (elm) => elm.shadowRoot.querySelector("calcite-scrim").click());
+    await page.$eval("calcite-modal", (el) => el.shadowRoot.querySelector("calcite-scrim").click());
     await page.waitForChanges();
     expect(await modal.getProperty("open")).toBe(false);
   });
@@ -474,7 +530,7 @@ describe("calcite-modal accessibility checks", () => {
     modal.setProperty("open", true);
     await page.waitForChanges();
     expect(modal).toHaveAttribute("open");
-    await page.$eval("calcite-modal", (elm) => elm.shadowRoot.querySelector("calcite-scrim").click());
+    await page.$eval("calcite-modal", (el) => el.shadowRoot.querySelector("calcite-scrim").click());
     await page.waitForChanges();
     expect(await modal.getProperty("open")).toBe(true);
   });
@@ -536,11 +592,11 @@ describe("calcite-modal accessibility checks", () => {
         <calcite-button slot="primary">TEST</calcite-button>
       </calcite-modal>
     `);
-    let footer = await page.$eval("calcite-modal", (elm) => elm.shadowRoot.querySelector(".footer"));
+    let footer = await page.$eval("calcite-modal", (el) => el.shadowRoot.querySelector(".footer"));
     expect(footer).toBeDefined();
-    await page.$eval("calcite-button", (elm) => elm.parentElement.removeChild(elm));
+    await page.$eval("calcite-button", (el) => el.parentElement.removeChild(el));
     await page.waitForChanges();
-    footer = await page.$eval("calcite-modal", (elm) => elm.shadowRoot.querySelector(".footer"));
+    footer = await page.$eval("calcite-modal", (el) => el.shadowRoot.querySelector(".footer"));
     expect(footer).toBeFalsy();
   });
 

diff --git a/packages/calcite-components/src/components/modal/modal.scss b/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,

diff --git a/packages/calcite-components/src/components/modal/modal.tsx b/packages/calcite-components/src/components/modal/modal.tsx
@@ -86,9 +86,15 @@ export class Modal
   /** When `true`, displays and positions the component.  */
   @Prop({ mutable: true, reflect: true }) open = false;
 
+  /**
+   * 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;
+
   /** 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 +214,7 @@ export class Modal
         <div
           class={{
             [CSS.container]: true,
-            [CSS.containerOpen]: this.isOpen,
+            [CSS.containerOpen]: this.opened,
             [CSS.slottedInShell]: this.slottedInShell,
           }}
         >
@@ -284,7 +290,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 should be last so node attrs/props are in sync (see https://github.com/Esri/calcite-design-system/pull/6530)
         ref={(el) => (this.closeButtonEl = el)}
@@ -343,6 +349,8 @@ export class Modal
   //
   //--------------------------------------------------------------------------
 
+  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 {