fix(frontend/table): stop the grid scrolling when dragging a scrollbar

.pre-commit-config.yaml

@@ -11,12 +11,14 @@ repos:
           - id: check-merge-conflict
           - id: check-toml
           - id: check-yaml
+          # Patch files carry significant whitespace: a blank context line is a
+          # single space, and normalising it stops pnpm applying the patch.
           - id: end-of-file-fixer
             stages: [pre-commit]
-            exclude: ^frontend/apps/demo/public/
+            exclude: ^frontend/(apps/demo/public/|patches/)
           - id: trailing-whitespace
             stages: [pre-commit]
-            exclude: ^frontend/apps/demo/public/
+            exclude: ^frontend/(apps/demo/public/|patches/)
 
     # Recorded HTTP cassettes are scrubbed at record time; this gate catches
     # anything the scrubbing missed before it can be committed. Known-benign

frontend/apps/app/Dockerfile

@@ -12,8 +12,10 @@ ARG APP_DIR
 RUN corepack enable
 WORKDIR /usr/src/app
 
-# Prepare workspace
+# Prepare workspace. pnpm hashes every file named by patchedDependencies before
+# it resolves anything, so patches/ must land before the source tree does.
 COPY package.json pnpm-lock.yaml pnpm-workspace.yaml ./
+COPY patches/ patches/
 COPY ${APP_DIR}/package.json ${APP_DIR}/
 COPY packages/ui/package.json packages/ui/
 COPY packages/config/package.json packages/config/

frontend/apps/demo/Dockerfile

@@ -12,8 +12,10 @@ ARG APP_DIR
 RUN corepack enable
 WORKDIR /usr/src/app
 
-# Prepare workspace
+# Prepare workspace. pnpm hashes every file named by patchedDependencies before
+# it resolves anything, so patches/ must land before the source tree does.
 COPY package.json pnpm-lock.yaml pnpm-workspace.yaml ./
+COPY patches/ patches/
 COPY ${APP_DIR}/package.json ${APP_DIR}/
 COPY packages/ui/package.json packages/ui/
 COPY packages/config/package.json packages/config/

frontend/apps/site/Dockerfile

@@ -12,8 +12,10 @@ ARG APP_DIR
 RUN corepack enable
 WORKDIR /usr/src/app
 
-# Prepare workspace
+# Prepare workspace. pnpm hashes every file named by patchedDependencies before
+# it resolves anything, so patches/ must land before the source tree does.
 COPY package.json pnpm-lock.yaml pnpm-workspace.yaml ./
+COPY patches/ patches/
 COPY ${APP_DIR}/package.json ${APP_DIR}/
 COPY packages/ui/package.json packages/ui/
 COPY packages/config/package.json packages/config/

frontend/e2e/support/grid.ts

@@ -1,4 +1,4 @@
-import { expect, type Page } from '@playwright/test'
+import { expect, type Locator, type Page } from '@playwright/test'
 
 // The grid is a Glide <canvas>. Actions that must reach Glide's canvas mouse
 // handler (hover, context menu, cell/column selection, activation) are driven by
@@ -19,6 +19,7 @@ export const ROW_HEIGHT = 34
 
 export type Box = { x: number; y: number; width: number; height: number }
 export type Cell = { col: number; row: number }
+export type Point = { x: number; y: number }
 
 // Opening a plot switches to the Plots tab, which unmounts the table, so the
 // canvas may be absent when a later grid action runs. Wait for it before reading
@@ -56,3 +57,57 @@ export function cellPoint(box: Box, { col, row }: Cell) {
     y: box.y + HEADER_HEIGHT + row * ROW_HEIGHT + ROW_HEIGHT / 2,
   }
 }
