fix(dialog): prevent blurring of dialog and its contents

Sage · Oct 10, 2024 · 0fdeecf · 0fdeecf
1 parent 2719346
commit 0fdeecf
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 119 deletions.
diff --git a/src/components/dialog/dialog.component.tsx b/src/components/dialog/dialog.component.tsx
@@ -9,6 +9,7 @@ import {
   StyledDialog,
   StyledDialogTitle,
   StyledDialogContent,
+  DialogPositioner,
 } from "./dialog.style";
 import { DialogSizes } from "./dialog.config";
 
@@ -146,49 +147,41 @@ export const Dialog = forwardRef<DialogHandle, DialogProps>(
       []
     );
 
-    const closeIcon = () => {
-      if (!showCloseIcon || !onCancel) return null;
-
-      return (
-        <IconButton
-          aria-label={locale.dialog.ariaLabels.close()}
-          onClick={onCancel}
-          disabled={disableClose}
-          {...tagComponent("close", {
-            "data-element": "close",
-            ...closeButtonDataProps,
-          })}
-        >
-          <Icon type="close" />
-        </IconButton>
-      );
-    };
-
-    const dialogTitle = () => {
-      if (!title) return null;
+    const closeIcon = showCloseIcon && onCancel && (
+      <IconButton
+        aria-label={locale.dialog.ariaLabels.close()}
+        onClick={onCancel}
+        disabled={disableClose}
+        {...tagComponent("close", {
+          "data-element": "close",
+          ...closeButtonDataProps,
+        })}
+      >
+        <Icon type="close" />
+      </IconButton>
+    );
 
-      return (
-        <StyledDialogTitle
-          showCloseIcon={showCloseIcon}
-          hasSubtitle={!!subtitle}
-          ref={titleRef}
-        >
-          {typeof title === "string" ? (
-            <Heading
-              data-element="dialog-title"
-              title={title}
-              titleId={titleId}
-              subheader={subtitle}
-              subtitleId={subtitleId}
-              divider={false}
-              help={help}
-            />
-          ) : (
-            title
-          )}
-        </StyledDialogTitle>
-      );
-    };
+    const dialogTitle = title && (
+      <StyledDialogTitle
+        showCloseIcon={showCloseIcon}
+        hasSubtitle={!!subtitle}
+        ref={titleRef}
+      >
+        {typeof title === "string" ? (
+          <Heading
+            data-element="dialog-title"
+            title={title}
+            titleId={titleId}
+            subheader={subtitle}
+            subtitleId={subtitleId}
+            divider={false}
+            help={help}
+          />
+        ) : (
+          title
+        )}
+      </StyledDialogTitle>
+    );
 
     let dialogHeight = height;
 
@@ -226,33 +219,35 @@ export const Dialog = forwardRef<DialogHandle, DialogProps>(
           isOpen={open}
           additionalWrapperRefs={focusableContainers}
         >
-          <StyledDialog
-            data-component={dataComponent}
-            data-element={dataElement}
-            data-role={dataRole}
-            aria-modal={isTopModal ? true : undefined}
-            ref={containerRef}
-            {...dialogProps}
-            role={role}
-            tabIndex={-1}
-            {...contentPadding}
-            backgroundColor={
-              greyBackground
-                ? "var(--colorsUtilityMajor025)"
-                : "var(--colorsUtilityYang100)"
-            }
-          >
-            {dialogTitle()}
-            {closeIcon()}
-            <StyledDialogContent
-              {...contentPadding}
-              data-role="dialog-content"
+          <DialogPositioner>
+            <StyledDialog
+              data-component={dataComponent}
+              data-element={dataElement}
+              data-role={dataRole}
+              aria-modal={isTopModal ? true : undefined}
+              ref={containerRef}
+              {...dialogProps}
+              role={role}
               tabIndex={-1}
-              ref={innerContentRef}
+              {...contentPadding}
+              backgroundColor={
+                greyBackground
+                  ? "var(--colorsUtilityMajor025)"
+                  : "var(--colorsUtilityYang100)"
+              }
             >
-              {children}
-            </StyledDialogContent>
-          </StyledDialog>
+              {dialogTitle}
+              {closeIcon}
+              <StyledDialogContent
+                {...contentPadding}
+                data-role="dialog-content"
+                tabIndex={-1}
+                ref={innerContentRef}
+              >
+                {children}
+              </StyledDialogContent>
+            </StyledDialog>
+          </DialogPositioner>
         </FocusTrap>
       </Modal>
     );

diff --git a/src/components/dialog/dialog.pw.tsx b/src/components/dialog/dialog.pw.tsx
@@ -603,32 +603,6 @@ test("dialog re-centres itself in the viewport, when viewport size changes", asy
   expect(boundingBox.y).toBeCloseTo(expectedYPosition);
 });
 
