amadan

oficina / sheets Public

fix(menubar): reaching for a menu with a mouse closed it again

The menu bar worked perfectly from the keyboard and did not work with a
pointer at all. Reaching for a menu is ENTER THEN CLICK: the hover-switch
opened it on pointerenter, and <summary>'s own activation behaviour
toggled it shut a millisecond later. Every menu appeared inert.

Nothing in the Go suite can see this, and reading the code does not
show it either — both halves are correct on their own. tools/drive/
chrome.mjs is the browser drive that found it, kept as the sibling of
drive.mjs: it exercises the menu bar, the toolbar, the formula bar, the
cell paint, export, and the 320px reflow budget, and shoots light and
dark screenshots.

Two fixes in one: a hover-open now also leaves focus alone. openMenu
took a boolean and always focused something; hovering across the bar
therefore dragged the keyboard behind the cursor and fired the focusout
handler on every menu the pointer crossed. It now takes 'items',
'summary' or 'none'.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0182f7QfojuuqdS4Xd2fYFaL
Paul Campbell pushed by paul@keymail.dev 15a3a84d5aa1df57543b6725f9a53fc90e64138a parent b2db88c
2 files changed, +245 −9
  • internal/sheets/static/menubar.js +42 −9
  • tools/drive/chrome.mjs +203 −0