+
+export type Axis = 'horizontal' | 'vertical'
+
+// The element that carries the grid's scrollbars, which occupy the strip
+// between its client box and its border box. One owner: the class tracks Glide.
+export function gridScroller(page: Page): Locator {
+  return page.locator('.dvn-scroller')
+}
+
+// The grid mounts its scroller before the padders give it a scroll range, so an
+// early read can land on a grid that cannot scroll yet. Poll this.
+export async function canScroll(page: Page, axis: Axis): Promise<boolean> {
+  const track = await readTrack(gridScroller(page), axis)
+  return track.scroll > track.client
+}
+
+// The centre of a scrollbar thumb, in page coordinates, ready for page.mouse.
+export async function scrollbarThumb(page: Page, axis: Axis): Promise<Point> {
+  const scroller = gridScroller(page)
+  await expect(scroller).toBeVisible()
+  await expect.poll(() => canScroll(page, axis)).toBe(true)
+
+  const track = await readTrack(scroller, axis)
+  if (track.thickness === 0) {
+    throw new Error(
+      `the ${axis} scrollbar takes no layout space, so it cannot be pressed; launch the browser without --hide-scrollbars`
+    )
+  }
+
+  const length = (track.client * track.client) / track.scroll
+  const along = (track.offset / track.scroll) * track.client + length / 2
+  return axis === 'horizontal'
+    ? { x: track.x + along, y: track.y + track.height - track.thickness / 2 }
+    : { x: track.x + track.width - track.thickness / 2, y: track.y + along }
+}
+
+function readTrack(scroller: Locator, axis: Axis) {
+  return scroller.evaluate((element: HTMLElement, along: Axis) => {
+    const sideways = along === 'horizontal'
+    const { x, y, width, height } = element.getBoundingClientRect()
+    return {
+      x,
+      y,
+      width,
+      height,
+      thickness: sideways
+        ? element.offsetHeight - element.clientHeight
+        : element.offsetWidth - element.clientWidth,
+      client: sideways ? element.clientWidth : element.clientHeight,
+      scroll: sideways ? element.scrollWidth : element.scrollHeight,
+      offset: sideways ? element.scrollLeft : element.scrollTop,
+    }
+  }, axis)
+}

frontend/e2e/tests/app/table/scrollbar-drag.spec.ts

@@ -0,0 +1,66 @@
+import { type Page } from '@playwright/test'
+
+import { test, expect } from '#fixtures'
+import {
+  canScroll,
+  gridScroller,
+  scrollbarThumb,
+  type Point,
+} from '#support/grid'
+import { openProposal } from '#support/table'
+
+// Small enough that the example's columns overflow sideways and its rows
+// overflow downwards, so the grid has both scrollbars and somewhere to scroll.
+test.use({
+  viewport: { width: 900, height: 400 },
+  // Headless Chromium hides scrollbars by default, leaving only overlay ones,
+  // which take no layout space and so cannot be pressed.
+  launchOptions: { ignoreDefaultArgs: ['--hide-scrollbars'] },
+})
+
+test.beforeEach(async ({ page, example }) => {
+  await openProposal(page, example)
+
+  // Dragging one scrollbar only proves something about the other axis if the
+  // grid could have scrolled that way in the first place.
+  await expect.poll(() => canScroll(page, 'horizontal')).toBe(true)
+  await expect.poll(() => canScroll(page, 'vertical')).toBe(true)
+})
+
+test('dragging along the horizontal scrollbar leaves the vertical scroll alone', async ({
+  page,
+}) => {
+  const scroller = gridScroller(page)
+
+  await dragThumb(page, {
+    from: await scrollbarThumb(page, 'horizontal'),
+    by: { x: 150, y: 0 },
+  })
+
+  await expect(scroller).not.toHaveJSProperty('scrollLeft', 0)
+  await expect(scroller).toHaveJSProperty('scrollTop', 0)
+})
+
+test('dragging along the vertical scrollbar leaves the horizontal scroll alone', async ({
+  page,
+}) => {
+  const scroller = gridScroller(page)
+
+  await dragThumb(page, {
+    from: await scrollbarThumb(page, 'vertical'),
+    by: { x: 0, y: 60 },
+  })
+
+  await expect(scroller).not.toHaveJSProperty('scrollTop', 0)
+  await expect(scroller).toHaveJSProperty('scrollLeft', 0)
+})
+
+// Glide autoscrolls a drag that strays outside the grid, and speeds up the
+// longer the button is held. Hold before releasing so a regression shows.
+async function dragThumb(page: Page, { from, by }: { from: Point; by: Point }) {
+  await page.mouse.move(from.x, from.y)
+  await page.mouse.down()
+  await page.mouse.move(from.x + by.x, from.y + by.y, { steps: 10 })
+  await page.waitForTimeout(300)
+  await page.mouse.up()
+}

frontend/e2e/tsconfig.json