-test("dialog is positioned correctly when size is 'maximise' and viewport width is greater than 960px", async ({
-  mount,
-  page,
-}) => {
-  await page.setViewportSize({ width: 961, height: 600 });
-  await mount(<DialogComponent size="maximise" />);
-
-  const dialog = page.getByRole("dialog");
-  await expect(dialog).toHaveCSS("position", "fixed");
-  await expect(dialog).toHaveCSS("inset", "0px");
-  await expect(dialog).toHaveCSS("margin", "32px");
-});
-
-test("dialog is positioned correctly when size is 'maximise' and viewport width is less than 960px", async ({
-  mount,
-  page,
-}) => {
-  await page.setViewportSize({ width: 959, height: 600 });
-  await mount(<DialogComponent size="maximise" />);
-
-  const dialog = page.getByRole("dialog");
-  await expect(dialog).toHaveCSS("position", "fixed");
-  await expect(dialog).toHaveCSS("inset", "0px");
-  await expect(dialog).toHaveCSS("margin", "16px");
-});
-
 test.describe(
   "Accessibility tests for playwright mock components and storybook stories",
   () => {

diff --git a/src/components/dialog/dialog.stories.tsx b/src/components/dialog/dialog.stories.tsx
@@ -142,7 +142,9 @@ export const MaxSize: Story = () => {
   );
 };
 MaxSize.storyName = "With Max Size";
-MaxSize.parameters = { chromatic: { disableSnapshot: true } };
+MaxSize.parameters = {
+  chromatic: { viewports: [1200, 320] },
+};
 
 export const Editable: Story = () => {
   const [isOpen, setIsOpen] = useState(defaultOpenState);

diff --git a/src/components/dialog/dialog.style.ts b/src/components/dialog/dialog.style.ts
@@ -10,9 +10,8 @@ import {
   HORIZONTAL_PADDING,
   CONTENT_TOP_PADDING,
   CONTENT_BOTTOM_PADDING,
-  DialogSizes,
 } from "./dialog.config";
-import { ContentPaddingInterface } from "./dialog.component";
+import { ContentPaddingInterface, DialogProps } from "./dialog.component";
 import resolvePaddingSides from "../../style/utils/resolve-padding-sides";
 import {
   StyledFormContent,
@@ -31,35 +30,42 @@ const dialogSizes = {
   "extra-large": "1080px",
 };
 
-type StyledDialogProps = {
-  size?: DialogSizes;
+type StyledDialogProps = Required<Pick<DialogProps, "size">> & {
   dialogHeight?: string;
   backgroundColor: string;
 };
 
+const DialogPositioner = styled.div`
+  position: fixed;
+  inset: 0;
+  display: flex;
+  justify-content: center;
+  align-items: center;
+  z-index: ${({ theme }) => theme.zIndex.modal};
+`;
+
 const StyledDialog = styled.div<StyledDialogProps & ContentPaddingInterface>`
   box-shadow: var(--boxShadow300);
   display: flex;
   flex-direction: column;
+  position: relative;
   border-radius: var(--borderRadius200);
-  position: fixed;
-  z-index: ${({ theme }) => theme.zIndex.modal};
 
   ${({ size }) =>
     size === "maximise"
       ? css`
-          inset: 0;
-          margin: 16px;
+          height: calc(100% - var(--spacing400));
+          width: calc(100% - var(--spacing400));
 
           @media screen and (min-width: 960px) {
-            margin: 32px;
+            height: calc(100% - var(--spacing800));
+            width: calc(100% - var(--spacing800));
           }
         `
       : css`
           max-height: 90vh;
-          top: 50%;
-          left: 50%;
-          transform: translate(-50%, -50%);
+          max-width: ${dialogSizes[size]};
+          width: 100%;
         `};
 
   &:focus {
@@ -71,14 +77,6 @@ const StyledDialog = styled.div<StyledDialogProps & ContentPaddingInterface>`
       background-color: ${backgroundColor};
     `}
 
-  ${({ size }) =>
-    size &&
-    size !== "maximise" &&
-    css`
-      max-width: ${dialogSizes[size]};
-      width: 100%;
-    `}
-
   ${({ dialogHeight }) =>
     dialogHeight &&
     css`
@@ -167,12 +165,17 @@ const StyledDialogContent = styled.div<ContentPaddingInterface>((props) => {
   `;
 });
 
-StyledDialog.defaultProps = {
+DialogPositioner.defaultProps = {
   theme: baseTheme,
 };
 
 StyledDialogContent.defaultProps = {
   theme: baseTheme,
 };
 
-export { StyledDialog, StyledDialogTitle, StyledDialogContent };
+export {
+  DialogPositioner,
+  StyledDialog,
+  StyledDialogTitle,
+  StyledDialogContent,
+};
diff --git a/src/components/dialog/dialog.test.tsx b/src/components/dialog/dialog.test.tsx
@@ -322,9 +322,8 @@ test("dialog is positioned correctly, when size prop is maximise", () => {
 
   const dialog = screen.getByRole("dialog");
   expect(dialog).toHaveStyle(`
-    position: fixed;
-    inset: 0;
-    margin: 16px;
+    height: calc(100% - var(--spacing400));
+    width: calc(100% - var(--spacing400));
   `);
 });