Fix available vertical space from being undercalculated; fix top tran…

…sition not working; fix available width not accounting for child min-width
Shopify · Feb 5, 2024 · 211d227 · 211d227
1 parent e223144
commit 211d227
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 69 deletions.
diff --git a/polaris-react/src/components/PositionedOverlay/PositionedOverlay.module.scss b/polaris-react/src/components/PositionedOverlay/PositionedOverlay.module.scss
@@ -1,6 +1,13 @@
+:root {
+  // stylelint-disable-next-line polaris/conventions/polaris/custom-property-allowed-list -- PositionedOverlay CSS Properties; Supports consuming component styles tying into changes in order to animate without hacky JavaScript timers and forcing reflow
+  --pc-positioned-overlay-top: initial;
+}
+
 .PositionedOverlay {
   position: absolute;
   z-index: var(--p-z-index-2);
+  // stylelint-disable-next-line polaris/conventions/polaris/custom-property-allowed-list -- PositionedOverlay CSS Properties
+  top: var(--pc-positioned-overlay-top);
 }
 
 .fixed {

diff --git a/polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx b/polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx
@@ -138,11 +138,17 @@ export class PositionedOverlay extends PureComponent<
       zIndexOverride,
     } = this.props;
 
+    const nextTop = top == null || isNaN(top) ? undefined : top;
+    const nextLeft = left == null || isNaN(left) ? undefined : left;
+    const nextRight = right == null || isNaN(right) ? undefined : right;
+    const nextWidth = width == null || isNaN(width) ? undefined : width;
+
     const style = {
-      top: top == null || isNaN(top) ? undefined : top,
-      left: left == null || isNaN(left) ? undefined : left,
-      right: right == null || isNaN(right) ? undefined : right,
-      width: width == null || isNaN(width) ? undefined : width,
+      '--pc-positioned-overlay-top': nextTop,
+      top: nextTop,
+      left: nextLeft,
+      right: nextRight,
+      width: nextWidth,
       zIndex: zIndexOverride || zIndex || undefined,
     };
 

