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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