@@ -2,6 +2,9 @@
   "extends": "@damnit-frontend/config/tsconfig/node",
   "compilerOptions": {
     "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.tsbuildinfo",
+    // Specs run in node, but page.evaluate bodies run in the browser and need
+    // to name DOM types.
+    "lib": ["ES2023", "DOM"],
     "types": ["node", "@playwright/test"]
   },
   "include": [

frontend/patches/@glideapps__glide-data-grid@6.0.3.patch

@@ -0,0 +1,60 @@
+diff --git a/dist/cjs/internal/data-grid/data-grid.js b/dist/cjs/internal/data-grid/data-grid.js
+index 412d81c8748b979a9d988f2e3cfa8eddc1bfc555..1d7ebc9ce3dbfd793fc009b318f3e1e6c663d1a5 100644
+--- a/dist/cjs/internal/data-grid/data-grid.js
++++ b/dist/cjs/internal/data-grid/data-grid.js
+@@ -562,6 +562,13 @@ const DataGrid = (p, forwardedRef) => {
+         const eventTarget = eventTargetRef?.current;
+         if (canvas === null || (ev.target !== canvas && ev.target !== eventTarget))
+             return;
++        // A scrollbar press targets the scroller from outside its client box,
++        // and autoscrolls the grid if let through. Overlay bars slip past.
++        if (ev instanceof MouseEvent && eventTarget !== null &&
++            ev.target === eventTarget &&
++            (ev.offsetX > eventTarget.clientWidth ||
++                ev.offsetY > eventTarget.clientHeight))
++            return;
+         mouseDown.current = true;
+         let clientX;
+         let clientY;
+@@ -573,11 +580,6 @@ const DataGrid = (p, forwardedRef) => {
+             clientX = ev.touches[0].clientX;
+             clientY = ev.touches[0].clientY;
+         }
+-        if (ev.target === eventTarget && eventTarget !== null) {
+-            const bounds = eventTarget.getBoundingClientRect();
+-            if (clientX > bounds.right || clientY > bounds.bottom)
+-                return;
+-        }
+         const args = getMouseArgsForPosition(canvas, clientX, clientY, ev);
+         downPosition.current = args.location;
+         if (args.isTouch) {
+diff --git a/dist/esm/internal/data-grid/data-grid.js b/dist/esm/internal/data-grid/data-grid.js
+index c32e94b805eeb5291d0c3aa3b44b8dc43e9f99b4..11f7dfeeb7bfbe314a82a9afb9229ae3949456f4 100644
+--- a/dist/esm/internal/data-grid/data-grid.js
++++ b/dist/esm/internal/data-grid/data-grid.js
+@@ -535,6 +535,13 @@ const DataGrid = (p, forwardedRef) => {
+         const eventTarget = eventTargetRef?.current;
+         if (canvas === null || (ev.target !== canvas && ev.target !== eventTarget))
+             return;
++        // A scrollbar press targets the scroller from outside its client box,
++        // and autoscrolls the grid if let through. Overlay bars slip past.
++        if (ev instanceof MouseEvent && eventTarget !== null &&
++            ev.target === eventTarget &&
++            (ev.offsetX > eventTarget.clientWidth ||
++                ev.offsetY > eventTarget.clientHeight))
++            return;
+         mouseDown.current = true;
+         let clientX;
+         let clientY;
+@@ -546,11 +553,6 @@ const DataGrid = (p, forwardedRef) => {
+             clientX = ev.touches[0].clientX;
+             clientY = ev.touches[0].clientY;
+         }
+-        if (ev.target === eventTarget && eventTarget !== null) {
+-            const bounds = eventTarget.getBoundingClientRect();
+-            if (clientX > bounds.right || clientY > bounds.bottom)
+-                return;
+-        }
+         const args = getMouseArgsForPosition(canvas, clientX, clientY, ev);
+         downPosition.current = args.location;
+         if (args.isTouch) {

frontend/patches/README.md

@@ -0,0 +1,30 @@
+# Patches
+
+Local fixes to third-party packages, applied by pnpm through the
+`patchedDependencies` map in `pnpm-workspace.yaml`. A version bump makes
+`pnpm install` fail until the patch is regenerated:
+
+```sh
+pnpm patch <package>@<version>   # edit the printed directory, then:
+pnpm patch-commit <printed-path>
+```
+
+The package's `exports` map resolves to `dist/`, so `src/` edits are inert.
+Patch both `dist/cjs` and `dist/esm`.
+
+## `@glideapps/glide-data-grid`
+
+Pressing a scrollbar starts a cell drag, which autoscrolls the grid along the
+other axis. The grid's mousedown guard rejects presses past the scroller's
+border box, but scrollbars sit inside it, between the client box and the border
+box, so a press on one is never rejected. The patch measures the client box.
+
+Still present on upstream `main`, and reported there as
+[#1034](https://github.com/glideapps/glide-data-grid/issues/1034), which reaches
+the same cause from the other end: the press lands on whatever sits under the
+scrollbar. Drop the patch once that is fixed.
+
+The patch only recognises scrollbars that take layout space. Overlay scrollbars
+sit over the client box, so a press on one looks exactly like a press on a cell
+and still gets through. Covered by
+`e2e/tests/app/table/scrollbar-drag.spec.ts`.

frontend/pnpm-workspace.yaml

@@ -65,3 +65,6 @@ catalogs:
     '@types/react': 18.2.45
     '@types/react-dom': 18.2.18
     '@types/react-plotly.js': 2.6.3
+
+patchedDependencies:
+  '@glideapps/glide-data-grid@6.0.3': patches/@glideapps__glide-data-grid@6.0.3.patch