diff --git a/polaris-react/src/components/PositionedOverlay/utilities/math.ts b/polaris-react/src/components/PositionedOverlay/utilities/math.ts
@@ -19,74 +19,71 @@ export function calculateVerticalPosition(
   activatorRect: Rect,
   overlayRect: Rect,
   overlayMargins: Margins,
-  scrollableContainerRect: Rect,
+  _scrollableContainerRect: Rect,
   containerRect: Rect,
   preferredPosition: PreferredPosition,
   fixed: boolean | undefined,
   topBarOffset = 0,
 ) {
+  const positionHorizontal =
+    preferredPosition === 'right' || preferredPosition === 'left';
   const activatorTop = activatorRect.top;
   const activatorBottom = activatorTop + activatorRect.height;
-  const spaceAbove = activatorRect.top - topBarOffset;
-  const spaceBelow =
-    containerRect.height - activatorRect.top - activatorRect.height;
-
-  const desiredHeight = overlayRect.height;
   const verticalMargins = overlayMargins.activator + overlayMargins.container;
-  const minimumSpaceToScroll = overlayMargins.container;
-  const distanceToTopScroll =
-    activatorRect.top - Math.max(scrollableContainerRect.top, 0);
-  const distanceToBottomScroll =
-    containerRect.top +
-    Math.min(
-      containerRect.height,
-      scrollableContainerRect.top + scrollableContainerRect.height,
-    ) -
-    (activatorRect.top + activatorRect.height);
-  const enoughSpaceFromTopScroll = distanceToTopScroll >= minimumSpaceToScroll;
-  const enoughSpaceFromBottomScroll =
-    distanceToBottomScroll >= minimumSpaceToScroll;
-  const heightIfAbove = Math.min(spaceAbove, desiredHeight);
-  const heightIfBelow = Math.min(spaceBelow, desiredHeight);
+  const minimumSurroundingSpace = verticalMargins ? verticalMargins : 16;
+  const spaceAbove = positionHorizontal
+    ? activatorRect.top + activatorRect.height - topBarOffset
+    : activatorRect.top - topBarOffset;
+  const spaceBelow = positionHorizontal
+    ? containerRect.height - activatorRect.top
+    : containerRect.height - activatorRect.top - activatorRect.height;
+  const desiredHeight = overlayRect.height;
+  const enoughSpaceFromTopEdge =
+    spaceAbove + minimumSurroundingSpace >= desiredHeight;
+  const enoughSpaceFromBottomEdge =
+    spaceBelow + minimumSurroundingSpace >= desiredHeight;
+  const heightIfAbove = Math.min(
+    spaceAbove - minimumSurroundingSpace,
+    desiredHeight,
+  );
+  const heightIfBelow = Math.min(
+    spaceBelow - minimumSurroundingSpace,
+    desiredHeight,
+  );
   const containerRectTop = fixed ? 0 : containerRect.top;
-
-  const positionIfAbove =
-    preferredPosition === 'right' || preferredPosition === 'left'
-      ? {
-          height: heightIfAbove - verticalMargins,
-          top: activatorBottom + containerRectTop - heightIfAbove,
-          positioning: 'above',
-        }
-      : {
-          height: heightIfAbove - verticalMargins,
-          top: activatorTop + containerRectTop - heightIfAbove,
-          positioning: 'above',
-        };
-
-  const positionIfBelow =
-    preferredPosition === 'right' || preferredPosition === 'left'
-      ? {
-          height: heightIfBelow - verticalMargins,
-          top: activatorTop + containerRectTop,
-          positioning: 'below',
-        }
-      : {
-          height: heightIfBelow - verticalMargins,
-          top: activatorBottom + containerRectTop,
-          positioning: 'below',
-        };
-
   const mostSpaceOnTop =
-    (enoughSpaceFromTopScroll ||
-      (distanceToTopScroll >= distanceToBottomScroll &&
-        !enoughSpaceFromBottomScroll)) &&
-    (spaceAbove > desiredHeight || spaceAbove > spaceBelow);
+    (enoughSpaceFromTopEdge ||
+      (spaceAbove >= spaceBelow && !enoughSpaceFromBottomEdge)) &&
+    (spaceAbove >= desiredHeight || spaceAbove >= spaceBelow);
 
   const mostSpaceOnBottom =
-    (enoughSpaceFromBottomScroll ||
-      (distanceToBottomScroll >= distanceToTopScroll &&
-        !enoughSpaceFromTopScroll)) &&
-    (spaceBelow > desiredHeight || spaceBelow > spaceAbove);
+    (enoughSpaceFromBottomEdge ||
+      (spaceBelow >= spaceAbove && !enoughSpaceFromTopEdge)) &&
+    (spaceBelow >= desiredHeight || spaceBelow >= spaceAbove);
+
+  const positionIfAbove = positionHorizontal
+    ? {
+        height: heightIfAbove - verticalMargins,
+        top: activatorBottom + containerRectTop - heightIfAbove,
+        positioning: 'above',
+      }
+    : {
+        height: heightIfAbove - verticalMargins,
+        top: activatorTop + containerRectTop - heightIfAbove,
+        positioning: 'above',
+      };
+
+  const positionIfBelow = positionHorizontal
+    ? {
+        height: heightIfBelow - verticalMargins,
+        top: activatorTop + containerRectTop,
+        positioning: 'below',
+      }
+    : {
+        height: heightIfBelow - verticalMargins,
+        top: activatorBottom + containerRectTop,
+        positioning: 'below',
+      };
 
   if (preferredPosition === 'above') {
     return mostSpaceOnTop ? positionIfAbove : positionIfBelow;
@@ -96,13 +93,13 @@ export function calculateVerticalPosition(
     return mostSpaceOnBottom ? positionIfBelow : positionIfAbove;
   }
 
-  if (enoughSpaceFromTopScroll && enoughSpaceFromBottomScroll) {
-    return spaceAbove > spaceBelow ? positionIfAbove : positionIfBelow;
+  if (enoughSpaceFromTopEdge && enoughSpaceFromBottomEdge) {
+    return spaceBelow + minimumSurroundingSpace >= desiredHeight
+      ? positionIfBelow
+      : positionIfAbove;
   }
 
-  return distanceToTopScroll > minimumSpaceToScroll
-    ? positionIfAbove
-    : positionIfBelow;
+  return mostSpaceOnBottom ? positionIfBelow : positionIfAbove;
 }
 
 export function calculateHorizontalPosition(
@@ -116,15 +113,19 @@ export function calculateHorizontalPosition(
   overlayMinWidth = 0,
 ) {
   const maximumWidth = containerRect.width - overlayRect.width;
+  const activatorRight =
+    containerRect.width - (activatorRect.left + activatorRect.width);
+
   const minimumSurroundingSpace = overlayMargins.horizontal
     ? overlayMargins.horizontal
     : 16;
-
   const desiredWidth = overlayRect.width;
   const distanceToLeftEdge = activatorRect.left;
   const distanceToRightEdge = containerRect.width - activatorRect.right;
-  const enoughSpaceFromLeftEdge = distanceToLeftEdge >= overlayMinWidth;
-  const enoughSpaceFromRightEdge = distanceToRightEdge >= overlayMinWidth;
+  const enoughSpaceFromLeftEdge =
+    distanceToLeftEdge >= (overlayMinWidth || desiredWidth);
+  const enoughSpaceFromRightEdge =
+    distanceToRightEdge >= (overlayMinWidth || desiredWidth);
 
   if (!preferredHorizontalPosition) {
     if (preferredAlignment === 'left') {
@@ -139,7 +140,7 @@ export function calculateHorizontalPosition(
       return {
         left: Math.min(
           maximumWidth,
-          Math.max(0, activatorRect.right - minimumSurroundingSpace),
+          Math.max(0, activatorRight - minimumSurroundingSpace),
         ),
         width: null,
       };