sadpandajoe commented on code in PR #42548:
URL: https://github.com/apache/superset/pull/42548#discussion_r3684804320
##########
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:
Ant Design 6.5.1 only renders a higher modal z-index when it inherits a
`ZIndexContext`; this top-level sibling gets no inline z-index, while the View
query modal can inherit the dropdown's elevated context. That can leave the
unsaved-changes prompt behind the existing modal, so could this use a strategy
that is actually above the active overlay context?
##########
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:
This component is exported from `@superset-ui/core/components`, so removing
`zIndex` breaks downstream TypeScript callers and silently drops their overlay
override at runtime. Could we keep the optional pass-through while removing
only the hardcoded default?
##########
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:
This assertion currently fails in CI because both computed values are `NaN`,
and it also would not distinguish the old implementation in a browser: the
hardcoded 1300 still beats this plain sibling's default layer. Could the
regression test recreate the elevated View SQL modal/context so it fails when
the hardcoded `zIndex` is restored and passes with the intended fix?
--
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]
