amadan

oficina / sheets Public

Stop the grid library's other local-only edits

Review of B7 (discussion #33) found the library still changes its own model
behind the app's back without the menu:

- A row or column header plus Delete/Backspace ran confirm() and then
  deleteRow()/deleteColumn() locally, with no POST. Every later edit then
  posted to shifted coordinates: silent wrong-cell writes.
- Ctrl/Cmd+S ran the library's CSV download(), bypassing export.
- rowDrag is on by default, so dragging a row header's edge ran moveRow()
  locally.

Worksheet: allowDeleteRow, allowDeleteColumn and rowDrag are now false.
Spreadsheet: allowExport is now false. The option names were checked
against the vendored jspreadsheet-ce 5.0.2: keyDownControls gates on
allowDeleteRow/allowDeleteColumn, download() on config.allowExport, and
the tbody draggable class and mouseDownControls on rowDrag.

Sweep of the library's other local mutators. insertRow/insertColumn from
Enter/Tab at the edge are already off (allowInsertRow/Column), moveColumn
is off (columnDrag), and orderBy via header double-click is off
(columnSorting). setMerge, header rename and comments are reachable only
from the context menu, which is now off. Cell Delete, fill and paste go
through onchange or clipboard.js. Row and column resizing go through
sizes.js.

tools/drive/contextmenu.mjs now covers right-click, row and column header
plus Delete (and Backspace in Chromium), Ctrl+S and Meta+S, and dragging a
row header. For each it asserts that the grid's shape and values are
unchanged, with no confirm, no download, no write and no page error. All 14
checks fail with main's grid.js and pass here, in Chromium and WebKit.
grid.js stays at the 300-line cap.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev ae0e1ada7f0e7390fcc54e8b9e8f3436fdb734b1 parent af36460
2 files changed, +92 −37
  • internal/sheets/static/grid.js +9 −9
  • tools/drive/contextmenu.mjs +83 −28
diff --git a/internal/sheets/static/grid.js b/internal/sheets/static/grid.js
index 47aa92c..fda8e51 100644
--- a/internal/sheets/static/grid.js
+++ b/internal/sheets/static/grid.js
@@ -174,10 +174,9 @@
version = payload.version || 0;
var opts = {
- /* Our engine computes; the client paints. See the file comment. */
- parseFormulas: false,
- /* The library's right-click items bypass our write paths. false: browser menu. */
- contextMenu: function () { return false; },
+ parseFormulas: false, /* Our engine computes; the client paints. See the file comment. */
+ /* Library menu and Ctrl+S CSV bypass our write/export paths. false: browser menu. */
+ contextMenu: function () { return false; }, allowExport: false,
/* Every handler below is a SPREADSHEET-level option, not a
* worksheet one. Nested inside worksheets[0] they are accepted
@@ -190,10 +189,8 @@
write(parseInt(r, 10), parseInt(c, 10), value === null ? '' : String(value));
},
onselection: function (instance, x1, y1, x2, y2) {
- sel = {
- r: parseInt(y1, 10), c: parseInt(x1, 10),
- r2: parseInt(y2, 10), c2: parseInt(x2, 10)
- };
+ sel = { r: parseInt(y1, 10), c: parseInt(x1, 10),
+ r2: parseInt(y2, 10), c2: parseInt(x2, 10) };
announce();
},
/* The in-cell editor should show the formula, not its result.
@@ -219,7 +216,10 @@
minDimensions: [cols, rows],
tableOverflow: true,
columnSorting: false, /* sorting is a later slice; a half-done sort is worse than none */
- columnDrag: false, allowInsertRow: false, allowInsertColumn: false
+ /* Each of these changes the library's model only, never the server's, and then every
+ * later write lands at shifted coordinates (header + Delete, row drag). */
+ columnDrag: false, rowDrag: false, allowInsertRow: false, allowInsertColumn: false,
+ allowDeleteRow: false, allowDeleteColumn: false
}]
};
/* The resize callbacks are SPREADSHEET-level options, like the
diff --git a/tools/drive/contextmenu.mjs b/tools/drive/contextmenu.mjs
index 38eaee9..25b19bd 100644
--- a/tools/drive/contextmenu.mjs
+++ b/tools/drive/contextmenu.mjs
@@ -1,8 +1,13 @@
-// Right-clicking a cell must not open the grid library's stock context menu
-// (B7). Its items ("Delete selected rows", "Add comments", "Copy...",
-// "Paste...", "Save as...", "About") act on the library's own model and
-// bypass the app's write, comment and export paths. The app configures
-// contextMenu to return false, so the browser's own menu shows instead.
+// The grid library's own actions must not change the grid behind the app's
+// back (B7 and its review). Each of these acts on the library's model only,
+// never the server's, and after a local row delete or row move every later
+// write lands at shifted coordinates:
+// - right-click: the stock menu ("Delete selected rows", "Add comments",
+// "Save as...", "About"). contextMenu returns false: browser menu instead.
+// - row header + Delete, column header + Delete: confirm() then deleteRow /
+// deleteColumn locally (allowDeleteRow / allowDeleteColumn: false).
+// - Ctrl/Cmd+S: the library's CSV download(), bypassing export (allowExport).
+// - dragging a row header's edge: moveRow locally (rowDrag: false).
// Real server-rendered page and assets, mocked transport; no live data.
// SHEETS_MENU_FIXTURE=/tmp/menus.json go test ./internal/sheets -run TestBrowserMenuFixture -count=1
// PLAYWRIGHT=/path/to/playwright/index.mjs SHEETS_MENU_FIXTURE=/tmp/menus.json node tools/drive/contextmenu.mjs
@@ -11,13 +16,18 @@ import fs from 'node:fs';
const pw = await import(process.env.PLAYWRIGHT || 'playwright');
const fixture = JSON.parse(fs.readFileSync(process.env.SHEETS_MENU_FIXTURE));
const origin = 'https://sheets.test';
+const failures = [];
for (const name of ['chromium', 'webkit']) {
const options = name === 'webkit' && process.env.WEBKIT_EXECUTABLE_PATH ? {executablePath:process.env.WEBKIT_EXECUTABLE_PATH} : {};
const browser = await pw[name].launch(options);
try {
- const page = await browser.newPage({viewport:{width:1440,height:1000}});
- const errors = []; let writes = [];
+ const context = await browser.newContext({viewport:{width:1440,height:1000}, acceptDownloads:true});
+ const page = await context.newPage();
+ let errors = [], writes = [], dialogs = [], downloads = [];
page.on('pageerror', e=>errors.push(e.message));
+ // Accept every confirm(): dismissing would hide the delete it guards.
+ page.on('dialog', d=>{ dialogs.push(d.message()); d.accept(); });
+ page.on('download', d=>downloads.push(d.suggestedFilename()));
await page.route(origin+'/**', async route => {
const req = route.request(), path = new URL(req.url()).pathname;
if (path in fixture.assets) return route.fulfill({body:fixture.assets[path],contentType:
@@ -25,33 +35,78 @@ for (const name of ['chromium', 'webkit']) {
if (path.endsWith('/stream')) return route.fulfill({body:'',contentType:'text/event-stream'});
if (req.method() !== 'GET') writes.push(path);
if (path.startsWith('/api/')) return route.fulfill({json:{version:1,cells:[
- {r:0,c:0,raw:'10',display:'10'},{r:1,c:0,raw:'20',display:'20'}]}});
+ {r:0,c:0,raw:'10',display:'10'},{r:1,c:0,raw:'20',display:'20'},{r:2,c:0,raw:'30',display:'30'},
+ {r:0,c:1,raw:'b1',display:'b1'}]}});
return route.fulfill({body:fixture.html,contentType:'text/html'});
});
- await page.goto(origin+'/sheets/1');
- await page.waitForFunction(()=>window.SheetsGrid?.worksheet());
+ const shape = () => page.evaluate(() => {
+ const t = document.querySelector('[data-sheet-grid] .jss_worksheet');
+ const at = (x, y) => t.querySelector(`tbody td[data-x="${x}"][data-y="${y}"]`)?.textContent ?? null;
+ return {rows: t.querySelectorAll(':scope > tbody > tr').length,
+ cols: t.querySelectorAll(':scope > thead > tr:last-child > td[data-x]').length,
+ a: [at(0,0), at(0,1), at(0,2)], b1: at(1,0)};
+ });
+ async function check(label, act) {
+ await page.goto(origin+'/sheets/1');
+ await page.waitForFunction(()=>window.SheetsGrid?.worksheet());
+ await page.locator('td[data-x="0"][data-y="0"]').click();
+ const before = await shape();
+ errors = []; writes = []; dialogs = []; downloads = [];
+ try {
+ await act();
+ await page.waitForTimeout(300);
+ const after = await shape();
+ assert.deepEqual(after, before, `${name} ${label}: the grid changed locally`);
+ assert.deepEqual(dialogs, [], `${name} ${label}: the library asked to confirm`);
+ assert.deepEqual(downloads, [], `${name} ${label}: the library downloaded a file`);
+ assert.deepEqual(writes, [], `${name} ${label}: sent writes`);
+ assert.deepEqual(errors, [], `${name} ${label}: page errors`);
+ console.log(`${name}: ok ${label}`);
+ } catch (e) { failures.push(e.message); console.log(`${name}: FAIL ${label}: ${e.message.split('\n')[0]}`); }
+ }
const cell = page.locator('td[data-x="0"][data-y="0"]');
- await cell.click();
- writes = [];
- await cell.click({button:'right'});
- const shown = await page.evaluate(()=>[...document.querySelectorAll('.jcontextmenu')]
- .filter(m=>getComputedStyle(m).display!=='none' && m.offsetHeight>0 && m.children.length>0).map(m=>m.innerText));
- assert.deepEqual(shown, [], `${name}: the library context menu opened: ${JSON.stringify(shown)}`);
- for (const label of ['Delete selected rows','Add comments','Save as','About']) {
- assert.equal(await page.getByText(label).filter({visible:true}).count(), 0, `${name}: "${label}" is on screen`);
+ await check('right-click opens no library menu', async () => {
+ await cell.click({button:'right'});
+ const shown = await page.evaluate(()=>[...document.querySelectorAll('.jcontextmenu')]
+ .filter(m=>getComputedStyle(m).display!=='none' && m.offsetHeight>0 && m.children.length>0).map(m=>m.innerText));
+ assert.deepEqual(shown, [], `${name}: the library context menu opened: ${JSON.stringify(shown)}`);
+ for (const label of ['Delete selected rows','Add comments','Save as','About'])
+ assert.equal(await page.getByText(label).filter({visible:true}).count(), 0, `${name}: "${label}" is on screen`);
+ await page.keyboard.press('Escape');
+ // Nothing may claim the event: the browser's own menu should show.
+ const prevented = await cell.evaluate(td => {
+ const r = td.getBoundingClientRect();
+ const e = new MouseEvent('contextmenu', {bubbles:true, cancelable:true, button:2, clientX:r.x+5, clientY:r.y+5});
+ td.dispatchEvent(e); return e.defaultPrevented;
+ });
+ assert.equal(prevented, false, `${name}: something swallowed the right-click`);
+ });
+ // Playwright's WebKit treats an unclaimed Backspace as history-back and
+ // leaves the page (on main too), so Backspace is driven in Chromium only;
+ // the library's handler takes Delete and Backspace down one path.
+ for (const key of name === 'webkit' ? ['Delete'] : ['Delete', 'Backspace']) {
+ await check(`row header + ${key} deletes nothing`, async () => {
+ await page.locator('td.jss_row[data-y="1"]').click();
+ await page.keyboard.press(key);
+ });
+ await check(`column header + ${key} deletes nothing`, async () => {
+ await page.locator('thead td[data-x="0"]').click();
+ await page.keyboard.press(key);
+ });
+ }
+ for (const combo of ['Control+s', 'Meta+s']) {
+ await check(`${combo} downloads nothing`, async () => { await page.keyboard.press(combo); });
}
- // Dispatched as a real contextmenu event; defaultPrevented tells us whether
- // anything claimed it (the library's menu does, the browser's does not).
- const prevented = await cell.evaluate(td => {
- const e = new MouseEvent('contextmenu', {bubbles:true, cancelable:true, button:2, clientX:td.getBoundingClientRect().x+5, clientY:td.getBoundingClientRect().y+5});
- td.dispatchEvent(e);
- return e.defaultPrevented;
+ await check('dragging a row header moves no row', async () => {
+ const from = await page.locator('td.jss_row[data-y="0"]').boundingBox();
+ const to = await page.locator('td.jss_row[data-y="2"]').boundingBox();
+ await page.mouse.move(from.x + from.width - 3, from.y + from.height / 2);
+ await page.mouse.down();
+ await page.mouse.move(to.x + to.width - 3, to.y + to.height / 2, {steps: 8});
+ await page.mouse.up();
});
- assert.equal(prevented, false, `${name}: something swallowed the right-click instead of leaving the browser menu`);
- assert.deepEqual(writes, [], `${name}: right-click sent writes`);
- assert.deepEqual(errors, [], `${name}: page errors`);
- console.log(`${name}: no library context menu`);
} finally {
await browser.close();
}
}
+if (failures.length) { console.error(`${failures.length} failure(s)`); process.exit(1); }