Add DialogApi.open() and deprecate show/showModal

The existing show() and showModal() methods render dialog chrome (a Material UI Dialog) as part of the implementation. This causes focus trap conflicts when the caller's content uses components from a different design library (e.g. Backstage UI). The new open() method renders the caller's content as-is, without any dialog chrome. The caller provides the full dialog component including overlay, backdrop, and surface, making the API design-library-agnostic. The deprecated show/showModal are re-implemented on top of open() with a MUI Dialog wrapper for backward compatibility, and emit console warnings when used. Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com> Made-with: Cursor Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com> Made-with: Cursor Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com> Made-with: Cursor Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com> Made-with: Cursor
2026-04-11 23:52:35 +02:00
parent 0ecb8225cb
commit e4804abb44
10 changed files with 438 additions and 263 deletions
@@ -1,70 +0,0 @@
-/*
- * Copyright 2025 The Backstage Authors
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import { DialogApi, DialogApiDialog } from '@backstage/frontend-plugin-api';
-
-export type OnShowDialog = (options: {
-  component: (props: { dialog: DialogApiDialog<any> }) => React.JSX.Element;
-  modal: boolean;
-}) => DialogApiDialog<unknown>;
-
-/**
- * Default implementation for the {@link DialogApi}.
- * @internal
- */
-export class DefaultDialogApi implements DialogApi {
-  #onShow?: OnShowDialog;
-
-  show<TResult = void>(
-    elementOrComponent:
-      | JSX.Element
-      | ((props: {
-          dialog: DialogApiDialog<TResult | undefined>;
-        }) => JSX.Element),
-  ): DialogApiDialog<TResult | undefined> {
-    if (!this.#onShow) {
-      throw new Error('Dialog API has not been connected');
-    }
-    return this.#onShow({
-      component:
-        typeof elementOrComponent === 'function'
-          ? elementOrComponent
-          : () => elementOrComponent,
-      modal: false,
-    }) as DialogApiDialog<TResult | undefined>;
-  }
-
-  showModal<TResult = void>(
-    elementOrComponent:
-      | JSX.Element
-      | ((props: { dialog: DialogApiDialog<TResult> }) => JSX.Element),
-  ): DialogApiDialog<TResult> {
-    if (!this.#onShow) {
-      throw new Error('Dialog API has not been connected');
-    }
-    return this.#onShow({
-      component:
-        typeof elementOrComponent === 'function'
-          ? elementOrComponent
-          : () => elementOrComponent,
-      modal: true,
-    }) as DialogApiDialog<TResult>;
-  }
-
-  connect(onShow: OnShowDialog): void {
-    this.#onShow = onShow;
-  }
-}
@@ -0,0 +1,141 @@
+/*
+ * Copyright 2025 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { DialogApi, DialogApiDialog } from '@backstage/frontend-plugin-api';
+import Dialog from '@material-ui/core/Dialog';
+
+export type OnOpenDialog = (options: {
+  component: (props: { dialog: DialogApiDialog<any> }) => JSX.Element;
+}) => DialogApiDialog<unknown>;
+
+/**
+ * Default implementation for the {@link DialogApi}.
+ * @internal
+ */
+export class DefaultDialogApi implements DialogApi {
+  #onOpen?: OnOpenDialog;
+
+  open<TResult = void>(
+    elementOrComponent:
+      | JSX.Element
+      | ((props: { dialog: DialogApiDialog<TResult> }) => JSX.Element),
+  ): DialogApiDialog<TResult> {
+    if (!this.#onOpen) {
+      throw new Error('Dialog API has not been connected');
+    }
+    return this.#onOpen({
+      component:
+        typeof elementOrComponent === 'function'
+          ? elementOrComponent
+          : () => elementOrComponent,
+    }) as DialogApiDialog<TResult>;
+  }
+
+  /** @deprecated Use {@link DefaultDialogApi.open} instead */
+  show<TResult = void>(
+    elementOrComponent:
+      | JSX.Element
+      | ((props: {
+          dialog: DialogApiDialog<TResult | undefined>;
+        }) => JSX.Element),
+  ): DialogApiDialog<TResult | undefined> {
+    // eslint-disable-next-line no-console
+    console.warn(
+      'DialogApi.show() is deprecated and will be removed in a future release. Use DialogApi.open() instead.',
+    );
+    const innerDialog = this.open<TResult | undefined>(({ dialog }) => (
+      <DeprecatedMuiDialogWrapper
+        dialog={dialog}
+        content={elementOrComponent}
+        modal={false}
+      />
+    ));
+    return wrapDialogHandle(innerDialog, false);
+  }
+
+  /** @deprecated Use {@link DefaultDialogApi.open} instead */
+  showModal<TResult = void>(
+    elementOrComponent:
+      | JSX.Element
+      | ((props: { dialog: DialogApiDialog<TResult> }) => JSX.Element),
+  ): DialogApiDialog<TResult> {
+    // eslint-disable-next-line no-console
+    console.warn(
+      'DialogApi.showModal() is deprecated and will be removed in a future release. Use DialogApi.open() instead.',
+    );
+    const innerDialog = this.open<TResult>(({ dialog }) => (
+      <DeprecatedMuiDialogWrapper
+        dialog={dialog}
+        content={elementOrComponent}
+        modal
+      />
+    ));
+    return wrapDialogHandle(innerDialog, true);
+  }
+
+  connect(onOpen: OnOpenDialog): void {
+    this.#onOpen = onOpen;
+  }
+}
+
+function DeprecatedMuiDialogWrapper({
+  dialog,
+  content,
+  modal,
+}: {
+  dialog: DialogApiDialog<any>;
+  content:
+    | JSX.Element
+    | ((props: { dialog: DialogApiDialog<any> }) => JSX.Element);
+  modal: boolean;
+}) {
+  if (typeof content === 'function') {
+    const Content = content;
+    return (
+      <Dialog open onClose={modal ? undefined : () => dialog.close()}>
+        <Content dialog={dialog} />
+      </Dialog>
+    );
+  }
+  return (
+    <Dialog open onClose={modal ? undefined : () => dialog.close()}>
+      {content}
+    </Dialog>
+  );
+}
+
+function wrapDialogHandle<TResult>(
+  innerDialog: DialogApiDialog<TResult>,
+  modal: boolean,
+): DialogApiDialog<TResult> {
+  return {
+    close(...args: any[]) {
+      (innerDialog.close as any)(...args);
+    },
+    result() {
+      return innerDialog.result();
+    },
+    update(newContent: any) {
+      innerDialog.update(({ dialog }: { dialog: DialogApiDialog<TResult> }) => (
+        <DeprecatedMuiDialogWrapper
+          dialog={dialog}
+          content={newContent}
+          modal={modal}
+        />
+      ));
+    },
+  };
+}
@@ -16,6 +16,7 @@

 import { renderTestApp } from '@backstage/frontend-test-utils';
 import { act, useEffect } from 'react';
