Merge pull request #80 from keepass-web/issue-72-78-release-ux

1.0.0 UX fixes (#72,#73,#76,#77,#78)

Author: bishopb

docs/PAGES.md

@@ -37,6 +37,8 @@ flowchart TD
 
 "Create a new database" is the other way into the same iframe: with no file to sniff, the connector embeds `0x67.html` straight away and, once it announces readiness, tells it to start a fresh database instead of opening one (`kw-create`, the create-side counterpart to `kw-open` in `packages/embed-protocol`). Naming and creating the database — `Kdbx.create` and everything it depends on — happens exactly where opening one does, inside `0x67.html`; the connector never gains its own copy of that logic, it only decides which of the two messages to send.
 
+A connector owns two things beyond the file itself, both because the browser gives them to whichever document owns the tab rather than to the one inside the iframe. The first is the tab: the app knows which database is open and whether it is locked, but only the connector can name the tab and mark it with that state, so the app reports and the connector applies — which is why that logic lives in `pages/shared/` rather than in either page. The second is the keyboard: a keystroke goes to whichever document has focus, so a find pressed while the visitor is on the connector's own chrome would search the connector's page instead of the database, and the connector forwards it to the app rather than let that happen.
+
 On save, since there is nowhere to write back to, the local connector downloads the updated bytes the same way `0x67.html` would if opened standalone — the only piece of this connector that's genuinely local-specific. This applies equally to a freshly created database: its first save is just a download, named for whatever the create screen's own form was given.
 
 ## How the Google Drive connector works

e2e/auto-lock.test.ts

@@ -81,7 +81,7 @@ test('a tab left hidden locks the embedded database on its own', async () => {
   await otherTab.close();
   assert.equal(
     await page.title(),
-    `🔒 ${basename(fixture.path)} - KeePass Web - Local file`,
+    `🔒 ${basename(fixture.path)} - Locked - KeePass Web - Local file`,
     'and the tab bar shows it locked, without being opened',
   );
 });

e2e/entry-copy.test.ts

@@ -1,7 +1,9 @@
-/** Real-browser coverage for the entry table's controls (issue #67). jsdom
- * dispatches events straight at an element; only a real browser hit-tests a
- * coordinate, so this is what proves the cells and the open control are
- * actually clickable where they render.
+/** Real-browser coverage for the entry table's controls (issues #67, #76,
+ * #77). jsdom dispatches events straight at an element and has no layout
+ * engine at all; only a real browser hit-tests a coordinate and gives a cell
+ * or a column a width, so this is what proves the controls are clickable where
+ * they render, that the copy control holds the cell's right edge, and that a
+ * dragged column actually changes size.
  *
  * What gets copied is asserted in the jsdom tests instead: headless Chrome
  * refuses `navigator.clipboard.writeText` outright ("Write permission denied"),
@@ -49,7 +51,7 @@ async function usernameCellCentre(): Promise<{ x: number; y: number }> {
   const box = await app.$$eval(
     '.entry-table tbody td',
     (cells, name) => {
-      const cell = cells.find((c) => c.firstChild?.textContent === name);
+      const cell = cells.find((c) => c.querySelector('.entry-cell-text')?.textContent === name);
       if (!cell) return null;
       const rect = cell.getBoundingClientRect();
       return { x: rect.x + rect.width / 2, y: rect.y + rect.height / 2 };
@@ -69,6 +71,62 @@ test('clicking a value never opens the entry', async () => {
   assert.equal(await app.$('#detail-title'), null, 'the card stayed shut');
 });
 
+test('the copy control holds the right edge of its cell', async () => {
+  const geometry = await app.$$eval(
+    '.entry-table tbody td',
+    (cells, name) => {
+      const cell = cells.find((c) => c.querySelector('.entry-cell-text')?.textContent === name);
+      const hint = cell?.querySelector('.copy-hint');
+      const text = cell?.querySelector('.entry-cell-text');
+      if (!cell || !hint || !text) return null;
+      const inner = cell.querySelector('.entry-cell') as HTMLElement;
+      return {
+        gapToTheRightEdge: inner.getBoundingClientRect().right - hint.getBoundingClientRect().right,
+        gapAfterTheText: hint.getBoundingClientRect().left - text.getBoundingClientRect().right,
+      };
+    },
+    'octocat',
+  );
+  assert.ok(geometry, 'the username cell carries both a text box and a copy control');
+
+  assert.ok(
+    geometry.gapToTheRightEdge < 1,
+    `the control sits at the cell's right edge, ${geometry.gapToTheRightEdge}px short of it`,
+  );
+  // A short value leaves room, and the control does not follow the text into it.
+  assert.ok(
+    geometry.gapAfterTheText > 8,
+    `it is pinned there rather than trailing the text, ${geometry.gapAfterTheText}px behind it`,
+  );
+});
+
+test('dragging a column header changes that column width', async () => {
+  const handle = await app.$('th[data-column="username"] .col-resize');
+  assert.ok(handle, 'every resizable column carries a handle');
+
+  const widthOf = (): Promise<number> =>
+    app.$eval('th[data-column="username"]', (th) => th.getBoundingClientRect().width);
+  const before = await widthOf();
+
+  const box = await handle.boundingBox();
+  assert.ok(box, 'the handle is laid out');
+  const y = box.y + box.height / 2;
+  await page.mouse.move(box.x + box.width / 2, y);
+  await page.mouse.down();
+  await page.mouse.move(box.x + box.width / 2 + 90, y);
+  await page.mouse.up();
+
+  const after = await widthOf();
+  assert.ok(after > before + 60, `the column widened, from ${before}px to ${after}px`);
+  assert.equal(
+    await app.$eval('th[data-column="username"] .col-resize', (el) =>
+      el.getAttribute('aria-valuenow'),
+    ),
+    String(Math.round(after)),
+    'and says so to anyone reading it out',
+  );
+});
+
 test('the row control is the way into the card', async () => {
   const openButton = await app.$('.entry-table-open button');
   assert.ok(openButton, 'every row carries one');

e2e/local-to-app-embed.test.ts

@@ -69,13 +69,58 @@ test('dropping a file on local.html embeds a working 0x67 app that unlocks the s
   await iframeFrame.click('#unlock-btn');
 
   await iframeFrame.waitForSelector('.entry-table');
-  const titleText = await iframeFrame.$eval('.entry-table-title', (el) => el.textContent);
+  const titleText = await iframeFrame.$eval(
+    '.entry-table-title .entry-cell-text',
+    (el) => el.textContent,
+  );
   assert.ok(
     titleText?.includes(fixture.entryTitle),
     `unlocked vault shows the fixture entry, got "${titleText}"`,
   );
 });
 
+test('the find keystroke reaches the app even when focus is on the host page', async () => {
+  const fixture = await writeKdbxFixture();
+
+  await page.goto(`${server.origin}/local.html`, { waitUntil: 'networkidle0' });
+  const fileInput = (await page.waitForSelector('#file-input')) as ElementHandle<HTMLInputElement>;
+  assert.ok(fileInput, 'the file input exists');
+  await fileInput.uploadFile(fixture.path);
+
+  const iframeElement = await page.waitForSelector('#app-frame');
+  assert.ok(iframeElement, 'the app is embedded');
+  const app = await iframeElement.contentFrame();
+  assert.ok(app, 'the iframe has a content frame');
+
+  const passwordInput = await app.waitForSelector('#master-password');
+  assert.ok(passwordInput, 'the app shows its unlock screen');
+  await passwordInput.type(fixture.password);
+  await app.click('#unlock-btn');
+  await app.waitForSelector('.entry-table');
+
+  // Click the host's own chrome, so this document — not the iframe — is the
+  // one holding focus and the one the keystroke will be delivered to. Without
+  // the host forwarding it, the app would never see it at all.
+  await page.click('#host-filename');
+  await app.$eval('#search-input', (el) => (el as HTMLElement).blur());
+  assert.notEqual(
+    await app.evaluate(() => document.activeElement?.id),
+    'search-input',
+    'focus really is off the search field to begin with',
+  );
+
+  await page.keyboard.down('Control');
+  await page.keyboard.press('f');
+  await page.keyboard.up('Control');
+
+  await app.waitForFunction(() => document.activeElement?.id === 'search-input');
+  assert.equal(
+    await app.evaluate(() => document.activeElement?.id),
+    'search-input',
+    'the find crossed into the app and landed in its database-wide search',
+  );
+});
+
 test('clicking "Create a new database" on local.html embeds the app straight on its create-database screen', async () => {
   await page.goto(`${server.origin}/local.html`, { waitUntil: 'networkidle0' });
 

e2e/reveal-focus.test.ts

@@ -0,0 +1,65 @@
+/** The caret has to survive a click on a control that acts on the field it
+ * sits beside (issue #72). jsdom never moves focus on a press at all, so the
+ * pages suite can only assert that the press is cancelled; a real browser is
+ * the only place the focus move it prevents actually happens. The unlock
+ * screen's reveal toggle stands in for the entry-edit row's controls too —
+ * all of them go through the same helper. */
+import assert from 'node:assert/strict';
+import { after, before, test } from 'node:test';
+import { fileURLToPath } from 'node:url';
+import puppeteer, { type Browser, type ElementHandle, type Frame, type Page } from 'puppeteer-core';
+import { resolveChromePath } from './support/chrome.ts';
+import { type DistServer, startDistServer } from './support/dist-server.ts';
+import { writeKdbxFixture } from './support/fixture.ts';
+import { resolveLaunchOptions } from './support/launch-options.ts';
+
+const distDir = fileURLToPath(new URL('../dist', import.meta.url));
+
+let server: DistServer;
+let browser: Browser;
+let page: Page;
+
+before(async () => {
+  server = await startDistServer(distDir);
+  browser = await puppeteer.launch({
+    executablePath: resolveChromePath(),
+    ...resolveLaunchOptions(),
+    args: ['--no-sandbox'],
+  });
+  page = await browser.newPage();
+});
+
+after(async () => {
+  await browser.close();
+  await server.close();
+});
+
+test('revealing the master password leaves the caret in the field', async () => {
+  const fixture = await writeKdbxFixture();
+
+  await page.goto(`${server.origin}/local.html`, { waitUntil: 'networkidle0' });
+  const fileInput = (await page.waitForSelector('#file-input')) as ElementHandle<HTMLInputElement>;
+  assert.ok(fileInput, 'the chooser offers a file input');
+  await fileInput.uploadFile(fixture.path);
+
+  const frameElement = await page.waitForSelector('#app-frame');
+  assert.ok(frameElement, 'the app is embedded in an iframe');
+  const app = (await frameElement.contentFrame()) as Frame;
+
+  // Stop at the unlock screen: this is about typing a password, not reading a database.
+  const passwordInput = await app.waitForSelector('#master-password');
+  assert.ok(passwordInput, 'the embedded app shows its unlock screen');
+  await passwordInput.type('half-typed');
+
+  await app.click('[data-action="toggle-password"]');
+
+  const state = await app.evaluate(() => ({
+    focused: document.activeElement?.id ?? '',
+    type: (document.getElementById('master-password') as HTMLInputElement).type,
+    value: (document.getElementById('master-password') as HTMLInputElement).value,
+  }));
+
+  assert.equal(state.focused, 'master-password', 'the field kept focus, so typing carries on');
+  assert.equal(state.type, 'text', 'and the toggle still revealed the password');
+  assert.equal(state.value, 'half-typed', 'without disturbing what was already typed');
+});

e2e/tab-title.test.ts

@@ -1,9 +1,9 @@
-/** The tab title belongs to local.html, but only the embedded 0x67 app knows
- * which database is open and whether it is locked — so the title is right
- * only if a real cross-document postMessage is delivered and handled. The
- * jsdom suites test each page in its own isolated window and cannot show
- * that; this drives the built distributables in Chrome, where the two
- * documents really are separate. */
+/** The tab belongs to local.html, but only the embedded 0x67 app knows which
+ * database is open and whether it is locked — so the tab's name and its icon
+ * are right only if a real cross-document postMessage is delivered and
+ * handled. The jsdom suites test each page in its own isolated window and
+ * cannot show that; this drives the built distributables in Chrome, where the
+ * two documents really are separate. */
 import assert from 'node:assert/strict';
 import { basename } from 'node:path';
 import { after, before, test } from 'node:test';
@@ -51,12 +51,16 @@ async function waitForTitle(expected: string): Promise<void> {
   }
 }
 
