diff --git a/context/code/clients.md b/context/code/clients.md index 1c606de6..5a4575b5 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1245,6 +1245,30 @@ Neither arm leaves one armed. harness one, because the standalone deployment persists on every accepted batch and clears the flag in the same round trip, so `[+]` is transient there. +### The selection scrolls itself into view + +`web/app.js` called `scrollIntoView` nowhere at all, so a selection driven past +the bottom of `#grid` stayed there, invisible, with no way back but scrolling by +hand (improvise-jq8l). Two things about it are easy to get wrong. + +**It is not a projection bug and not a `row_offset` bug.** The row had been sent: +`#grid` has `overflow: auto` and the page announces an extent from `ROW_PX`/ +`COL_PX`, which **under-estimate** — a real row is 28.6px against the assumed 26 +— and inline group rows are deliberately not charged for at all. So the page +renders more rows than its own box can show, and the overflow is the DOM's, not +the window's. Measured on `harness/fixtures/tall.improv`: 105px of overflow, the +selected cell 96px below the visible bottom, `scrollTop` still 0. + +**`block: "nearest"` is the whole option.** It scrolls only when the element is +actually out of view, and it scrolls the nearest scroll container rather than the +document, which is what keeps the page itself from moving sideways. + +The reason this was untestable until now is the fixture: `web/demo.improv` is +four rows and `harness/fixtures/grouped.improv` is eight, and neither overflows a +484px box. `tall.improv` is thirty rows in three groups, and the spec asserts the +overflow exists **before** asserting anything about the scroll — without that +line the whole test passes on a page that never scrolls. + ### The pointer takes cells on `mousedown` A cell's handler is `mousedown` with `preventDefault()`, not `click`, and the diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 9f32691b..3702765c 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -2100,3 +2100,24 @@ round trip (gotcha 45), so the marker is transient in a browser and any timing assertion over it would be flaky rather than discriminating. Its spelling — the order `name (stored_as) [+]`, the `null` `stored_as` writing no empty parens — is `web/tests/shell.test.js`, over `documentTitle` directly. + +#### "a selection driven below the fold scrolls itself into view" + +improvise-jq8l, and the expensive half was the **fixture**, not the assertion. +Neither shipped document can reach the state: `web/demo.improv` is four rows and +`harness/fixtures/grouped.improv` is eight, and a probe measured both at +`scrollHeight === clientHeight` in a 484px box — nothing to scroll, so the +assertion would have passed against a page that never scrolls. That is the +vacuity this entry exists to record, and it is why the spec asserts +`scrollHeight - clientHeight > 0` **before** it presses a key. + +`harness/fixtures/tall.improv` is thirty rows in three groups, which overflows for +a reason worth stating: `ROW_PX` is 26 and a real row measures 28.6px, and inline +group rows are not charged for at all, so the page renders more rows than its own +box can show. Measured against the unfixed page: 105px of overflow, the selected +cell 96px below the visible bottom, `scrollTop` still 0 after twenty-five +`ArrowDown`s. + +Both directions are asserted, and neither alone is enough: the cell being inside +`#grid`'s rect would also hold if the selection had never moved, and `scrollTop > +0` would also hold if the page scrolled to the wrong place. diff --git a/harness/fixtures/tall.improv b/harness/fixtures/tall.improv new file mode 100644 index 00000000..5daa32d2 --- /dev/null +++ b/harness/fixtures/tall.improv @@ -0,0 +1,114 @@ +v2025-04-09 +# Harness tall fixture +Initial View: Tall + +## View: Default +_Index: none +_Dim: none +_Measure: page +Region: row +format: ,.0 + +## View: Tall +_Index: none +_Dim: none +_Measure: column +Region: row +format: ,.0 + +## Category: _Measure +- Revenue, Cost + +## Category: Region +- R01[Alpha] +- R02[Alpha] +- R03[Alpha] +- R04[Alpha] +- R05[Alpha] +- R06[Alpha] +- R07[Alpha] +- R08[Alpha] +- R09[Alpha] +- R10[Alpha] +- R11[Beta] +- R12[Beta] +- R13[Beta] +- R14[Beta] +- R15[Beta] +- R16[Beta] +- R17[Beta] +- R18[Beta] +- R19[Beta] +- R20[Beta] +- R21[Gamma] +- R22[Gamma] +- R23[Gamma] +- R24[Gamma] +- R25[Gamma] +- R26[Gamma] +- R27[Gamma] +- R28[Gamma] +- R29[Gamma] +- R30[Gamma] + +## Data +Region=R01, _Measure=Cost = 50 +Region=R01, _Measure=Revenue = 100 +Region=R02, _Measure=Cost = 51 +Region=R02, _Measure=Revenue = 101 +Region=R03, _Measure=Cost = 52 +Region=R03, _Measure=Revenue = 102 +Region=R04, _Measure=Cost = 53 +Region=R04, _Measure=Revenue = 103 +Region=R05, _Measure=Cost = 54 +Region=R05, _Measure=Revenue = 104 +Region=R06, _Measure=Cost = 55 +Region=R06, _Measure=Revenue = 105 +Region=R07, _Measure=Cost = 56 +Region=R07, _Measure=Revenue = 106 +Region=R08, _Measure=Cost = 57 +Region=R08, _Measure=Revenue = 107 +Region=R09, _Measure=Cost = 58 +Region=R09, _Measure=Revenue = 108 +Region=R10, _Measure=Cost = 59 +Region=R10, _Measure=Revenue = 109 +Region=R11, _Measure=Cost = 60 +Region=R11, _Measure=Revenue = 110 +Region=R12, _Measure=Cost = 61 +Region=R12, _Measure=Revenue = 111 +Region=R13, _Measure=Cost = 62 +Region=R13, _Measure=Revenue = 112 +Region=R14, _Measure=Cost = 63 +Region=R14, _Measure=Revenue = 113 +Region=R15, _Measure=Cost = 64 +Region=R15, _Measure=Revenue = 114 +Region=R16, _Measure=Cost = 65 +Region=R16, _Measure=Revenue = 115 +Region=R17, _Measure=Cost = 66 +Region=R17, _Measure=Revenue = 116 +Region=R18, _Measure=Cost = 67 +Region=R18, _Measure=Revenue = 117 +Region=R19, _Measure=Cost = 68 +Region=R19, _Measure=Revenue = 118 +Region=R20, _Measure=Cost = 69 +Region=R20, _Measure=Revenue = 119 +Region=R21, _Measure=Cost = 70 +Region=R21, _Measure=Revenue = 120 +Region=R22, _Measure=Cost = 71 +Region=R22, _Measure=Revenue = 121 +Region=R23, _Measure=Cost = 72 +Region=R23, _Measure=Revenue = 122 +Region=R24, _Measure=Cost = 73 +Region=R24, _Measure=Revenue = 123 +Region=R25, _Measure=Cost = 74 +Region=R25, _Measure=Revenue = 124 +Region=R26, _Measure=Cost = 75 +Region=R26, _Measure=Revenue = 125 +Region=R27, _Measure=Cost = 76 +Region=R27, _Measure=Revenue = 126 +Region=R28, _Measure=Cost = 77 +Region=R28, _Measure=Revenue = 127 +Region=R29, _Measure=Cost = 78 +Region=R29, _Measure=Revenue = 128 +Region=R30, _Measure=Cost = 79 +Region=R30, _Measure=Revenue = 129 diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 2c3394cc..e7ceee98 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -243,3 +243,47 @@ test.describe("a pointer event reaches the client and its answer is painted", () await expect(page.locator("#grid tbody td.selected")).toHaveCount(1); }); }); + +test.describe("the selection is on screen", () => { + // improvise-jq8l: `web/app.js` called `scrollIntoView` NOWHERE, so a selection + // moved past the bottom of `#grid` simply stayed there, invisible, with no way + // back but scrolling by hand. Only a browser can see this: it is a fact about + // `#grid`'s own scroll box, not about the projection, which had already sent + // the row — the page renders more rows than fit, for the reason below. + test("a selection driven below the fold scrolls itself into view", async ({ page }) => { + // `harness/fixtures/tall.improv`: 30 rows in three groups, which is more + // than the box can show because `ROW_PX` (26) under-estimates the real + // 28.6px row and inline group rows are deliberately not charged for. + await page.goto("/app/index.html?doc=tall-scroll.improv&seed=tall.improv"); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await expect(page.locator("#view-name")).toHaveText("Tall"); + await expect(page.locator("#grid tbody th[scope=row]").first()).toHaveText("R01"); + await page.locator("#grid").focus(); + + // Not vacuous by construction: if `#grid` does not overflow there is + // nothing to scroll and every assertion below passes on a broken page. + const overflow = await page + .locator("#grid") + .evaluate((g) => g.scrollHeight - g.clientHeight); + expect(overflow).toBeGreaterThan(0); + + for (let i = 0; i < 25; i++) await page.keyboard.press("ArrowDown"); + + // The selected cell must be inside the box the user can see. Measured + // against `#grid`'s own rect rather than the window's, because `#grid` is + // the scroll container — the page itself does not scroll. + await expect + .poll(async () => + page.locator("#grid").evaluate((g) => { + const sel = g.querySelector("td.selected"); + if (!sel) return "no selected cell"; + const cell = sel.getBoundingClientRect(); + const box = g.getBoundingClientRect(); + return cell.bottom <= box.bottom + 1 && cell.top >= box.top - 1; + }), + ) + .toBe(true); + // And it really did have to move: a page that never scrolls leaves this 0. + expect(await page.locator("#grid").evaluate((g) => g.scrollTop)).toBeGreaterThan(0); + }); +}); diff --git a/web/app.js b/web/app.js index b435d56d..41ded498 100644 --- a/web/app.js +++ b/web/app.js @@ -182,6 +182,13 @@ function repaint() { client.click_formula(i), ); + // The selection must be on screen. `#grid` scrolls on its own — ROW_PX and + // COL_PX under-estimate and group rows are uncharged — so a selection the + // projection did send can still sit below the fold (improvise-jq8l). + el.grid + .querySelector("td.selected") + ?.scrollIntoView({ block: "nearest", inline: "nearest" }); + // What the frame costs, so the NEXT viewport announcement is honest. // Inline group ROWS are deliberately NOT counted — context/code/clients.md. const next = frameCostOf(model);