fix(desktop): keep horizontal content scrolls from navigating history

apps/web/src/components/HistorySwipeNavigation.browser.test.tsx

@@ -1,74 +1,129 @@
 import { describe, expect, it, vi } from "vite-plus/test";
 
-import { render } from "../../test/render";
+import { render, stubGlobal } from "../../test/render";
 import { HistorySwipeNavigation } from "./HistorySwipeNavigation";
 
-const navigateHistory = vi.hoisted(() => vi.fn());
-vi.mock("../navigationHistory", () => ({ canNavigateHistory: () => true, navigateHistory }));
+type Scroller = "shadow" | "none" | "shadow-dialog";
+type Step = readonly [
+  event: number | "begin" | "end" | "show-scroller" | "gain-overflow",
+  after?: "remove" | "close-dialog",
+];
 
-function swipe(target: Element, deltaX: number) {
-  for (const delta of [deltaX, 0]) {
-    target.dispatchEvent(new WheelEvent("wheel", { deltaX: delta, bubbles: true, composed: true }));
-  }
+function mountPanel(container: HTMLElement, scroller: Scroller, scrollLeft: number) {
+  const panel = container.appendChild(document.createElement("div"));
+  const root = scroller.startsWith("shadow") ? panel.attachShadow({ mode: "open" }) : panel;
+  const parent =
+    scroller === "shadow-dialog"
+      ? root.appendChild(Object.assign(document.createElement("div"), { role: "dialog" }))
+      : root;
+  const overflows = scroller === "shadow";
+  const viewport = parent.appendChild(document.createElement("div"));
+  viewport.style.cssText = `width: 200px; overflow-x: ${overflows ? "auto" : "visible"}`;
+  const content = viewport.appendChild(document.createElement("div"));
+  content.style.cssText = `height: 20px; width: ${overflows ? 1000 : 100}px`;
+  viewport.scrollLeft = scrollLeft;
+  return { viewport, content };
 }
 
 describe("HistorySwipeNavigation", () => {
-  it.each([
-    ["scrolling left inside a shadow-root scroller keeps the thread", "shadow", 100, -130, false],
-    ["scrolling right inside a shadow-root scroller keeps the thread", "shadow", 100, 130, false],
+  it.each<[string, Scroller, number, readonly Step[], "back" | "forward" | null]>([
+    ["scrolling left inside a shadow-root scroller", "shadow", 400, [[-130]], null],
+    ["swiping left at a shadow-root scroller's left edge", "shadow", 0, [[-130]], null],
+    ["a scroller removed after a small first event", "shadow", 400, [[1, "remove"], [130]], null],
     [
-      "scrolling after a shadow scroller remount keeps the thread",
-      "shadow-remount",
-      100,
-      130,
-      false,
+      "a history gesture after a content scroll ends",
+      "shadow",
+      400,
+      [[1, "remove"], ["end"], ["begin"], [130]],
+      "forward",
     ],
-    ["scrolling left inside a scroller keeps the thread", "light", 100, -130, false],
-    ["swiping back at a shadow-root scroller's left edge navigates", "shadow", 0, -130, true],
-    ["swiping back over a panel without overflow navigates", "none", 0, -130, true],
-    ["a swipe that started in a closed dialog keeps the thread", "closed-dialog", 0, -130, false],
-  ] as const)("%s", async (_name, scroller, scrollLeft, deltaX, navigates) => {
-    navigateHistory.mockReset();
+    [
+      "content appearing under tracking that a trailing history wheel started",
+      "none",
+      0,
+      [["begin"], [130], ["end"], [18], ["show-scroller"], ["begin"], [7], [130], ["end"]],
+      "forward",
+    ],
+    [
+      "a gesture that starts in a dialog before it closes",
+      "shadow-dialog",
+      0,
+      [[-1, "close-dialog"], [-130]],
+      null,
+    ],
+    [
+      "a history swipe over content replaced without overflow, released over a scroller",
+      "none",
+      0,
+      [[-60, "remove"], [-70], ["show-scroller"]],
+      "back",
+    ],
+    [
+      "a history swipe whose target gains overflow",
+      "none",
+      0,
+      [[-60], ["gain-overflow"], [-70]],
+      "back",
+    ],
+  ])("%s", async (_name, scroller, scrollLeft, steps, navigates) => {
     using _platform = vi.spyOn(navigator, "platform", "get").mockReturnValue("MacIntel");
+    const listeners = { begin: new Set<() => void>(), end: new Set<() => void>() };
+    using _bridge = stubGlobal("desktopBridge", {
+      onTrackpadScrollBegin: (listener: () => void) => {
+        listeners.begin.add(listener);
+        return () => listeners.begin.delete(listener);
+      },
+      onTrackpadScrollEnd: (listener: () => void) => {
+        listeners.end.add(listener);
+        return () => listeners.end.delete(listener);
+      },
+    });
     await using screen = await render(<HistorySwipeNavigation />);
-    const panel = screen.container.appendChild(document.createElement("div"));
-    const root = scroller.startsWith("shadow") ? panel.attachShadow({ mode: "open" }) : panel;
-    const viewport = root.appendChild(document.createElement("div"));
-    const noOverflow = scroller === "none" || scroller === "closed-dialog";
-    viewport.style.cssText = `width: 200px; overflow-x: ${noOverflow ? "visible" : "auto"}`;
-    const content = viewport.appendChild(document.createElement("div"));
-    content.style.cssText = `height: 20px; width: ${noOverflow ? 100 : 1000}px`;
-    viewport.scrollLeft = scrollLeft;
+    using back = vi.spyOn(window.history, "back").mockImplementation(() => undefined);
+    using forward = vi.spyOn(window.history, "forward").mockImplementation(() => undefined);
+    using _canGoBack = vi.spyOn(window.navigation, "canGoBack", "get").mockReturnValue(true);
+    using _canGoForward = vi.spyOn(window.navigation, "canGoForward", "get").mockReturnValue(true);
+    let { viewport, content } = mountPanel(screen.container, scroller, scrollLeft);
 
-    if (scroller === "closed-dialog") {
-      panel.setAttribute("role", "dialog");
-      content.dispatchEvent(new WheelEvent("wheel", { deltaX: -1, bubbles: true, composed: true }));
-      panel.remove();
-      screen.container.appendChild(viewport);
+    for (const [event, after] of [...steps, [0] as const]) {
+      if (event === "begin") {
+        for (const listener of listeners.begin) listener();
+      } else if (event === "end") {
+        for (const listener of listeners.end) listener();
+      } else if (event === "show-scroller") {
+        ({ viewport, content } = mountPanel(screen.container, "shadow", 400));
+      } else if (event === "gain-overflow") {
+        viewport.style.overflowX = "auto";
+        content.style.width = "1000px";
+      } else {
+        content.dispatchEvent(
+          new WheelEvent("wheel", { deltaX: event, bubbles: true, composed: true }),
+        );
+      }
+      if (after === "remove") {
+        viewport.remove();
+        ({ viewport, content } = mountPanel(screen.container, "none", 0));
+      } else if (after === "close-dialog") {
+        viewport.parentElement!.remove();
+        screen.container.appendChild(viewport);
+      }
     }
 
-    if (scroller === "shadow-remount") {
-      content.dispatchEvent(new WheelEvent("wheel", { deltaX: 1, bubbles: true, composed: true }));
-      const replacement = viewport.cloneNode(true) as HTMLDivElement;
-      viewport.replaceWith(replacement);
-      replacement.scrollLeft = scrollLeft;
-      swipe(replacement, deltaX);
-    } else {
-      swipe(content, deltaX);
-    }
-
-    expect(navigateHistory).toHaveBeenCalledTimes(navigates ? 1 : 0);
+    expect(back).toHaveBeenCalledTimes(navigates === "back" ? 1 : 0);
+    expect(forward).toHaveBeenCalledTimes(navigates === "forward" ? 1 : 0);
   });
 
   it("a horizontal mouse wheel tick off macOS keeps the thread", async () => {
-    navigateHistory.mockReset();
     using _platform = vi.spyOn(navigator, "platform", "get").mockReturnValue("Win32");
     await using screen = await render(<HistorySwipeNavigation />);
-    const content = screen.container.appendChild(document.createElement("div"));
-    content.style.cssText = "height: 20px; width: 100px";
+    using back = vi.spyOn(window.history, "back").mockImplementation(() => undefined);
+    using _canGoBack = vi.spyOn(window.navigation, "canGoBack", "get").mockReturnValue(true);
+    const { content } = mountPanel(screen.container, "none", 0);
 
-    swipe(content, -120);
+    for (const deltaX of [-120, 0]) {
+      content.dispatchEvent(new WheelEvent("wheel", { deltaX, bubbles: true, composed: true }));
+    }
 
-    expect(navigateHistory).not.toHaveBeenCalled();
+    expect(back).not.toHaveBeenCalled();
   });
 });

apps/web/src/components/HistorySwipeNavigation.tsx

@@ -19,6 +19,7 @@ type SwipeGesture =
       deltas: number[];
       y: number;
       bounds: SwipeBounds;
+      target: EventTarget | undefined;
     }
   // Not a swipe, or one already decided: the rest of its events are ignored.
   | { kind: "ignored" };
@@ -37,30 +38,25 @@ function readMainContentBounds(): SwipeBounds {
   return { left, top, width, height };
 }
 
-// `event.target` stops at a shadow host; the composed path also reaches scrollers inside it.
-function canScrollHorizontally(path: readonly EventTarget[], deltaX: number): boolean {
-  for (const element of path) {
-    if (!(element instanceof Element) || element.scrollWidth <= element.clientWidth) continue;
+// The composed path reaches scrollers inside shadow roots, such as the diff viewer's.
+// A scroller at its edge still owns the gesture, so history never takes a content scroll.
+function isInHorizontalScroller(path: readonly EventTarget[]): boolean {
+  return path.some((element) => {
+    if (!(element instanceof Element) || element.scrollWidth <= element.clientWidth) return false;
     const { overflowX } = getComputedStyle(element);
-    if (overflowX !== "auto" && overflowX !== "scroll") continue;
-    const canScroll =
-      deltaX < 0
-        ? element.scrollLeft > 0
-        : element.scrollLeft + element.clientWidth < element.scrollWidth - 1;
-    if (canScroll) return true;
-  }
-  return false;
+    return overflowX === "auto" || overflowX === "scroll";
+  });
 }
 
 function lockSwipeAxis(
   pending: Extract<SwipeGesture, { kind: "pending" }>,
+  target: EventTarget | undefined,
 ): Exclude<SwipeGesture, { kind: "pending" }> {
   const { path, sumX, sumY } = pending;
   if (Math.abs(sumX) <= Math.abs(sumY)) return { kind: "ignored" };
   if (path.some((node) => node instanceof Element && node.matches('[role="dialog"]'))) {
     return { kind: "ignored" };
   }
-  if (canScrollHorizontally(path, sumX)) return { kind: "ignored" };
   const direction = sumX < 0 ? "back" : "forward";
   if (!canNavigateHistory(direction)) return { kind: "ignored" };
   return {
@@ -70,6 +66,7 @@ function lockSwipeAxis(
     deltas: [],
     y: pending.y,
     bounds: readMainContentBounds(),
+    target,
   };
 }
 
@@ -125,15 +122,29 @@ export function HistorySwipeNavigation() {
             : { kind: "pending", path: event.composedPath(), y: event.clientY, sumX: 0, sumY: 0 };
       }
       if (gesture.kind === "ignored") return;
+      if (event.deltaX === 0 && event.deltaY === 0) {
+        if (gesture.kind === "tracking") release();
+        return;
+      }
+      const path = event.composedPath();
+      // Pending wheels are checked while a scroller is still laid out, before it can be removed.
+      // Tracking rechecks only a new target, so overflow gained under a swipe stays history.
+      if (
+        (gesture.kind === "pending" || path[0] !== gesture.target) &&
+        isInHorizontalScroller(path)
+      ) {
+        gesture = { kind: "ignored" };
+        return hideIndicator();
+      }
       if (gesture.kind === "pending") {
         gesture.sumX += event.deltaX;
         gesture.sumY += event.deltaY;
         if (Math.abs(gesture.sumX) + Math.abs(gesture.sumY) < SWIPE_AXIS_LOCK_DISTANCE) return;
-        gesture = lockSwipeAxis({ ...gesture, path: [...gesture.path, ...event.composedPath()] });
+        gesture = lockSwipeAxis({ ...gesture, path: [...gesture.path, ...path] }, path[0]);
         if (gesture.kind === "ignored") return;
       } else {
+        gesture.target = path[0];
         const delta = gesture.direction === "back" ? -event.deltaX : event.deltaX;
-        if (event.deltaX === 0 && event.deltaY === 0) return release();
         gesture.deltas = [...gesture.deltas.slice(1 - MOMENTUM_SAMPLE_SIZE), delta];
         if (isMomentumTail(gesture.deltas)) return release();
         gesture.distance = Math.max(0, gesture.distance + delta);

apps/web/src/components/RightPanelTabs.browser.test.tsx

@@ -64,7 +64,7 @@ const surfaces: RightPanelSurface[] = Array.from({ length: 16 }, (_, index) => (
 }));
 
 describe("RightPanelTabs", () => {
-  it("a flick that scrolls the tab strip to its left edge keeps the thread, a swipe from the edge goes back", async () => {
+  it("a flick that scrolls the tab strip to its left edge and a swipe from that edge keep the thread", async () => {
     navigateHistory.mockReset();
     const { bridge, scroll } = trackpad();
     using _bridge = stubGlobal("desktopBridge", bridge);
@@ -135,7 +135,7 @@ describe("RightPanelTabs", () => {
     viewport.scrollLeft = 0;
     scroll(viewport.querySelector("[data-active-tab]")!, [-40, -40, -40, -40, -40]);
 
-    expect(navigateHistory).toHaveBeenCalledExactlyOnceWith("back");
+    expect(navigateHistory).not.toHaveBeenCalled();
   });
 });