fix(browser): keep the annotation composer off the element it annotates (#26596)

Placement only measured the gap below the target, so a tall target or a short pane sent the card to a side with no room and Radix slid it back over the selection.
2026-11-03 18:47:40 +00:00 · 2026-10-08 13:14:47 -07:00
parent 6ceb423599
commit 23fd5e6869
9 changed files with 161 additions and 60 deletions
@@ -51,7 +51,7 @@ export function PendingBrowserAnnotationCard({
        />
      </PopoverAnchor>
      <PopoverContent
-        side={anchor.below ? 'bottom' : 'top'}
+        side={anchor.side}
        align="center"
        sideOffset={10}
        collisionBoundary={portalContainer ?? undefined}
@@ -67,15 +67,16 @@ export function PendingBrowserAnnotationCard({
          onCancel()
        }}
      >
-        <div className="mb-2 min-w-0">
-          <div className="truncate text-xs font-medium text-foreground">
+        {/* Selector lives in the tooltip: truncated to a popover width it reads as noise. */}
+        <div className="mb-2 flex min-w-0 items-center gap-1.5" title={payload.target.selector}>
+          <span className="shrink-0 rounded bg-muted px-1 py-0.5 font-mono text-[10px] leading-none text-muted-foreground">
+            {payload.target.tagName}
+          </span>
+          <span className="truncate text-xs font-medium text-foreground">
            {payload.target.accessibility.accessibleName ||
              payload.target.textSnippet ||
-              payload.target.tagName}
-          </div>
-          <div className="mt-0.5 truncate font-mono text-[11px] text-muted-foreground">
-            {payload.target.selector}
-          </div>
+              payload.target.selector}
+          </span>
        </div>
        <Label htmlFor="browser-annotation-comment" className="sr-only">
          {translate('auto.components.browser.pane.BrowserPane.d2a7092e6e', 'Annotation comment')}
@@ -107,42 +108,37 @@ export function PendingBrowserAnnotationCard({
            }
          }}
        />
-        <div className="mt-2 min-w-0">
-          <Label className="mb-1 block text-xs text-muted-foreground">
-            {translate('auto.components.browser.pane.BrowserPane.8f87e6c2e5', 'Intent')}
-          </Label>
-          <ToggleGroup
-            type="single"
-            size="sm"
-            variant="outline"
-            value={intent}
-            onValueChange={(value) => {
-              if (value) {
-                setIntent(value as BrowserAnnotationIntent)
-              }
-            }}
-            className="h-8 w-full [&_[data-slot=toggle-group-item]]:h-8 [&_[data-slot=toggle-group-item]]:flex-1 [&_[data-slot=toggle-group-item]]:px-2"
-            aria-label={translate(
-              'auto.components.browser.pane.BrowserPane.0cb3bd6221',
-              'Annotation intent'
-            )}
-          >
-            {BROWSER_ANNOTATION_INTENT_OPTIONS.map((option) => {
-              const Icon = option.icon
-              return (
-                <ToggleGroupItem
-                  key={option.value}
-                  value={option.value}
-                  aria-label={option.label}
-                  className="gap-1.5 text-xs data-[state=on]:border-foreground/20 data-[state=on]:bg-foreground/10 data-[state=on]:text-foreground data-[state=on]:shadow-xs data-[state=on]:hover:bg-foreground/15 data-[state=on]:hover:text-foreground"
-                >
-                  <Icon className="size-3.5" />
-                  <span>{option.label}</span>
-                </ToggleGroupItem>
-              )
-            })}
-          </ToggleGroup>
-        </div>
+        <ToggleGroup
+          type="single"
+          size="sm"
+          variant="outline"
+          value={intent}
+          onValueChange={(value) => {
+            if (value) {
+              setIntent(value as BrowserAnnotationIntent)
+            }
+          }}
+          className="mt-2 h-8 w-full [&_[data-slot=toggle-group-item]]:h-8 [&_[data-slot=toggle-group-item]]:flex-1"
+          aria-label={translate(
+            'auto.components.browser.pane.BrowserPane.0cb3bd6221',
+            'Annotation intent'
+          )}
+        >
+          {BROWSER_ANNOTATION_INTENT_OPTIONS.map((option) => {
+            const Icon = option.icon
+            return (
+              <ToggleGroupItem
+                key={option.value}
+                value={option.value}
+                aria-label={option.label}
+                className="gap-1.5 text-xs data-[state=on]:border-foreground/20 data-[state=on]:bg-foreground/10 data-[state=on]:text-foreground data-[state=on]:shadow-xs data-[state=on]:hover:bg-foreground/15 data-[state=on]:hover:text-foreground"
+              >
+                <Icon className="size-3.5" />
+                <span>{option.label}</span>
+              </ToggleGroupItem>
+            )
+          })}
+        </ToggleGroup>
        <div className="mt-3 flex justify-end gap-2">
          <Button size="sm" variant="ghost" className="h-8" onClick={onCancel}>
            {translate('auto.components.browser.pane.BrowserPane.fa6ea61de3', 'Cancel')}
@@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'
 import type { BrowserGrabPayload } from '../../../../../shared/browser-grab-types'
 import {
  createBrowserAnnotationPayload,
+  getBrowserOverlayAnchor,
  getLiveBrowserAnnotationRect
 } from './browser-annotation-geometry'

@@ -24,6 +25,62 @@ function payload(overrides: Partial<BrowserGrabPayload['target']> = {}): Browser
  } as unknown as BrowserGrabPayload
 }