+import { screen } from '@testing-library/react';
 import {
  AppRootElementBlueprint,
  DialogApi,
@@ -49,11 +50,11 @@ async function withDialogApi<T>(
 }

 describe('DialogDisplay', () => {
-  function AutoDialog({
+  function AutoCloseDialog({
    dialog,
    result,
  }: {
-    dialog: DialogApiDialog<string | undefined>;
+    dialog: DialogApiDialog<string>;
    result?: string;
  }) {
    useEffect(() => {
@@ -64,112 +65,219 @@ describe('DialogDisplay', () => {
    return <div />;
  }

-  it('should render a simple dialog', async () => {
-    const result = await withDialogApi(async dialogApi => {
-      const dialog = await act(() => dialogApi.show<string>(<div>Test</div>));
-      dialog.close('test');
-      return dialog.result();
-    });
-    expect(result).toBe('test');
-  });
-
-  it('should allow dialog to be updated', async () => {
-    const result = await withDialogApi(async dialogApi => {
-      const dialog = await act(() => dialogApi.show(AutoDialog));
-
-      setTimeout(async () => {
-        await act(async () => {
-          dialog.update(props => <AutoDialog {...props} result="test2" />);
-        });
-      }, 100);
-
-      return dialog.result();
+  describe('open', () => {
+    it('should render a dialog and return a result', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.open<string>(<div>Test</div>));
+        dialog.close('test');
+        return dialog.result();
+      });
+      expect(result).toBe('test');
    });

-    expect(result).toBe('test2');
-  });
-
-  it('should allow dialog to be closed by pressing escape', async () => {
-    const result = await withDialogApi(async dialogApi => {
-      const dialog = await act(() => dialogApi.show(AutoDialog));
-
-      setTimeout(async () => {
-        await userEvent.keyboard('{Escape}');
-      }, 100);
-
-      return dialog.result();
+    it('should render dialog content directly without wrapper chrome', async () => {
+      await withDialogApi(async dialogApi => {
+        await act(() =>
+          dialogApi.open(<div data-testid="bare-content">Hello</div>),
+        );
+        expect(await screen.findByTestId('bare-content')).toBeInTheDocument();
+        return undefined;
+      });
    });

-    expect(result).toBe(undefined);
-  });
-
-  it('should allow a stack of dialogs', async () => {
-    const result = await withDialogApi(async dialogApi => {
-      const dialog1 = await act(() => dialogApi.show(AutoDialog));
-      const dialog2 = await act(() => dialogApi.show(AutoDialog));
-      const dialog3 = await act(() => dialogApi.show(AutoDialog));
-
-      setTimeout(async () => {
-        await act(async () => {
-          dialog3.close('test3');
-          dialog1.close('test1');
-          dialog2.close('test2');
-        });
-      }, 100);
-
-      return Promise.all([
-        dialog1.result(),
-        dialog2.result(),
-        dialog3.result(),
-      ]);
-    });
-
-    expect(result).toEqual(['test1', 'test2', 'test3']);
-  });
-
-  it('should only cancel one dialog at a time', async () => {
-    const result = await withDialogApi(async dialogApi => {
-      const dialog1 = await act(() => dialogApi.show(AutoDialog));
-      const dialog2 = await act(() => dialogApi.show(AutoDialog));
-      const dialog3 = await act(() => dialogApi.show(AutoDialog));
-
-      setTimeout(async () => {
-        await userEvent.keyboard('{Escape}');
-
-        await act(async () => {
-          dialog1.close('test1');
-          dialog2.close('test2');
-          dialog3.close('test3');
-        });
-      }, 100);
-
-      return Promise.all([
-        dialog1.result(),
-        dialog2.result(),
-        dialog3.result(),
-      ]);
-    });
-
-    expect(result).toEqual(['test1', 'test2', undefined]);
-  });
-
-  it('should not allow modal dialog to be closed by pressing escape', async () => {
-    const result = await withDialogApi(async dialogApi => {
-      const dialog = await act(() => dialogApi.showModal(AutoDialog));
-
-      setTimeout(async () => {
-        await userEvent.keyboard('{Escape}');
+    it('should allow dialog to be updated', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.open<string>(AutoCloseDialog));

        setTimeout(async () => {
          await act(async () => {
-            dialog.close('test');
+            dialog.update(props => (
+              <AutoCloseDialog {...props} result="test2" />
+            ));
          });
        }, 100);
-      }, 100);

-      return dialog.result();
+        return dialog.result();
+      });
+
+      expect(result).toBe('test2');
    });

-    expect(result).toBe('test');
+    it('should allow a stack of dialogs, rendering only the most recent', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog1 = await act(() =>
+          dialogApi.open<string>(AutoCloseDialog),
+        );
+        const dialog2 = await act(() =>
+          dialogApi.open<string>(AutoCloseDialog),
+        );
+        const dialog3 = await act(() =>
+          dialogApi.open<string>(AutoCloseDialog),
+        );
+
+        setTimeout(async () => {
+          await act(async () => {
+            dialog3.close('test3');
+            dialog1.close('test1');
+            dialog2.close('test2');
+          });
+        }, 100);
+
+        return Promise.all([
+          dialog1.result(),
+          dialog2.result(),
+          dialog3.result(),
+        ]);
+      });
+
+      expect(result).toEqual(['test1', 'test2', 'test3']);
+    });
+
+    it('should accept a component that receives the dialog handle', async () => {
+      function TestDialog({ dialog }: { dialog: DialogApiDialog<string> }) {
+        return (
+          <button onClick={() => dialog.close('from-component')}>Close</button>
+        );
+      }
+
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.open<string>(TestDialog));
+
+        setTimeout(async () => {
+          const button = await screen.findByRole('button', { name: 'Close' });
+          await userEvent.click(button);
+        }, 100);
+
+        return dialog.result();
+      });
+
+      expect(result).toBe('from-component');
+    });
+  });
+
+  describe('deprecated show', () => {
+    function AutoDialog({
+      dialog,
+      result,
+    }: {
+      dialog: DialogApiDialog<string | undefined>;
+      result?: string;
+    }) {
+      useEffect(() => {
+        if (result) {
+          dialog.close(result);
+        }
+      }, [dialog, result]);
+      return <div />;
+    }
+
+    it('should render a simple dialog', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.show<string>(<div>Test</div>));
+        dialog.close('test');
+        return dialog.result();
+      });
+      expect(result).toBe('test');
+    });
+
+    it('should allow dialog to be updated', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.show(AutoDialog));
+
+        setTimeout(async () => {
+          await act(async () => {
+            dialog.update(props => <AutoDialog {...props} result="test2" />);
+          });
+        }, 100);
+
+        return dialog.result();
+      });
+
+      expect(result).toBe('test2');
+    });
+
+    it('should allow dialog to be closed by pressing escape', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.show(AutoDialog));
+
+        setTimeout(async () => {
+          await userEvent.keyboard('{Escape}');
+        }, 100);
+
+        return dialog.result();
+      });
+
+      expect(result).toBe(undefined);
+    });
+
+    it('should allow a stack of dialogs', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog1 = await act(() => dialogApi.show(AutoDialog));
+        const dialog2 = await act(() => dialogApi.show(AutoDialog));
+        const dialog3 = await act(() => dialogApi.show(AutoDialog));
+
+        setTimeout(async () => {
+          await act(async () => {
+            dialog3.close('test3');
+            dialog1.close('test1');
+            dialog2.close('test2');
+          });
+        }, 100);
+
+        return Promise.all([
+          dialog1.result(),
+          dialog2.result(),
+          dialog3.result(),
+        ]);
+      });
+
+      expect(result).toEqual(['test1', 'test2', 'test3']);
+    });
+
+    it('should only cancel one dialog at a time', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog1 = await act(() => dialogApi.show(AutoDialog));
+        const dialog2 = await act(() => dialogApi.show(AutoDialog));
+        const dialog3 = await act(() => dialogApi.show(AutoDialog));
+
+        setTimeout(async () => {
+          await userEvent.keyboard('{Escape}');
+
+          await act(async () => {
+            dialog1.close('test1');
+            dialog2.close('test2');
+            dialog3.close('test3');
+          });
+        }, 100);
+
+        return Promise.all([
+          dialog1.result(),
+          dialog2.result(),
+          dialog3.result(),
+        ]);
+      });
+
+      expect(result).toEqual(['test1', 'test2', undefined]);
+    });
+
+    it('should not allow modal dialog to be closed by pressing escape', async () => {
+      const result = await withDialogApi(async dialogApi => {
+        const dialog = await act(() => dialogApi.showModal(AutoDialog));
+
+        setTimeout(async () => {
+          await userEvent.keyboard('{Escape}');
+
+          setTimeout(async () => {
+            await act(async () => {
+              dialog.close('test');
+            });
+          }, 100);
+        }, 100);
+
+        return dialog.result();
+      });
+
+      expect(result).toBe('test');
+    });
  });
 });
