diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 15ff6714..ebe02a28 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4749,3 +4749,58 @@ claims at commit. The commit was then *accepted*. With the refusal assertion disabled, the host held `Some(Number(42.0))`, so A's commit had re-created the cell B cleared. Under that rival, the 0j0f test above failed as well. Both files were restored. + +### a_locally_resolved_page_scroll_lands_where_the_host_would_put_it + +**What was missing (improvise-5xbq).** `tests/lockstep.rs` compared only `j` +and `G` against the host. The browser's page rule (`resolve::page_rows`) and +the TUI's (`Move`'s `Page` arm, `visible_rows * 3 / 4`) were two copies, and +two faults injected into the browser's copy left the whole workspace green: +`page_rows` returning `window_rows / 2`, and `grid_move` paging by +`window_cols`. The second was invisible for a further reason: the only window +the page-rule test used was square, `(8, 8)`. + +**The fixture.** 20 rows by 3 columns; the host sees 8 rows and the client +announces `(8, 3)`. The three candidate rules then give three different +steps: 6 (three quarters of the rows), 4 (half the rows) and 2 (three quarters +of the columns). The walk starts off the edge (two `j`), then presses `PgDn`, +`PgUp`, `Ctrl-d` and `Ctrl-u`, including a `PgDn` that clamps at row 19. + +**Why it pins the rows as well as comparing the ends.** `page_rows` is now the +one home (the TUI's `Move` calls it), so a wrong rule moves host and client +together, and a comparison alone passes. Each press therefore also asserts the +row the TUI's rule names. + +**Injected, before and after the move to one home.** + +| Fault | Two copies | One home | +|---|---|---| +| `page_rows` returns `window_rows / 2` | fails at press 2 (`PgDn`), "client and host disagree" | fails at press 2, "not the row the TUI's page rule names"; `app.rs`'s `page_down_scrolls_by_three_quarters_visible` and `page_up_scrolls_backward` fail too | +| `grid_move` pages by `window_cols` | fails at press 2, "client and host disagree" | fails at press 2, "client and host disagree" | + +`Ctrl-d`/`Ctrl-u` are `scroll-rows ±5`, not the page rule. They are in the walk +because the ticket named them, and they move the start point of the next page +turn to a row the fixed rules alone would not reach. + +### a narrow window still shows how to page through help — `harness/tests/help.spec.js` + +**What broke (improvise-1c5m).** Below the width at which the app bar wraps, +`#help` (fixed, `inset: 2rem`, `z-index: 30`) covers `#hint`, and since PR #180 +Welcome carries no paging instruction of its own. So a new user on Welcome at +phone width could not see how to reach page two. + +**The fix is option (b) from the ticket.** The overlay draws `model.hint` in a +footer. In Help mode that is already `hint_line(Help)`, the same string `#hint` +carries, so it adds no derivation, and it sits outside `.help-body`, so +"Welcome is prose" still holds. The footer is `position: sticky` because the +overlay itself scrolls and Welcome is longer than a phone-height overlay. + +**What the case asserts.** At 600x800 on Welcome, it reads the hint string from +`#hint`, so the spec spells no keys. It then requires some element showing +exactly that string to be the topmost element at its own centre, which is the +`elementFromPoint` measurement the ticket made by hand. + +**Red, then a rival.** At base it failed with "the paging hint is under the +help overlay". With the fix and `position: sticky` changed to `static`, it +fails the same way: the footer exists but sits below the fold of a scrolling +overlay, so presence alone cannot satisfy the case. diff --git a/crates/improvise-command/src/command/cmd/navigation.rs b/crates/improvise-command/src/command/cmd/navigation.rs index 5f13f2cb..14523b5c 100644 --- a/crates/improvise-command/src/command/cmd/navigation.rs +++ b/crates/improvise-command/src/command/cmd/navigation.rs @@ -118,7 +118,7 @@ impl Cmd for Move { } } MoveKind::Page(dir) => { - let delta = (self.cursor.visible_rows as i32 * 3 / 4).max(1) * dir; + let delta = improvise_protocol::resolve::page_rows(self.cursor.visible_rows) * dir; let nr = (self.cursor.row as i32 + delta).clamp(0, row_max) as usize; (nr, self.cursor.col) } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index d88b4e23..fa22ee24 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -620,7 +620,8 @@ enum GridMove { } /// Rows one `page-scroll` step moves: three quarters of the window, at least one. -fn page_rows(window_rows: usize) -> i32 { +/// The one home: the TUI's `Move` calls it too (improvise-5xbq). +pub fn page_rows(window_rows: usize) -> i32 { (window_rows as i32 * 3 / 4).max(1) } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 294b3fc0..07e51ceb 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -266,6 +266,70 @@ fn a_locally_resolved_cursor_move_lands_where_the_host_would_put_it() { ); } +/// Twenty rows by three columns, the host's window eight rows by three: not +/// square, so a rule keyed off the wrong axis lands somewhere else. +fn paging_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for i in 0..20 { + wb.add_item("Region", format!("R{i:02}")).unwrap(); + } + for m in ["A", "B", "C"] { + wb.add_item("_Measure", m).unwrap(); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + let mut app = App::new(wb, None); + app.term_height = 8 + 8; + app.rebuild_layout(); + app +} + +/// Page and half-page scrolls land in the same place on both ends, and in the +/// place the TUI's rule names. Why the fixture and the pinned rows: the evidence file. +#[test] +fn a_locally_resolved_page_scroll_lands_where_the_host_would_put_it() { + let window = (8, 3); + let mut app = paging_host(); + let mut client = client_of(&app, window); + let key = |code, mods| KeyEvent::new(code, mods); + let none = KeyModifiers::NONE; + let ctrl = KeyModifiers::CONTROL; + let presses = [ + (key(KeyCode::Char('j'), none), 1), + (key(KeyCode::Char('j'), none), 2), + (key(KeyCode::PageDown, none), 8), + (key(KeyCode::PageDown, none), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::Char('d'), ctrl), 13), + (key(KeyCode::PageDown, none), 19), + (key(KeyCode::Char('u'), ctrl), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::PageUp, none), 2), + ]; + for (press, (event, want_row)) in presses.into_iter().enumerate() { + match resolve(&client, event) { + KeyOutcome::Local(cmds) => { + for cmd in &cmds { + client.fold(cmd); + } + } + other => panic!("press {press}: the client must answer {event:?}, got {other:?}"), + } + reduce_key(&mut app, event); + assert_eq!( + client.view.cursor(), + app.view_state.cursor(), + "press {press} ({event:?}): client and host disagree" + ); + assert_eq!( + client.view.cursor().selected, + (want_row, 0), + "press {press} ({event:?}): not the row the TUI's page rule names" + ); + } +} + /// The same for `G`, the key `GridExtent` exists for: it is *defined* as "the /// last row", so a window-only client would have nothing to jump to. #[test] diff --git a/harness/tests/help.spec.js b/harness/tests/help.spec.js index 54b9c961..6a2893e6 100644 --- a/harness/tests/help.spec.js +++ b/harness/tests/help.spec.js @@ -132,4 +132,30 @@ test.describe("the help overlay draws the shared content", () => { await expect(rows_.last().locator("td.selected")).toHaveCount(1); await expect(page.locator("#grid tbody td.selected")).toHaveCount(1); }); + + // improvise-1c5m. Below the width at which the app bar wraps, `#help` covers + // `#hint`, and Welcome itself says nothing about paging. The contract: the + // mode's own hint is UNCOVERED somewhere while help is open. + test("a narrow window still shows how to page through help", async ({ page }) => { + await page.setViewportSize({ width: 600, height: 800 }); + await openHelp(page); + + // The string is the page's own `#hint`, i.e. `hint_line(Help)`; this spec + // spells no keys. Asserting it names paging keeps an empty hint out. + const hint = (await page.locator("#hint").textContent()).trim(); + expect(hint).toContain("next-page"); + + // An element showing exactly that string, topmost at its own centre: the + // overlay covering `#hint` is precisely what `elementFromPoint` sees. + const uncovered = await page.evaluate((text) => + [...document.querySelectorAll("body *")].some((node) => { + if (node.children.length > 0 || node.textContent.trim() !== text) return false; + const r = node.getBoundingClientRect(); + if (r.width === 0 || r.height === 0) return false; + const top = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2); + return top !== null && (top === node || node.contains(top)); + }), + hint); + expect(uncovered, "the paging hint is under the help overlay").toBe(true); + }); }); diff --git a/web/app.js b/web/app.js index 22e94a34..31289ce1 100644 --- a/web/app.js +++ b/web/app.js @@ -348,7 +348,13 @@ function paintHelp(model) { } } - el.help.replaceChildren(tabs, body); + // The mode's own hint again, as the TUI popup's footer: once the app bar + // wraps, the overlay covers `#hint` (improvise-1c5m). + const footer = document.createElement("p"); + footer.className = "help-footer"; + footer.textContent = model.hint; + + el.help.replaceChildren(tabs, body, footer); } /** The page bar: which item each fixed category is fixed to. Without it a diff --git a/web/style.css b/web/style.css index 88ed64cc..6d02e7d1 100644 --- a/web/style.css +++ b/web/style.css @@ -736,6 +736,17 @@ td.total { font-weight: 700; } +/* Sticky, so the paging keys stay in view however long the page scrolls. */ +.help-footer { + position: sticky; + bottom: -1.25rem; + margin: 0.75rem 0 0; + padding: 0.5rem 0; + border-top: 1px solid var(--line); + background: var(--bg); + color: var(--dim); +} + .help-body { display: grid; grid-template-columns: 16rem 1fr;