fix(web): a selection driven below the fold scrolls itself into view
improvise-jq8l. `web/app.js` called `scrollIntoView` NOWHERE, so a selection moved past the bottom of `#grid` stayed there, invisible, with no way back but scrolling by hand. What the ticket did not predict: this is NOT a projection bug and NOT a `row_offset` bug, and the acceptance criteria's talk of reconciling the absolute selection against the announced window describes work `render.rs` already does. The row had been sent. `#grid` has `overflow: auto`, `ROW_PX`/`COL_PX` UNDER-estimate (a real row measures 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. `block: "nearest"` is the whole option: it scrolls only when the element is out of view, and it scrolls the nearest container rather than the document. The expensive half was the fixture. Neither shipped document can reach the state — `web/demo.improv` is four rows, `harness/fixtures/grouped.improv` is eight, and a probe measured BOTH at `scrollHeight === clientHeight` in a 484px box. The assertion would have passed against a page that never scrolls, which is why the spec asserts the overflow exists before it presses a key. `harness/fixtures/tall.improv` is thirty rows in three groups; measured against the unfixed page it gives 105px of overflow with the selected cell 96px below the visible bottom and `scrollTop` still 0. Tests: 25 chromium harness (was 24), Rust and node unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -1245,6 +1245,30 @@ Neither arm leaves one armed.
|
|||||||
harness one, because the standalone deployment persists on every accepted
|
harness one, because the standalone deployment persists on every accepted
|
||||||
batch and clears the flag in the same round trip, so `[+]` is transient there.
|
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`
|
### The pointer takes cells on `mousedown`
|
||||||
|
|
||||||
A cell's handler is `mousedown` with `preventDefault()`, not `click`, and the
|
A cell's handler is `mousedown` with `preventDefault()`, not `click`, and the
|
||||||
|
|||||||
@@ -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
|
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
|
order `name (stored_as) [+]`, the `null` `stored_as` writing no empty parens — is
|
||||||
`web/tests/shell.test.js`, over `documentTitle` directly.
|
`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.
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -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);
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -182,6 +182,13 @@ function repaint() {
|
|||||||
client.click_formula(i),
|
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.
|
// What the frame costs, so the NEXT viewport announcement is honest.
|
||||||
// Inline group ROWS are deliberately NOT counted — context/code/clients.md.
|
// Inline group ROWS are deliberately NOT counted — context/code/clients.md.
|
||||||
const next = frameCostOf(model);
|
const next = frameCostOf(model);
|
||||||
|
|||||||
Reference in New Issue
Block a user