Merge pull request #215 from fiddlerwoaroof/fix/5xbq-1c5m

One page rule for both ends; the help overlay shows how to page (5xbq, 1c5m)

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.

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)
             }

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)
 }
 

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]

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);
+  });
 });

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

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;