+const tabIcon = (): Promise<string> =>
+  page.$eval('link[rel="icon"]', (link) => link.getAttribute('href') ?? '');
+
 test('the tab names the open database and tracks its lock state', async () => {
   const fixture = await writeKdbxFixture();
   const filename = basename(fixture.path);
 
   await page.goto(`${server.origin}/local.html`, { waitUntil: 'networkidle0' });
   assert.equal(await page.title(), BASE_TITLE, 'nothing open, so the tab is just this page');
+  const pageIcon = await tabIcon();
 
   // waitForSelector can't infer the element type from an id selector.
   const fileInput = (await page.waitForSelector('#file-input')) as ElementHandle<HTMLInputElement>;
@@ -68,15 +72,19 @@ test('the tab names the open database and tracks its lock state', async () => {
   const iframeFrame = await iframeElement.contentFrame();
   assert.ok(iframeFrame, 'the iframe has a content frame');
 
-  await waitForTitle(`🔒 ${filename} - ${BASE_TITLE}`);
+  await waitForTitle(`🔒 ${filename} - Locked - ${BASE_TITLE}`);
+  const lockedIcon = await tabIcon();
+  assert.notEqual(lockedIcon, pageIcon, 'a held database is not the page at rest');
 
   const passwordInput = await iframeFrame.waitForSelector('#master-password');
   assert.ok(passwordInput, 'the embedded app went straight to its unlock screen');
   await passwordInput.type(fixture.password);
   await iframeFrame.click('#unlock-btn');
 
   await iframeFrame.waitForSelector('.entry-table');
-  await waitForTitle(`🔓 ${filename} - ${BASE_TITLE}`);
+  await waitForTitle(`🔓 ${filename} - Unlocked - ${BASE_TITLE}`);
+  const unlockedIcon = await tabIcon();
+  assert.notEqual(unlockedIcon, lockedIcon, 'and the two states do not share an icon');
 
   // Nothing is unsaved, but the app still asks before giving the database up.
   await page.click('[data-action="back-to-chooser"]');
@@ -86,4 +94,5 @@ test('the tab names the open database and tracks its lock state', async () => {
   await iframeFrame.click('#dlg-confirm-discard [data-action="confirm-discard"]');
   await page.waitForSelector('#drop-zone');
   await waitForTitle(BASE_TITLE);
+  assert.equal(await tabIcon(), pageIcon, 'closing hands the page its own icon back');
 });

packages/embed-protocol/src/index.ts

@@ -2,7 +2,8 @@
 keepass-web implementation and whatever host embeds it in an iframe.
 Centralizes shapes/guards/builders (previously duplicated per side) so
 both ends provably agree on the wire format: kw-ready, kw-open, kw-create,
-kw-save, kw-saved, kw-title, kw-close-request, kw-close-ack, kw-close. */
+kw-save, kw-saved, kw-title, kw-find, kw-close-request, kw-close-ack,
+kw-close. */
 
 export interface ReadyMessage {
   type: 'kw-ready';
@@ -38,6 +39,13 @@ export interface TitleMessage {
   locked: boolean;
 }
 
+/* Whichever document has focus receives the keystroke, and outside the iframe
+that is the host; it forwards the find rather than letting the browser's own
+search the one page it can see (#78). */
+export interface FindMessage {
+  type: 'kw-find';
+}
+
 export interface CloseRequestMessage {
   type: 'kw-close-request';
 }
@@ -96,6 +104,10 @@ export function isTitleMessage(data: unknown): data is TitleMessage {
   return typeof rec.filename === 'string' && typeof rec.locked === 'boolean';
 }
 
+export function isFindMessage(data: unknown): data is FindMessage {
+  return hasType(data, 'kw-find');
+}
+
 export function isCloseRequestMessage(data: unknown): data is CloseRequestMessage {
   return hasType(data, 'kw-close-request');
 }
@@ -134,6 +146,10 @@ export function titleMessage(filename: string, locked: boolean): TitleMessage {
   return { type: 'kw-title', filename, locked };
 }
 
+export function findMessage(): FindMessage {
+  return { type: 'kw-find' };
+}
+
 export function closeRequestMessage(): CloseRequestMessage {
   return { type: 'kw-close-request' };
 }

packages/embed-protocol/tests/index.test.ts

@@ -5,10 +5,12 @@ import {
   closeMessage,
   closeRequestMessage,
   createMessage,
+  findMessage,
   isCloseAckMessage,
   isCloseMessage,
   isCloseRequestMessage,
   isCreateMessage,
+  isFindMessage,
   isOpenMessage,
   isReadyMessage,
   isSavedMessage,
@@ -84,6 +86,14 @@ test('titleMessage / isTitleMessage round-trip', () => {
   assert.equal(isTitleMessage({ type: 'kw-title', filename: 42, locked: true }), false);
 });
 
+test('findMessage / isFindMessage round-trip', () => {
+  assert.deepEqual(findMessage(), { type: 'kw-find' });
+  assert.equal(isFindMessage(findMessage()), true);
+  assert.equal(isFindMessage(null), false);
+  assert.equal(isFindMessage(42), false);
+  assert.equal(isFindMessage({ type: 'nope' }), false);
+});
+
 test('closeRequestMessage / isCloseRequestMessage round-trip', () => {
   assert.deepEqual(closeRequestMessage(), { type: 'kw-close-request' });
   assert.equal(isCloseRequestMessage(closeRequestMessage()), true);