Re: [PR] fix(core): let UnsavedChangesModal use Ant Design's automatic z-index stacking [superset]

["rusackas (via GitHub)" <gi...@apache.org>]

rusackas commented on code in PR #42548:
URL: https://github.com/apache/superset/pull/42548#discussion_r3696243927


##########
superset-frontend/packages/superset-ui-core/src/components/UnsavedChangesModal/index.tsx:
##########
@@ -41,15 +36,13 @@ export const UnsavedChangesModal: FC<UnsavedChangesModalProps> = ({
   onConfirmNavigation,
   title = 'Unsaved Changes',
   body = "If you don't save, changes will be lost.",
-  zIndex = UNSAVED_CHANGES_MODAL_Z_INDEX,
 }: UnsavedChangesModalProps): ReactElement => (
   <Modal
     centered
     responsive

Review Comment:
   Good catch, fixed with destroyOnHidden. These two modals are always top-level siblings tied on z-index, so the real fix is making sure this one's wrap node always ends up later in the DOM on every open, not trying to out-rank a value that's already tied.



##########
superset-frontend/packages/superset-ui-core/src/components/UnsavedChangesModal/index.tsx:
##########
@@ -20,18 +20,13 @@ import { t } from '@apache-superset/core/translation';
 import { Icons, Modal, Typography, Button } from '@superset-ui/core/components';
 import type { FC, ReactElement } from 'react';
 
-// Ant Design's default modal zIndex is 1000. Using a higher value ensures
-// this dialog always renders above other open modals (e.g. a draggable View SQL modal).
-const UNSAVED_CHANGES_MODAL_Z_INDEX = 1300;
-
 export type UnsavedChangesModalProps = {
   showModal: boolean;
   onHide: () => void;
   handleSave: () => void;
   onConfirmNavigation: () => void;
   title?: string;
   body?: string;

Review Comment:
   Checked, nothing in the codebase passes zIndex to this component so there's no compile break there. Keeping the passthrough would let a caller reintroduce the same hardcoded-override footgun this PR is fixing, so I'd rather not bring it back. Added a note in UPDATING.md for any external consumer that was relying on it.



##########
superset-frontend/packages/superset-ui-core/src/components/UnsavedChangesModal/UnsavedChangesModal.test.tsx:
##########
@@ -94,3 +95,57 @@ test('should only call handleSave when clicking the Save button', async () => {
   expect(mockOnHide).not.toHaveBeenCalled();
   expect(mockOnConfirmNavigation).not.toHaveBeenCalled();
 });
+
+test('renders above an already-open modal without a hardcoded z-index', () => {
+  // Regression test for a bug where this modal could render BEHIND another
+  // already-open modal (e.g. a draggable "View query" modal), because its
+  // z-index was pinned to a hardcoded constant instead of relying on Ant
+  // Design's automatic z-index stacking. Since this modal is always opened
+  // on top of whatever it's interrupting, it should always come out ahead
+  // with no manual override at all.
+  render(
+    <>
+      <Modal show title="Other open modal" onHide={() => {}}>
+        <div>Other modal content</div>
+      </Modal>
+      <UnsavedChangesModal
+        showModal
+        onHide={() => {}}
+        handleSave={() => {}}
+        onConfirmNavigation={() => {}}
+      />
+    </>,
+  );
+
+  // rc-util's `useId` hook always returns the same mocked id ("test-id")
+  // in test environments, so with two modals open at once their
+  // `aria-labelledby` ids collide and `getByRole('dialog', { name })` can't
+  // tell them apart. Find each dialog by its title text instead.
+  const dialogs = screen.getAllByRole('dialog');
+  const otherDialog = dialogs.find(dialog =>
+    within(dialog).queryByText('Other open modal'),
+  );
+  const unsavedChangesDialog = dialogs.find(dialog =>
+    within(dialog).queryByText('Unsaved Changes'),
+  );
+
+  expect(otherDialog).toBeDefined();
+  expect(unsavedChangesDialog).toBeDefined();
+
+  // Ant Design applies the automatically-assigned stacking z-index to the
+  // `.ant-modal-wrap` element that wraps the dialog, not to the dialog
+  // (`role="dialog"`) element itself, so the wrapper is what needs checking.
+  const otherWrap = otherDialog?.closest<HTMLElement>('.ant-modal-wrap');
+  const unsavedChangesWrap =
+    unsavedChangesDialog?.closest<HTMLElement>('.ant-modal-wrap');
+
+  expect(otherWrap).not.toBeNull();
+  expect(unsavedChangesWrap).not.toBeNull();
+
+  const otherZIndex = Number(getComputedStyle(otherWrap as HTMLElement).zIndex);
+  const unsavedChangesZIndex = Number(
+    getComputedStyle(unsavedChangesWrap as HTMLElement).zIndex,
+  );
+
+  expect(unsavedChangesZIndex).toBeGreaterThan(otherZIndex);

Review Comment:
   Good catch, the test's rewritten now. It renders a second modal and checks DOM order via compareDocumentPosition against .ant-modal-wrap instead of comparing raw zIndex values (which were both NaN, as you found). Fails against the old hardcoded implementation and passes with destroyOnHidden.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org