+/** Anchor for a target of the given viewport box, inside a pane of the given size. */
+function anchorFor(
+  rect: { x: number; y: number; width: number; height: number },
+  paneWidth: number,
+  paneHeight: number
+): ReturnType<typeof getBrowserOverlayAnchor> {
+  const pane = {
+    getBoundingClientRect: () => ({ left: 0, top: 0, width: paneWidth, height: paneHeight })
+  }
+  return getBrowserOverlayAnchor(payload({ isFixed: true, rectViewport: rect }), pane, pane, {
+    scrollX: 0,
+    scrollY: 0,
+    version: 0
+  })
+}
+
+describe('browser annotation composer placement', () => {
+  it('sits below a target with room underneath', () => {
+    expect(anchorFor({ x: 100, y: 40, width: 185, height: 16 }, 900, 900)).toEqual({
+      x: 192.5,
+      y: 56,
+      side: 'bottom'
+    })
+  })
+
+  it('sits above a target with room overhead but not underneath', () => {
+    expect(anchorFor({ x: 100, y: 700, width: 185, height: 16 }, 900, 900)).toEqual({
+      x: 192.5,
+      y: 700,
+      side: 'top'
+    })
+  })
+
+  // The reported bug: a target too tall to clear vertically used to put the card on top of it.
+  it('moves beside a target that leaves no vertical room', () => {
+    expect(anchorFor({ x: 300, y: 150, width: 300, height: 300 }, 1400, 600)).toEqual({
+      x: 600,
+      y: 300,
+      side: 'right'
+    })
+  })
+
+  it('falls to the left band when the right one is too narrow', () => {
+    expect(anchorFor({ x: 800, y: 150, width: 300, height: 300 }, 1400, 600).side).toBe('left')
+  })
+
+  // Nothing fits here, so the card overlaps wherever Radix ends up putting it — this pins the
+  // anchor only. Where the card finally lands is Radix's to decide and is measured against the
+  // real popover in a browser, which jsdom cannot do.
+  it('anchors to the deeper vertical band when no side can hold the card', () => {
+    const anchor = anchorFor({ x: 20, y: 20, width: 860, height: 560 }, 900, 600)
+    expect(anchor.side).toBe('bottom')
+    expect(anchor.y).toBe(580)
+  })
+})
+
 describe('browser annotation geometry', () => {
  it('uses the viewport rect for fixed targets and scroll-adjusts flowing targets', () => {
    const fixed = payload({ isFixed: true })
@@ -8,10 +8,17 @@ import type {
 } from '../../../../../shared/browser-grab-types'
 import { translate } from '@/i18n/i18n'

+export type BrowserOverlaySide = 'top' | 'bottom' | 'left' | 'right'
+
+/** The pane and the guest view, narrowed to the measurement the anchor needs from each. */
+type OverlayRectSource = {
+  getBoundingClientRect: () => { left: number; top: number; width: number; height: number }
+}
+
 export type BrowserOverlayAnchor = {
  x: number
  y: number
-  below: boolean
+  side: BrowserOverlaySide
 }

 export const BROWSER_ANNOTATION_INTENT_OPTIONS = [
@@ -42,7 +49,13 @@ export type BrowserOverlayViewport = {
 }

 export const EMPTY_BROWSER_ANNOTATIONS: BrowserPageAnnotation[] = []
-export const PENDING_ANNOTATION_CARD_HEIGHT = 330
+
+// Footprint of PendingBrowserAnnotationCard (measured at 352x237), rounded up: it never grows
+// past this, so a band narrower than it plus the margin below cannot hold the card.
+const PENDING_ANNOTATION_CARD_HEIGHT = 240
+const PENDING_ANNOTATION_CARD_WIDTH = 352
+// PopoverContent's sideOffset (10) plus its collisionPadding (12).
+const PENDING_ANNOTATION_CARD_MARGIN = 22

 export function createBrowserAnnotationId(): string {
  return `browser-annotation-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`
@@ -58,10 +71,38 @@ export function createBrowserAnnotationPayload(
  }
 }

+// Preference order: a later side is only reached when every earlier one is too tight.
+const BROWSER_OVERLAY_SIDES = ['bottom', 'top', 'right', 'left'] as const
+
+/**
+ * Side of the target whose free band can hold the card.
+ *
+ * Why every side and not just below/above: Radix shifts a popover back along its own axis when
+ * the named side cannot hold it, so naming a side that does not fit lands the card *on* the
+ * target instead of beside it. A tall target, or a short pane, leaves neither vertical band deep
+ * enough — the horizontal ones usually still are.
+ */
+function pickBrowserOverlaySide(space: Record<BrowserOverlaySide, number>): BrowserOverlaySide {
+  const vertical = PENDING_ANNOTATION_CARD_HEIGHT + PENDING_ANNOTATION_CARD_MARGIN
+  const horizontal = PENDING_ANNOTATION_CARD_WIDTH + PENDING_ANNOTATION_CARD_MARGIN
+  const room: Record<BrowserOverlaySide, number> = {
+    bottom: space.bottom / vertical,
+    top: space.top / vertical,
+    right: space.right / horizontal,
+    left: space.left / horizontal
+  }
+  const fitting = BROWSER_OVERLAY_SIDES.find((side) => room[side] >= 1)
+  // No band fits, so overlap is unavoidable and Radix still flips and shifts to keep the card
+  // on screen — the deeper vertical band only makes it overlap least. Letting it cover part of
+  // the target beats the alternatives (anchoring to the target rect, or turning collision
+  // handling off), which both park ~95% of the composer outside the pane.
+  return fitting ?? (room.bottom >= room.top ? 'bottom' : 'top')
+}
+
 export function getBrowserOverlayAnchor(
  payload: BrowserGrabPayload,
-  container: HTMLElement | null,
-  webview: Electron.WebviewTag | null,
+  container: OverlayRectSource | null,
+  webview: OverlayRectSource | null,
  viewport: BrowserOverlayViewport
 ): BrowserOverlayAnchor {
  const containerRect = container?.getBoundingClientRect()
@@ -69,15 +110,28 @@ export function getBrowserOverlayAnchor(
  const rect = getLiveBrowserAnnotationRect(payload, viewport)
  const offsetX = (webviewRect?.left ?? 0) - (containerRect?.left ?? 0)
  const offsetY = (webviewRect?.top ?? 0) - (containerRect?.top ?? 0)
-  const elementBottom = offsetY + rect.y + rect.height
-  const elementTop = offsetY + rect.y
  const containerWidth = containerRect?.width ?? 0
  const containerHeight = containerRect?.height ?? 0
-  const below = elementBottom + PENDING_ANNOTATION_CARD_HEIGHT < containerHeight
+  const elementLeft = offsetX + rect.x
+  const elementRight = elementLeft + rect.width
+  const elementTop = offsetY + rect.y
+  const elementBottom = elementTop + rect.height
+  const side = pickBrowserOverlaySide({
+    top: elementTop,
+    bottom: containerHeight - elementBottom,
+    left: elementLeft,
+    right: containerWidth - elementRight
+  })
+  const edge = {
+    top: { x: elementLeft + rect.width / 2, y: elementTop },
+    bottom: { x: elementLeft + rect.width / 2, y: elementBottom },
+    left: { x: elementLeft, y: elementTop + rect.height / 2 },
+    right: { x: elementRight, y: elementTop + rect.height / 2 }
+  }[side]
  return {
-    x: clampNumber(offsetX + rect.x + rect.width / 2, 12, Math.max(12, containerWidth - 12)),
-    y: clampNumber(below ? elementBottom : elementTop, 12, Math.max(12, containerHeight - 12)),
-    below
+    x: clampNumber(edge.x, 12, Math.max(12, containerWidth - 12)),
+    y: clampNumber(edge.y, 12, Math.max(12, containerHeight - 12)),
+    side
  }
 }

@@ -16379,7 +16379,6 @@
            "deb5293610": "Browser annotations unavailable in remote runtime",
            "90d021f2ad": "Add",
            "0cb3bd6221": "Annotation intent",
-            "8f87e6c2e5": "Intent",
            "532bac48c5": "Describe what the agent should change here...",
            "d2a7092e6e": "Annotation comment",
            "b472c5fe03": "Add browser annotation",
@@ -16150,7 +16150,6 @@
            "deb5293610": "Anotaciones del navegador no disponibles en runtime remoto",
            "90d021f2ad": "Agregar",
            "0cb3bd6221": "Intención de anotación",
-            "8f87e6c2e5": "Intención",
            "532bac48c5": "Describe aquí lo que el agente debería cambiar...",
            "d2a7092e6e": "Comentario de anotación",
            "b472c5fe03": "Agregar anotación del navegador",
@@ -16255,7 +16255,6 @@
            "deb5293610": "Annotations du navigateur indisponibles dans un runtime distant",
            "90d021f2ad": "Ajouter",
            "0cb3bd6221": "Intention d'annotation",
-            "8f87e6c2e5": "Intention",
            "532bac48c5": "Décrivez ce que l'agent doit modifier ici...",
            "d2a7092e6e": "Commentaire d'annotation",
            "b472c5fe03": "Ajouter une annotation au navigateur",
@@ -16150,7 +16150,6 @@
            "deb5293610": "ブラウザの注釈はリモートランタイムでは使用できません",
            "90d021f2ad": "追加",
            "0cb3bd6221": "注釈の意図",
-            "8f87e6c2e5": "意図",
            "532bac48c5": "Agent がここで何を変更する必要があるかを説明してください…",
            "d2a7092e6e": "注釈コメント",
            "b472c5fe03": "ブラウザの注釈を追加する",
@@ -16150,7 +16150,6 @@
            "deb5293610": "원격 런타임에서는 브라우저 주석을 사용할 수 없습니다.",
            "90d021f2ad": "추가",
            "0cb3bd6221": "주석 의도",
-            "8f87e6c2e5": "의도",
            "532bac48c5": "여기에서 agent가 무엇을 변경해야 하는지 설명하세요...",
            "d2a7092e6e": "주석 코멘트",
            "b472c5fe03": "브라우저 주석 추가",
@@ -16150,7 +16150,6 @@
            "deb5293610": "浏览器注释在远程运行时不可用",
            "90d021f2ad": "添加",
            "0cb3bd6221": "注释意图",
-            "8f87e6c2e5": "意图",
            "532bac48c5": "描述智能体应该在这里改变什么......",
            "d2a7092e6e": "注释评论",
            "b472c5fe03": "添加浏览器注释",