fix(ui mode): filter dialog positioning (#37619)

Author: dgozman

packages/html-reporter/src/headerView.tsx

@@ -123,21 +123,24 @@ const SettingsButton: React.FC = () => {
   const [darkMode, setDarkMode] = useDarkModeSetting();
   const [mergeFiles, setMergeFiles] = useSetting('mergeFiles', false);
 
-  return <div
-    role='button'
-    ref={settingsRef}
-    style={{ cursor: 'pointer' }}
-    className='subnav-item'
-    title='Settings'
-    onClick={e => {
-      setSettingsOpen(!settingsOpen);
-      e.preventDefault();
-    }}
-    onMouseDown={preventDefault}>
+  return <>
+    <div
+      role='button'
+      ref={settingsRef}
+      style={{ cursor: 'pointer' }}
+      className='subnav-item'
+      title='Settings'
+      onClick={e => {
+        setSettingsOpen(!settingsOpen);
+        e.preventDefault();
+      }}
+      onMouseDown={preventDefault}>
+      {icons.settings()}
+    </div>
     <Dialog
       open={settingsOpen}
-      width={150}
-      verticalOffset={8}
+      minWidth={150}
+      verticalOffset={4}
       requestClose={() => setSettingsOpen(false)}
       anchor={settingsRef}
       dataTestId='settings-dialog'
@@ -151,8 +154,7 @@ const SettingsButton: React.FC = () => {
         Merge files
       </label>
     </Dialog>
-    {icons.settings()}
-  </div>;
+  </>;
 };
 
 const preventDefault = (e: any) => {

packages/injected/src/highlight.ts

@@ -215,6 +215,7 @@ export class Highlight {
     };
   }
 
+  // Note: there is a copy of this method in dialog.tsx. Please fix bugs in both places.
   tooltipPosition(box: DOMRect, tooltipElement: HTMLElement) {
     const tooltipWidth = tooltipElement.offsetWidth;
     const tooltipHeight = tooltipElement.offsetHeight;

packages/recorder/src/recorder.tsx

@@ -198,7 +198,6 @@ export const Recorder: React.FC<RecorderProps> = ({
       <Dialog
         style={{ padding: '4px 8px' }}
         open={settingsOpen}
-        width={200}
         verticalOffset={8}
         requestClose={() => setSettingsOpen(false)}
         anchor={settingsButtonRef}

packages/web/src/components/dialogToolbarButton.tsx

@@ -41,7 +41,6 @@ export const DialogToolbarButton: React.FC<React.PropsWithChildren<DialogToolbar
           padding: '4px 8px'
         }}
         open={open}
-        width={200}
         // TODO: Temporary spacing until design of toolbar buttons is revisited
         verticalOffset={8}
         requestClose={() => setOpen(false)}

packages/web/src/shared/dialog.tsx

@@ -16,12 +16,14 @@
 
 import * as React from 'react';
 
+import { useMeasureForRef } from '../uiUtils';
+
 export interface DialogProps {
   className?: string;
   style?: React.CSSProperties;
   open: boolean;
   isModal?: boolean;
-  width?: number;
+  minWidth?: number;
   verticalOffset?: number;
   requestClose?: () => void;
   anchor?: React.RefObject<HTMLElement|null>;
@@ -33,7 +35,7 @@ export const Dialog: React.FC<React.PropsWithChildren<DialogProps>> = ({
   style: externalStyle,
   open,
   isModal,
-  width,
+  minWidth,
   verticalOffset,
   requestClose,
   anchor,
@@ -44,22 +46,9 @@ export const Dialog: React.FC<React.PropsWithChildren<DialogProps>> = ({
 
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   const [_, setRecalculateDimensionsCount] = React.useState(0);
-
-  let style: React.CSSProperties | undefined = externalStyle;
-
-  if (anchor?.current) {
-    const bounds = anchor.current.getBoundingClientRect();
-
-    style = {
-      position: 'fixed',
-      margin: 0,
-      top: bounds.bottom + (verticalOffset ?? 0),
-      left: buildTopLeftCoord(bounds, width ?? 0),
-      width,
-      zIndex: 110,  // on top of split view resizer
-      ...externalStyle
-    };
-  }
+  const dialogMeasure = useMeasureForRef(dialogRef);
+  const anchorMeasure = useMeasureForRef(anchor);
+  const position = dialogPosition(dialogMeasure, anchorMeasure, verticalOffset);
 
   React.useEffect(() => {
     const onClick = (event: MouseEvent) => {
@@ -113,53 +102,34 @@ export const Dialog: React.FC<React.PropsWithChildren<DialogProps>> = ({
   }, [open, isModal]);
 
   return (
-    <dialog ref={dialogRef} style={style} className={className} data-testid={dataTestId}>
+    <dialog ref={dialogRef} style={{
+      position: 'fixed',
+      margin: 0,
+      zIndex: 110,  // on top of split view resizer
+      top: position.top,
+      left: position.left,
+      minWidth: minWidth || 0,
+      ...externalStyle,
+    }} className={className} data-testid={dataTestId}>
       {children}
     </dialog>
   );
 };
 
-const buildTopLeftCoord = (bounds: DOMRect, width: number): number => {
-  const leftAlignCoord = buildTopLeftCoordWithAlignment(bounds, width, 'left');
-
-  if (leftAlignCoord.inBounds)
-    return leftAlignCoord.value;
-
-  const rightAlignCoord = buildTopLeftCoordWithAlignment(
-      bounds,
-      width,
-      'right'
-  );
-
-  if (rightAlignCoord.inBounds)
-    return rightAlignCoord.value;
-
-  return leftAlignCoord.value;
-};
-
-const buildTopLeftCoordWithAlignment = (
-  bounds: DOMRect,
-  width: number,
-  alignment: 'left' | 'right'
-): {
-  value: number;
-  inBounds: boolean;
-} => {
-  const maxLeft = document.documentElement.clientWidth;
-
-  if (alignment === 'left') {
-    const value = bounds.left;
-
-    return {
-      value,
-      inBounds: value + width <= maxLeft,
-    };
-  } else {
-    const value = bounds.right - width;
-
-    return {
-      value,
-      inBounds: bounds.right - width >= 0,
-    };
+// Note: there is a copy of this method in highlight.ts. Please fix bugs in both places.
+function dialogPosition(dialogBox: DOMRect, anchorBox: DOMRect, verticalOffset = 4, horizontalOffset = 4): { top: number, left: number } {
+  let left = Math.max(horizontalOffset, anchorBox.left);
+  if (left + dialogBox.width > window.innerWidth - horizontalOffset)
+    left = window.innerWidth - dialogBox.width - horizontalOffset;
+  let top = Math.max(0, anchorBox.bottom) + verticalOffset;
+  if (top + dialogBox.height > window.innerHeight - verticalOffset) {
+    // If can't fit below, either position above...
+    if (Math.max(0, anchorBox.top) > dialogBox.height + verticalOffset) {
+      top = Math.max(0, anchorBox.top) - dialogBox.height - verticalOffset;
+    } else {
+      // Or on top in case of large element
+      top = window.innerHeight - verticalOffset - dialogBox.height;
+    }
   }
-};
+  return { left, top };
+}

packages/web/src/uiUtils.ts

@@ -16,8 +16,6 @@
 
 import React from 'react';
 
-import type { EffectCallback } from 'react';
-
 // Recalculates the value when dependencies change.
 export function useAsyncMemo<T>(fn: () => Promise<T>, deps: React.DependencyList, initialValue: T, resetValue?: T) {
   const [value, setValue] = React.useState<T>(initialValue);
@@ -37,28 +35,29 @@ export function useAsyncMemo<T>(fn: () => Promise<T>, deps: React.DependencyList
   return value;
 }
 
-// Tracks the element size and returns it's contentRect (always has x=0, y=0).
+// Tracks the element's bounding box.
 export function useMeasure<T extends Element>() {
   const ref = React.useRef<T | null>(null);
+  return [useMeasureForRef(ref), ref] as const;
+}
+
+export function useMeasureForRef<T extends Element>(ref?: React.RefObject<T | null>) {
   const [measure, setMeasure] = React.useState(new DOMRect(0, 0, 10, 10));
   React.useLayoutEffect(() => {
-    const target = ref.current;
+    const target = ref?.current;
     if (!target)
       return;
-
-    const bounds = target.getBoundingClientRect();
-
-    setMeasure(new DOMRect(0, 0, bounds.width, bounds.height));
-
-    const resizeObserver = new ResizeObserver((entries: any) => {
-      const entry = entries[entries.length - 1];
-      if (entry && entry.contentRect)
-        setMeasure(entry.contentRect);
-    });
+    const update = () => setMeasure(target.getBoundingClientRect());
+    update();
+    const resizeObserver = new ResizeObserver(update);
     resizeObserver.observe(target);
-    return () => resizeObserver.disconnect();
+    window.addEventListener('resize', update);
+    return () => {
+      resizeObserver.disconnect();
+      window.removeEventListener('resize', update);
+    };
   }, [ref]);
-  return [measure, ref] as const;
+  return measure;
 }
 
 export function msToString(ms: number): string {
@@ -233,7 +232,7 @@ export const kWebLinkRe = new RegExp('(?:[a-zA-Z][a-zA-Z0-9+.-]{2,}:\\/\\/|www\\
  * If `trigger` is called while a flash is ongoing, the ongoing flash will be cancelled and after 50ms a new flash is started.
  * @returns [flash, trigger]
  */
-export function useFlash(): [boolean, EffectCallback] {
+export function useFlash(): [boolean, React.EffectCallback] {
   const [flash, setFlash] = React.useState(false);
   const trigger = React.useCallback<React.EffectCallback>(() => {
     const timeouts: any[] = [];