Merge pull request #326 from ZoneMinder/fix/322-dialog-scroll

fix: cap dialog height so tall dialogs scroll instead of overflowing

Author: pliablepixels

app/src/components/dashboard/DashboardConfig.tsx

@@ -173,7 +173,7 @@ export function DashboardConfig() {
                     <span className="hidden sm:inline">{t('dashboard.add_widget')}</span>
                 </Button>
             </DialogTrigger>
-            <DialogContent className="max-h-[90vh] overflow-y-auto" data-testid="add-widget-dialog">
+            <DialogContent data-testid="add-widget-dialog">
                 <DialogHeader>
                     <DialogTitle>{t('dashboard.add_widget')}</DialogTitle>
                     <DialogDescription className="sr-only">

app/src/components/dashboard/WidgetEditDialog.tsx

@@ -179,7 +179,7 @@ export function WidgetEditDialog({ open, onOpenChange, widget, profileId }: Widg
 
     return (
         <Dialog open={open} onOpenChange={onOpenChange}>
-            <DialogContent className="sm:max-w-md max-h-[90vh] overflow-y-auto" data-testid="widget-edit-dialog">
+            <DialogContent className="sm:max-w-md" data-testid="widget-edit-dialog">
                 <DialogHeader>
                     <DialogTitle>{t('dashboard.edit_layout')}</DialogTitle>
                     <DialogDescription className="sr-only">

app/src/components/live-activity/LiveActivitySettingsDialog.tsx

@@ -212,7 +212,7 @@ export function LiveActivitySettingsDialog({
 
   return (
     <Dialog open={open} onOpenChange={onOpenChange}>
-      <DialogContent className="max-h-[85vh] overflow-y-auto" data-testid="live-activity-settings-dialog">
+      <DialogContent data-testid="live-activity-settings-dialog">
         <DialogHeader>
           <DialogTitle>{t('live_activity.settings_title')}</DialogTitle>
           <DialogDescription>{t('live_activity.settings_desc')}</DialogDescription>

app/src/components/ui/dialog.tsx

@@ -46,12 +46,22 @@ const DialogContent = React.forwardRef<
       <DialogPrimitive.Content
         ref={ref}
         className={cn(
-          "fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg",
+          // A dialog taller than the screen used to run off both ends of it,
+          // taking its own Save and Cancel buttons with it, worst on a phone
+          // held sideways. Cap the height here and scroll the body below, so
+          // no dialog has to remember to do it for itself (refs #322).
+          "fixed left-[50%] top-[50%] z-50 flex max-h-[calc(100dvh-2rem)] w-full max-w-lg flex-col gap-4 translate-x-[-50%] translate-y-[-50%] border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg",
           className
         )}
         {...props}
       >
-        {children}
+        {/* The scroll lives on the body, not on the content: the close button
+            is positioned against the content, so scrolling the content itself
+            would carry the button off with it. The body inherits the content's
+            gap so a caller passing `gap-0` still reaches its own children. */}
+        <div className="grid min-h-0 gap-[inherit] overflow-y-auto" data-testid="dialog-body">
+          {children}
+        </div>
         <DialogPrimitive.Close
           className="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground"
           data-testid="dialog-close-button"

app/tests/features/profiles.feature

@@ -23,6 +23,17 @@ Feature: Profile Management
     And I save profile edits
     Then the updated profile name should appear in the list
 
+  @web
+  Scenario: Edit profile dialog scrolls when it is taller than a short screen
+    Given the viewport is phone landscape size
+    When I open the edit dialog for the first profile
+    Then I should see the profile edit dialog
+    And the profile edit dialog should fit within the viewport
+    And the profile edit dialog body should be scrollable
+    When I scroll the profile edit dialog to the bottom
+    Then the profile edit save button should be within the viewport
+    And the dialog close button should be within the viewport
+
   @all
   Scenario: Add profile with connection details
     When I click the add profile button

app/tests/steps/common.steps.ts

@@ -147,6 +147,12 @@ Given('the viewport is tablet size', async ({ page }) => {
   await page.setViewportSize({ width: 1024, height: 768 });
 });
 
+// Phone held sideways: the shortest screen the app has to work on, and where
+// a dialog that cannot scroll hides its own buttons (refs #322).
+Given('the viewport is phone landscape size', async ({ page }) => {
+  await page.setViewportSize({ width: 812, height: 375 });
+});
+
 // Generic dialog steps used across multiple features
 When('I click outside the dialog', async ({ page }) => {
   await page.locator('body').click({ position: { x: 10, y: 10 } });

app/tests/steps/profiles.steps.ts

@@ -200,3 +200,46 @@ Then('the newly added profile should still appear in the list', async ({ page })
   const card = page.locator('[data-testid="profile-card"]').filter({ hasText: newProfileName });
   await expect(card).toBeVisible({ timeout: testConfig.timeouts.element });
 });
+
+// The edit dialog is the tallest in the app, so it is where a dialog that
+// cannot scroll first hides its own Save and Cancel buttons (refs #322).
+function dialogBody(page: import('@playwright/test').Page) {
+  return page.getByTestId('profile-edit-dialog').getByTestId('dialog-body');
+}
+
+async function isWithinViewport(locator: import('@playwright/test').Locator) {
+  const box = await locator.boundingBox();
+  if (!box) return false;
+  const viewport = locator.page().viewportSize();
+  if (!viewport) throw new Error('E2E: no viewport size set');
+  return box.y >= 0 && box.y + box.height <= viewport.height;
+}
+
+Then('the profile edit dialog should fit within the viewport', async ({ page }) => {
+  expect(await isWithinViewport(page.getByTestId('profile-edit-dialog'))).toBe(true);
+});
+
+Then('the profile edit dialog body should be scrollable', async ({ page }) => {
+  const overflow = await dialogBody(page).evaluate((el) => el.scrollHeight - el.clientHeight);
+  // The fixture profile's fields are what make this dialog overflow a 375px
+  // tall screen; if they ever stop doing so the scenario is vacuous, so this
+  // is an assertion rather than a skip.
+  expect(overflow).toBeGreaterThan(0);
+});
+
+When('I scroll the profile edit dialog to the bottom', async ({ page }) => {
+  await dialogBody(page).evaluate((el) => {
+    el.scrollTop = el.scrollHeight;
+  });
+});
+
+Then('the profile edit save button should be within the viewport', async ({ page }) => {
+  await expect(page.getByTestId('profile-edit-save')).toBeVisible();
+  expect(await isWithinViewport(page.getByTestId('profile-edit-save'))).toBe(true);
+});
+
+Then('the dialog close button should be within the viewport', async ({ page }) => {
+  const close = page.getByTestId('profile-edit-dialog').getByTestId('dialog-close-button');
+  await expect(close).toBeVisible();
+  expect(await isWithinViewport(close)).toBe(true);
+});