@@ -22,8 +22,7 @@ import {
  dialogApiRef,
 } from '@backstage/frontend-plugin-api';
 import { createDeferred } from '@backstage/types';
-import { OnShowDialog } from '../apis/DefaultDialogApi';
-import Dialog from '@material-ui/core/Dialog';
+import { OnOpenDialog } from '../apis/DefaultDialogApi';

 let dialogId = 0;
 function getDialogId() {
@@ -33,20 +32,25 @@ function getDialogId() {

 type DialogState = DialogApiDialog<unknown> & {
  id: string;
-  modal: boolean;
 };

 /**
 * The other half of the default implementation of the {@link DialogApi}.
 *
- * This component is responsible for rendering the dialogs in the React tree and managing a stack of dialogs.
- * It expects the implementation of the {@link DialogApi} to be the `DefaultDialogApi`. If one is replaced the other must be too.
+ * This component is responsible for rendering the dialogs in the React tree
+ * and managing a stack of dialogs. It renders only the most recently opened
+ * dialog, without any dialog chrome — the caller is expected to provide their
+ * own dialog component (overlay, backdrop, surface, etc.).
+ *
+ * It expects the implementation of the {@link DialogApi} to be the
+ * `DefaultDialogApi`. If one is replaced the other must be too.
+ *
 * @internal
 */
 function DialogDisplay({
  dialogApi,
 }: {
-  dialogApi: DialogApi & { connect(onShow: OnShowDialog): void };
+  dialogApi: DialogApi & { connect(onOpen: OnOpenDialog): void };
 }) {
  const [dialogs, setDialogs] = useState<
    { dialog: DialogState; element: React.JSX.Element }[]
@@ -58,7 +62,6 @@ function DialogDisplay({
      const deferred = createDeferred<unknown>();
      const dialog: DialogState = {
        id,
-        modal: options.modal,
        close(result) {
          deferred.resolve(result);
          setDialogs(ds => ds.filter(d => d.dialog.id !== id));
@@ -85,19 +88,7 @@ function DialogDisplay({
  }, [dialogApi]);

  if (dialogs.length > 0) {
-    const lastDialog = dialogs[dialogs.length - 1];
-    return (
-      <Dialog
-        open
-        onClose={() => {
-          if (!lastDialog.dialog.modal) {
-            lastDialog.dialog.close();
-          }
-        }}
-      >
-        {lastDialog.element}
-      </Dialog>
-    );
+    return dialogs[dialogs.length - 1].element;
  }

  return null;
@@ -121,7 +112,7 @@ export const dialogDisplayAppRootElement =

 function isInternalDialogApi(
  dialogApi?: DialogApi,
-): dialogApi is DialogApi & { connect(onShow: OnShowDialog): void } {
+): dialogApi is DialogApi & { connect(onOpen: OnOpenDialog): void } {
  if (!dialogApi) {
    return false;
  }