Sfoglia il codice sorgente

Fix: AMS drying popover no longer renders off the bottom of the viewport (#1447 part 1)

  The flame-icon onClick on PrintersPage computed popover position as a
  fixed { top: rect.bottom + 4, left: Math.max(8, rect.right - 240) }
  with no viewport-overflow check. The flame icon sits at the bottom of
  the AMS info section on the printer card, so on most realistic viewports
  rect.bottom + 4 + popover_height (~320px) overruns viewport.height and
  the popover renders partially or entirely off-screen with the Start
  button unreachable. Reporter (kleinweby, P1S + AMS-HT) worked around it
  via DevTools to confirm the popover was actually there, just clipped.

  Extract a computePopoverPosition() helper in utils/popoverPosition.ts:
  - defaults to below + right-aligned to the trigger (preserves the
    original visual layout when there's room),
  - flips ABOVE the trigger when below would overflow AND above fits,
  - stays below in the degraded case (popover taller than the viewport) —
    at least the top is visible and the user can scroll inside the
    popover; flipping to a top-clipped position would lose the action
    buttons too,
  - clamps the left coordinate so a trigger near either viewport edge
    can't push the popover off-screen horizontally either.

  Both PrintersPage callsites (the compact AMS row at :3498 and the
  dual-nozzle layout at :4011) route through the helper.

  This is part 1 of #1447. The functional drying bug — printer receives
  the ams_filament_drying MQTT command, ACKs it, but never starts/stops
  drying on Bambuddy's request while the printer's own touchscreen works
  — is NOT addressed here. Diagnosing it needs the printer's actual
  response payload (whether result: "fail" and the specific reason code),
  which bambu_mqtt.py:918 currently doesn't log for ams_filament_drying.
  Punted to a follow-up where I'll extend the existing extrusion_cali_* /
  ams_filament_setting payload-logging path at :919-920 to cover
  ams_filament_drying too, ask the reporter to retry, and fix the
  command side based on what the printer actually returns.
maziggy 1 settimana fa
parent
commit
0f68039416

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 116 - 0
frontend/src/__tests__/utils/popoverPosition.test.ts

@@ -0,0 +1,116 @@
+import { describe, it, expect } from 'vitest';
+import { computePopoverPosition } from '../../utils/popoverPosition';
+
+/**
+ * Tests for #1447: the AMS drying popover was rendering off the bottom of
+ * the viewport with the Start button unreachable. The new helper must:
+ * - keep the popover below when below fits
+ * - flip above when below would overflow AND above fits
+ * - stay below (degraded) when neither side fits
+ * - clamp the horizontal position so a trigger near the viewport's right
+ *   edge doesn't push the popover off-screen.
+ */
+describe('computePopoverPosition (#1447)', () => {
+  // Trigger positioned in the middle of a 1024x768 viewport.
+  const middleTrigger = { top: 300, bottom: 320, left: 400, right: 440 };
+  const viewport = { viewportWidth: 1024, viewportHeight: 768 };
+
+  it('places the popover below the trigger when below has room', () => {
+    const pos = computePopoverPosition({
+      triggerRect: middleTrigger,
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      ...viewport,
+    });
+    expect(pos.top).toBe(middleTrigger.bottom + 4); // 324
+  });
+
+  it('right-aligns the popover to the trigger by default', () => {
+    const pos = computePopoverPosition({
+      triggerRect: middleTrigger,
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      ...viewport,
+    });
+    expect(pos.left).toBe(middleTrigger.right - 240); // 200
+  });
+
+  it('flips above when the popover would overflow the bottom of the viewport', () => {
+    // Trigger near the bottom — bottom=700 + gap 4 + height 320 = 1024 > 768.
+    const bottomTrigger = { top: 680, bottom: 700, left: 400, right: 440 };
+    const pos = computePopoverPosition({
+      triggerRect: bottomTrigger,
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      ...viewport,
+    });
+    // Above placement: trigger.top - gap - height = 680 - 4 - 320 = 356.
+    expect(pos.top).toBe(356);
+  });
+
+  it('stays below when neither below nor above can fully fit (degraded)', () => {
+    // A popover taller than the viewport itself can never fit anywhere. Stay
+    // below so the user at least sees the top of the popover and can scroll
+    // through it — flipping to a top-clipped position would lose visibility
+    // of the action buttons at the bottom of the popover too.
+    const tallPopover = { estimatedHeight: 900 };
+    const trigger = { top: 380, bottom: 400, left: 400, right: 440 };
+    const pos = computePopoverPosition({
+      triggerRect: trigger,
+      popoverWidth: 240,
+      ...tallPopover,
+      ...viewport,
+    });
+    expect(pos.top).toBe(trigger.bottom + 4);
+  });
+
+  it('clamps horizontally when trigger sits near the right viewport edge', () => {
+    // Trigger.right=1020, popoverWidth=240. Default left would be 780; the
+    // popover would extend to 1020 which is within viewport=1024 minus the
+    // 8px margin -> 1016, so it overflows by 4px. Clamp pushes it left.
+    const rightEdgeTrigger = { top: 100, bottom: 120, left: 980, right: 1020 };
+    const pos = computePopoverPosition({
+      triggerRect: rightEdgeTrigger,
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      ...viewport,
+    });
+    expect(pos.left).toBeLessThanOrEqual(1024 - 240 - 8); // 776
+    expect(pos.left).toBeGreaterThanOrEqual(8);
+  });
+
+  it('clamps horizontally when trigger sits near the left viewport edge', () => {
+    // Trigger.right=120, popoverWidth=240. Default left would be -120 (off
+    // viewport). Clamp to the margin.
+    const leftEdgeTrigger = { top: 100, bottom: 120, left: 80, right: 120 };
+    const pos = computePopoverPosition({
+      triggerRect: leftEdgeTrigger,
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      ...viewport,
+    });
+    expect(pos.left).toBe(8); // default margin
+  });
+
+  it('respects a custom margin', () => {
+    const pos = computePopoverPosition({
+      triggerRect: { top: 100, bottom: 120, left: 80, right: 120 },
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      margin: 16,
+      ...viewport,
+    });
+    expect(pos.left).toBe(16);
+  });
+
+  it('respects a custom gap between trigger and popover', () => {
+    const pos = computePopoverPosition({
+      triggerRect: middleTrigger,
+      popoverWidth: 240,
+      estimatedHeight: 320,
+      gap: 12,
+      ...viewport,
+    });
+    expect(pos.top).toBe(middleTrigger.bottom + 12); // 332
+  });
+});

+ 11 - 2
frontend/src/pages/PrintersPage.tsx

@@ -1,6 +1,15 @@
 import { useState, useEffect, useLayoutEffect, useMemo, useRef, useCallback } from 'react';
 import { compareFwVersions } from '../utils/firmwareVersion';
 import { formatPrintName } from '../utils/printName';
+import { computePopoverPosition } from '../utils/popoverPosition';
+
+// AMS drying popover dimensions — w-[240px] on the popover, estimated height
+// covers header + filament select + temp slider + duration + rotate-tray
+// toggle + buttons. Over-estimating is fine (flip-above kicks in slightly
+// earlier); under-estimating leaves the popover clipped off the bottom (the
+// original bug at #1447).
+const DRYING_POPOVER_WIDTH = 240;
+const DRYING_POPOVER_ESTIMATED_HEIGHT = 320;
 import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
 import { useTranslation } from 'react-i18next';
 import { useTheme } from '../contexts/ThemeContext';
@@ -3486,7 +3495,7 @@ function PrinterCard({
                                           setDryingPopoverModuleType(ams.module_type);
                                           setDryingPopoverAmsId(ams.id);
                                           const rect = (e.currentTarget as HTMLElement).getBoundingClientRect();
-                                          setDryingPopoverPos({ top: rect.bottom + 4, left: Math.max(8, rect.right - 240) });
+                                          setDryingPopoverPos(computePopoverPosition({ triggerRect: rect, popoverWidth: DRYING_POPOVER_WIDTH, estimatedHeight: DRYING_POPOVER_ESTIMATED_HEIGHT }));
                                         }
                                       }}
                                       className={`flex items-center gap-0.5 px-1 py-0.5 rounded text-[9px] transition-colors ${
@@ -3999,7 +4008,7 @@ function PrinterCard({
                                         setDryingPopoverModuleType(ams.module_type);
                                         setDryingPopoverAmsId(ams.id);
                                         const rect = (e.currentTarget as HTMLElement).getBoundingClientRect();
-                                        setDryingPopoverPos({ top: rect.bottom + 4, left: Math.max(8, rect.right - 240) });
+                                        setDryingPopoverPos(computePopoverPosition({ triggerRect: rect, popoverWidth: DRYING_POPOVER_WIDTH, estimatedHeight: DRYING_POPOVER_ESTIMATED_HEIGHT }));
                                       }
                                     }}
                                     className={`flex items-center gap-0.5 px-1 py-0.5 rounded text-[9px] transition-colors ${

+ 82 - 0
frontend/src/utils/popoverPosition.ts

@@ -0,0 +1,82 @@
+export interface PopoverPosition {
+  top: number;
+  left: number;
+}
+
+interface RectLike {
+  top: number;
+  bottom: number;
+  left: number;
+  right: number;
+}
+
+export interface ComputePopoverPositionOpts {
+  /** Trigger element's bounding rect (viewport coordinates). */
+  triggerRect: RectLike;
+  /** Popover width in CSS pixels. */
+  popoverWidth: number;
+  /**
+   * Estimated popover height in CSS pixels. Used to detect bottom-edge
+   * overflow so we can flip above the trigger. A conservative over-estimate
+   * is preferable to an under-estimate — over-estimating just flips slightly
+   * sooner, under-estimating leaves the popover clipped off the viewport.
+   */
+  estimatedHeight: number;
+  /** Viewport height. Defaults to window.innerHeight. Injectable for tests. */
+  viewportHeight?: number;
+  /** Viewport width. Defaults to window.innerWidth. Injectable for tests. */
+  viewportWidth?: number;
+  /** Margin to keep between the popover and the viewport edges. */
+  margin?: number;
+  /** Gap between the trigger and the popover. */
+  gap?: number;
+}
+
+/**
+ * Compute fixed-positioning coordinates for a popover anchored to a trigger.
+ *
+ * Default placement is BELOW the trigger, right-aligned to the trigger. Flips
+ * to ABOVE the trigger when below would overflow the viewport (#1447 — the
+ * AMS drying popover on the printer card sits at the bottom of the AMS row
+ * and was rendering off the bottom of the viewport with the Start button
+ * unreachable on smaller screens).
+ *
+ * Horizontal axis right-aligns to triggerRect.right and clamps to the
+ * viewport with the configured margin so a trigger near the right edge
+ * doesn't push the popover off-screen.
+ */
+export function computePopoverPosition(opts: ComputePopoverPositionOpts): PopoverPosition {
+  const {
+    triggerRect,
+    popoverWidth,
+    estimatedHeight,
+    viewportHeight = window.innerHeight,
+    viewportWidth = window.innerWidth,
+    margin = 8,
+    gap = 4,
+  } = opts;
+
+  // Vertical: prefer below, flip to above only when below overflows AND
+  // above would actually fit. If neither fits (a popover taller than the
+  // viewport), stay below — at least the top of the popover is visible
+  // and the user can scroll inside it, which is better than flipping to a
+  // top-clipped position where the action buttons might also be unreachable.
+  let top = triggerRect.bottom + gap;
+  const wouldOverflowBottom = top + estimatedHeight > viewportHeight - margin;
+  if (wouldOverflowBottom) {
+    const aboveTop = triggerRect.top - gap - estimatedHeight;
+    if (aboveTop >= margin) {
+      top = aboveTop;
+    }
+  }
+
+  // Horizontal: right-align to trigger; clamp to viewport bounds.
+  let left = triggerRect.right - popoverWidth;
+  if (left < margin) {
+    left = margin;
+  } else if (left + popoverWidth > viewportWidth - margin) {
+    left = Math.max(margin, viewportWidth - popoverWidth - margin);
+  }
+
+  return { top, left };
+}

File diff suppressed because it is too large
+ 0 - 0
static/assets/index-B3OGHT8z.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-BrIgWKU8.js"></script>
+    <script type="module" crossorigin src="/assets/index-B3OGHT8z.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-KYwGxnG9.css">
   </head>
   <body>

Some files were not shown because too many files changed in this diff