diff --git a/internal/sheets/static/menubar.js b/internal/sheets/static/menubar.js
index e0b382d..ba2d976 100644
--- a/internal/sheets/static/menubar.js
+++ b/internal/sheets/static/menubar.js
@@ -73,11 +73,20 @@
}
}
- function openMenu(details, focusFirst) {
+ /* where is 'items', 'summary' or 'none'.
+ *
+ * Three and not a boolean because the pointer path needs the third:
+ * hovering across the bar must not move focus. A menu bar that
+ * focuses whatever the mouse passes over drags the keyboard around
+ * behind the cursor, and here it would also fire the focusout handler
+ * below on every menu the pointer crossed. */
+ function openMenu(details, where) {
+ if (!details) return;
menus.forEach(function (d) { if (d !== details) d.open = false; });
details.open = true;
rove(details);
- var list = focusFirst ? items(details) : [];
+ if (where === 'none') return;
+ var list = where === 'items' ? items(details) : [];
if (list.length) list[0].focus();
else { var s = summaryOf(details); if (s) s.focus(); }
}
@@ -114,17 +123,17 @@
switch (e.key) {
case 'ArrowDown':
- if (isTop) openMenu(menu, true);
+ if (isTop) openMenu(menu, 'items');
else move(menu, target, 1);
break;
case 'ArrowUp':
- if (isTop) openMenu(menu, true);
+ if (isTop) openMenu(menu, 'items');
else move(menu, target, -1);
break;
case 'ArrowRight':
var sub = submenuOf(target);
if (sub && !sub.open) { sub.open = true; focusInside(sub); break; }
- openMenu(step(menu, 1), !isTop || menu.open);
+ openMenu(step(menu, 1), (!isTop || menu.open) ? 'items' : 'summary');
break;
case 'ArrowLeft':
var inner = target.closest('details[rst-menu-group]');
@@ -134,7 +143,7 @@
break;
}
if (inner && inner.open) { inner.open = false; break; }
- openMenu(step(menu, -1), !isTop || menu.open);
+ openMenu(step(menu, -1), (!isTop || menu.open) ? 'items' : 'summary');
break;
case 'Home':
if (isTop) { consumed = false; break; }
@@ -151,7 +160,7 @@
* <a> or a <button>, which both do the right thing already. */
if (!isTop) { consumed = false; break; }
e.preventDefault();
- openMenu(menu, true);
+ openMenu(menu, 'items');
break;
default:
consumed = false;
@@ -213,15 +222,39 @@
/* Pointer hover switches between open menus, the way every menu bar
* does: open File, slide to Edit, Edit opens. Pointer only — on a
* touch screen "hover" is the tap that opened it, and this would
- * reopen a menu the tap was closing. */
+ * reopen a menu the tap was closing.
+ *
+ * hoverOpened is not a nicety. Reaching for a menu with a mouse is
+ * ENTER THEN CLICK, and <summary>'s activation behaviour toggles: the
+ * hover opens it, the click that follows a millisecond later closes
+ * it again, and the menu bar appears not to work at all with a
+ * pointer while testing perfectly from the keyboard. It did exactly
+ * that, and only a real browser found it. So a click on the summary
+ * this handler just opened suppresses the native toggle once. */
+ var hoverOpened = null;
+
menus.forEach(function (d) {
var s = summaryOf(d);
if (!s) return;
+
s.addEventListener('pointerenter', function (e) {
if (e.pointerType !== 'mouse') return;
if (d.open) return;
if (!menus.some(function (m) { return m.open; })) return;
- openMenu(d, false);
+ openMenu(d, 'none');
+ hoverOpened = d;
+ });
+
+ /* Leaving without clicking clears the suppression, so a later click
+ * on this summary closes the menu the way it should. */
+ s.addEventListener('pointerleave', function () {
+ if (hoverOpened === d) hoverOpened = null;
+ });
+
+ s.addEventListener('click', function (e) {
+ if (hoverOpened !== d) return;
+ hoverOpened = null;
+ e.preventDefault();
});
});
}());
diff --git a/tools/drive/chrome.mjs b/tools/drive/chrome.mjs
new file mode 100644
index 0000000..2d6add5
--- /dev/null
+++ b/tools/drive/chrome.mjs
@@ -0,0 +1,203 @@
+/* Drive the editor chrome in a real browser.
+ *
+ * The sibling of drive.mjs, which drives live sync across two tabs.
+ * This one drives the menu bar, the toolbar, the formula bar and the
+ * cell paint. Like its sibling it is NOT part of `make test`: it needs
+ * a browser and a running instance, neither of which CI has.
+ *
+ * It earned its place immediately. The menu bar tested perfectly from
+ * the keyboard and did not work with a mouse at all: reaching for a
+ * menu is ENTER THEN CLICK, the hover-switch opened it, and <summary>'s
+ * own activation behaviour closed it again a millisecond later. No Go
+ * test can see that, and neither could reading the code.
+ *
+ * Usage, from the repository root — see drive.mjs for the full recipe:
+ *
+ * SHEETS_SOCK=$D/s.sock SHEETS_LOG=$D/s.log OUT=$D node tools/drive/chrome.mjs
+ *
+ * OUT is where the four screenshots land (light, dark, 320px, 768px).
+ */
+import net from 'node:net';
+import fs from 'node:fs';
+const PLAYWRIGHT = process.env.PLAYWRIGHT || 'playwright';
+const pw = await import(PLAYWRIGHT);
+/* A CJS build imported from ESM puts its exports on .default. */
+const chromium = pw.chromium || (pw.default && pw.default.chromium);
+if (!chromium) { console.error('playwright: no chromium export'); process.exit(1); }
+
+const SOCK = process.env.SHEETS_SOCK;
+const LOG = process.env.SHEETS_LOG;
+const PORT = parseInt(process.env.SHEETS_PORT || '8799', 10);
+const OUT = process.env.OUT || '.';
+
+const proxy = net.createServer((c) => {
+ const u = net.connect(SOCK);
+ c.pipe(u); u.pipe(c);
+ c.on('error', () => u.destroy());
+ u.on('error', () => c.destroy());
+});
+await new Promise((r) => proxy.listen(PORT, '127.0.0.1', r));
+const base = `http://127.0.0.1:${PORT}`;
+
+const fails = [];
+const ok = (name) => console.log(` ok ${name}`);
+const bad = (name, detail) => { fails.push(`${name}: ${detail}`); console.log(` FAIL ${name} — ${detail}`); };
+const check = (name, cond, detail) => cond ? ok(name) : bad(name, detail || 'false');
+
+const browser = await chromium.launch();
+const ctx = await browser.newContext({ viewport: { width: 1440, height: 900 } });
+const page = await ctx.newPage();
+const errors = [];
+page.on('console', (m) => { if (m.type() === 'error') errors.push(m.text()); });
+page.on('pageerror', (e) => errors.push(String(e)));
+
+/* Sign in through the link the log prints. Strip at the first backslash:
+ * the log escapes the mail body, so a naive grep captures the token plus
+ * "\n\nIt..." and the link comes back expired. */
+await page.goto(`${base}/signin`);
+await page.fill('input[type=email]', 'paul@example.com');
+await Promise.all([page.waitForLoadState('networkidle'), page.click('button[type=submit]')]);
+const log = fs.readFileSync(LOG, 'utf8');
+const m = [...log.matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop();
+if (!m) { console.error('no magic link in the log'); process.exit(1); }
+await page.goto(m[0]);
+await page.waitForURL(/\/sheets/);
+
+/* A sheet with something in it. */
+await page.fill('#title', 'Commission Q2');
+await Promise.all([page.waitForURL(/\/sheets\/\d+$/), page.click('form[action="/sheets"] button[type=submit]')]);
+const url = page.url();
+await page.waitForFunction(() => window.SheetsGrid && window.SheetsGrid.worksheet());
+
+async function type(r, c, text) {
+ await page.evaluate(([r, c, text]) => window.SheetsGrid.write(r, c, text), [r, c, text]);
+}
+const rows = [
+ ['Invoice Quarter', 'Event', 'Invoice (GBP)', 'Commission'],
+ ['Q2/26', 'Juxt', '3228.33', '=C2*0.1'],
+ ['Q2/26', 'Techarena', '1840.00', '=C3*0.1'],
+ ['Q2/26', 'Seapoint', '920.50', '=C4*0.1'],
+ ['', 'Total commission owing', '', '=SUM(D2:D4)'],
+];
+for (let r = 0; r < rows.length; r++) {
+ for (let c = 0; c < rows[r].length; c++) if (rows[r][c]) await type(r, c, rows[r][c]);
+}
+await page.waitForTimeout(400);
+
+console.log('\n— the formula engine still owns every painted string');
+const d5 = await page.evaluate(() => window.SheetsGrid.worksheet().getValueFromCoords(3, 4));
+check('SUM of the commission column', d5 === '598.883', `got ${JSON.stringify(d5)}`);
+
+console.log('\n— the menu bar');
+await page.click('[data-menubar] details:nth-child(1) > summary');
+check('File opens', await page.isVisible('a[href$="/export.xlsx"]'));
+await page.click('[data-menubar] details:nth-child(2) > summary');
+check('Edit opens and File closed', await page.isVisible('[data-cmd="fill-down"]')
+ && !(await page.evaluate(() => document.querySelector('[data-menubar] details:nth-child(1)').open)),
+ 'the exclusive group is not working');
+await page.keyboard.press('Escape');
+
+/* Keyboard: focus the first summary, walk right, open, walk down. */
+await page.evaluate(() => document.querySelector('[data-menubar] > details > summary').focus());
+await page.keyboard.press('ArrowRight');
+await page.keyboard.press('ArrowDown');
+const focusTag = await page.evaluate(() => document.activeElement.textContent.trim().split('\n')[0]);
+check('arrows open a menu and land on its first item', focusTag.length > 0, `focus is on ${JSON.stringify(focusTag)}`);
+const cursorMoved = await page.evaluate(() => {
+ const s = window.SheetsGrid.selection();
+ return s ? `${s.r},${s.c}` : 'none';
+});
+await page.keyboard.press('Escape');
+
+console.log('\n— formatting reaches the cell and the paint reaches the td');
+await page.evaluate(() => window.SheetsGrid.worksheet().updateSelectionFromCoords(0, 0, 3, 0));
+await page.waitForTimeout(150);
+await page.click('[data-toolbar] [data-cmd="bold"]');
+await page.waitForTimeout(400);
+const hdr = await page.evaluate(() => {
+ const td = window.SheetsGrid.worksheet().records[0][0].element;
+ return { attr: td.hasAttribute('data-b'), weight: getComputedStyle(td).fontWeight };
+});
+check('header row is bold on the td', hdr.attr, 'data-b was never set');
+check('...and the CSS actually applies it', parseInt(hdr.weight, 10) >= 600, `font-weight ${hdr.weight}`);
+
+const pressed = await page.getAttribute('[data-toolbar] [data-cmd="bold"]', 'aria-pressed');
+check('the toolbar reflects the selection', pressed === 'true', `aria-pressed=${pressed}`);
+
+/* Fill: a swatch by name, resolved against the theme's lightness. */
+await page.evaluate(() => window.SheetsGrid.worksheet().updateSelectionFromCoords(1, 4, 3, 4));
+await page.waitForTimeout(150);
+await page.click('[data-toolbar] [data-tool][aria-label="Fill colour"]');
+await page.click('[data-toolbar] [data-cmd="fill"][data-value="green"]');
+await page.waitForTimeout(400);
+const fill = await page.evaluate(() => {
+ const td = window.SheetsGrid.worksheet().records[4][1].element;
+ return { attr: td.hasAttribute('data-fill'), bg: getComputedStyle(td).backgroundColor };
+});
+check('the total row has a fill', fill.attr, 'data-fill was never set');
+check('...and it paints a real colour', fill.bg && fill.bg !== 'rgba(0, 0, 0, 0)', `background ${fill.bg}`);
+
+/* A number format changes the SERVER-rendered string, not a client one. */
+await page.evaluate(() => window.SheetsGrid.worksheet().updateSelectionFromCoords(3, 1, 3, 4));
+await page.waitForTimeout(150);
+await page.click('[data-toolbar] [data-tool][aria-label="Number format"]');
+await page.click('[data-toolbar] [data-cmd="numfmt"][data-value="0.00"]');
+await page.waitForTimeout(500);
+const formatted = await page.evaluate(() => window.SheetsGrid.worksheet().getValueFromCoords(3, 4));
+check('number format re-renders server-side', formatted === '598.88', `got ${JSON.stringify(formatted)}`);
+
+console.log('\n— the formula bar');
+await page.evaluate(() => window.SheetsGrid.worksheet().updateSelectionFromCoords(3, 4, 3, 4));
+await page.waitForTimeout(200);
+check('name box shows the reference', (await page.inputValue('[data-name-box]')) === 'D5',
+ `got ${await page.inputValue('[data-name-box]')}`);
+check('formula bar shows the input, not the value',
+ (await page.inputValue('[data-formula-input]')) === '=SUM(D2:D4)',
+ `got ${await page.inputValue('[data-formula-input]')}`);
+
+await page.fill('[data-name-box]', 'B2');
+await page.press('[data-name-box]', 'Enter');
+await page.waitForTimeout(250);
+const jumped = await page.evaluate(() => { const s = window.SheetsGrid.selection(); return `${s.r},${s.c}`; });
+check('typing a reference moves the selection', jumped === '1,1', `selection is ${jumped}`);
+
+console.log('\n— export');
+const dl = await page.request.get(`${url}/export.xlsx`);
+const buf = await dl.body();
+check('export downloads a real xlsx', dl.status() === 200 && buf[0] === 0x50 && buf[1] === 0x4b,
+ `status ${dl.status()}, ${buf.length} bytes`);
+check('...named after the sheet',
+ (dl.headers()['content-disposition'] || '').includes('Commission Q2.xlsx'),
+ dl.headers()['content-disposition']);
+
+console.log('\n— screenshots and reflow');
+await page.evaluate(() => document.activeElement.blur());
+await page.screenshot({ path: `${OUT}/sheet-light.png` });
+
+/* Dark, chosen explicitly through the form the account menu posts. */
+await page.evaluate(() => {
+ const f = document.createElement('form');
+ f.method = 'post'; f.action = '/mode';
+ f.innerHTML = '<input name="return" value="' + location.pathname + '"><input name="mode" value="dark">';
+ document.body.appendChild(f); f.submit();
+});
+await page.waitForFunction(() => window.SheetsGrid && window.SheetsGrid.worksheet());
+await page.waitForTimeout(700);
+await page.screenshot({ path: `${OUT}/sheet-dark.png` });
+const darkFill = await page.evaluate(() => getComputedStyle(window.SheetsGrid.worksheet().records[4][1].element).backgroundColor);
+check('the fill follows the theme into dark', darkFill !== fill.bg, `light ${fill.bg} vs dark ${darkFill}`);
+
+for (const w of [320, 768]) {
+ await page.setViewportSize({ width: w, height: 800 });
+ await page.waitForTimeout(300);
+ const over = await page.evaluate(() => document.documentElement.scrollWidth - document.documentElement.clientWidth);
+ check(`no horizontal scroll at ${w}px`, over <= 0, `overflows by ${over}px`);
+ await page.screenshot({ path: `${OUT}/sheet-${w}.png` });
+}
+
+check('no console errors', errors.length === 0, errors.slice(0, 3).join(' | '));
+
+await browser.close();
+proxy.close();
+console.log(fails.length ? `\n${fails.length} FAILED\n` + fails.map((f) => ' ' + f).join('\n') : '\nall checks passed');
+process.exit(fails.length ? 1 : 0);