From 5d6d451a8b299b298a4e493a1a3aab7d200a0f62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kasper=20H=C3=A4gele?= Date: Sun, 27 Sep 2026 14:24:32 +0200 Subject: [PATCH] fix(app,web): put the caret in the target picker's search when it opens Reaching a node by name took three actions: open the picker, press its search field, type. Opening the app's target sheet and the web's sender picker now focuses the search in the same tap, so a phone raises its keyboard and the next keystroke searches. The app's sheet also opens on an empty query every time; a query left from the last open used to come back with a list narrowed by it. On web the field keeps its value, since it is the sender filter bound to ?sender=. Closing with the focus inside lets go of it rather than leaving it on a hidden field. Closes #714 Co-Authored-By: Claude Opus 5.5 --- app/changelog.json | 7 ++ app/src/__tests__/sheetfocus.test.js | 26 ++++++++ app/src/__tests__/targetlist.test.js | 41 ++++++++++++ app/src/app.js | 64 +++++++++---------- app/src/sheetfocus.js | 10 +++ app/src/targetlist.js | 19 +++++- .../2026-09-27-07-picker-search-focus.json | 7 ++ web/changelog.json | 7 ++ web/e2e/targetpicker.spec.js | 18 ++++++ web/map.js | 3 + web/multiselect.js | 14 +++- web/multiselect.test.js | 60 ++++++++++++++++- 12 files changed, 237 insertions(+), 39 deletions(-) create mode 100644 app/src/__tests__/sheetfocus.test.js create mode 100644 app/src/__tests__/targetlist.test.js create mode 100644 app/src/sheetfocus.js create mode 100644 changelog.d/2026-09-27-07-picker-search-focus.json diff --git a/app/changelog.json b/app/changelog.json index b2ca7c2a..e80778f8 100644 --- a/app/changelog.json +++ b/app/changelog.json @@ -20,6 +20,13 @@ "title": "A larger audio buffer", "body": "Sound uses a larger output buffer, against ticking while the app is busy right after start." }, + { + "id": "2026-09-27-picker-search-focus", + "date": "2026-09-27", + "where": "both", + "title": "Type straight away in the target picker", + "body": "Opening the target picker puts the cursor in its search. In the app it opens on an empty search every time." + }, { "id": "2026-09-26-broker-presets-shown", "date": "2026-09-26", diff --git a/app/src/__tests__/sheetfocus.test.js b/app/src/__tests__/sheetfocus.test.js new file mode 100644 index 00000000..163c3658 --- /dev/null +++ b/app/src/__tests__/sheetfocus.test.js @@ -0,0 +1,26 @@ +import { describe, it, expect, vi } from 'vitest' +import { closeSheet } from '../sheetfocus.js' + +// A stand-in for a sheet and its toggle: the sheet holds a set of elements. +const setup = (focusInside) => { + const field = { id: 'ts-search' }, outside = { id: 'map' } + const sheet = { hidden: false, contains: (x) => x === field } + const toggle = { focus: vi.fn() } + const doc = { activeElement: focusInside ? field : outside } + return { sheet, toggle, doc } +} + +describe('closeSheet (#714)', () => { + it('hands the focus back to the toggle when it was inside the sheet', () => { + const { sheet, toggle, doc } = setup(true) + closeSheet(sheet, toggle, doc) + expect(sheet.hidden).toBe(true) + expect(toggle.focus).toHaveBeenCalledTimes(1) + }) + it('leaves the focus where it is when it was elsewhere', () => { + const { sheet, toggle, doc } = setup(false) + closeSheet(sheet, toggle, doc) + expect(sheet.hidden).toBe(true) + expect(toggle.focus).not.toHaveBeenCalled() + }) +}) diff --git a/app/src/__tests__/targetlist.test.js b/app/src/__tests__/targetlist.test.js new file mode 100644 index 00000000..185619c7 --- /dev/null +++ b/app/src/__tests__/targetlist.test.js @@ -0,0 +1,41 @@ +import { describe, it, expect } from 'vitest' +import { createTargetList } from '../targetlist.js' + +// #714. The target chip opened the sheet and focused nothing, so reaching a +// node by name took a press on the field between the chip and the first +// keystroke. And reset() reset paging only: reopening the sheet brought back +// the last query and a list narrowed by it, with nothing on screen saying why. +// No jsdom in this suite; the elements are the fakes the list touches. +function fakes(query) { + const listEl = { scrollTop: 0, children: [], addEventListener() {}, replaceChildren(...c) { this.children = c } } + const searchEl = { value: query, focusedWith: null, listeners: {}, addEventListener(type, fn) { this.listeners[type] = fn }, focus(opts) { this.focusedWith = opts || {} } } + const browseEl = { hidden: false } + const list = createTargetList(listEl, { searchEl, browseEl }) + return { list, listEl, searchEl, browseEl } +} + +describe('opening the target sheet (#714)', () => { + it('puts the caret in the search field, without scrolling the page', () => { + const f = fakes('') + f.list.open() + expect(f.searchEl.focusedWith).toEqual({ preventScroll: true }) + }) + + it('starts from an empty query every time, and repaints at once', () => { + // A query that matches nothing paints the "No senders match." row, which + // is the one element render builds with no rows at all. + globalThis.document = { createElement: () => ({ className: '', textContent: '' }) } + try { + const f = fakes('dikke') + f.list.render([], new Set(), Date.now(), null) + expect(f.listEl.children).toHaveLength(1) + // A query hides the browse chrome (Top and the list header). + expect(f.browseEl.hidden).toBe(true) + f.list.open() + expect(f.searchEl.value).toBe('') + expect(f.listEl.children).toHaveLength(0) + // What a user sees come back: the browse chrome, with the query gone. + expect(f.browseEl.hidden).toBe(false) + } finally { delete globalThis.document } + }) +}) diff --git a/app/src/app.js b/app/src/app.js index ccca2b38..6c7aafac 100644 --- a/app/src/app.js +++ b/app/src/app.js @@ -76,6 +76,7 @@ import { fabRingSvg } from './fabring.js' import { SOUND_MODES, nextSoundMode, receptionCue, createSoundEngine } from './sound.js' import { parseVersion, isUpdateAvailable } from './update.js' import { fetchMe, postAuth, validateRegistration, buildRegisterBody, buildLoginBody, buildLinkBody, accountDisplayState, submitLabelForMode } from './auth.js' +import { closeSheet } from './sheetfocus.js' // --------------------------------------------------------------------------- // State @@ -2580,7 +2581,7 @@ function buildFilterSheet() { drawOnce() }) - el('fs-close').addEventListener('click', () => { sheet.hidden = true }) + el('fs-close').addEventListener('click', () => closeSheet(sheet, el('filter-pill'))) } function buildTargetSheet() { @@ -2623,7 +2624,7 @@ function buildTargetSheet() { document.dispatchEvent(new CustomEvent('hunt:isolate-sender', { detail: null })) }) - el('ts-close').addEventListener('click', () => { sheet.hidden = true }) + el('ts-close').addEventListener('click', () => closeSheet(sheet, el('target-chip'))) } function renderIgnoreList(listEl) { @@ -2862,7 +2863,7 @@ function buildSettingsSheet() { el('ss-conn-btn').addEventListener('click', () => { if (state.connected) { disconnectAll() - sheet.hidden = true + closeSheet(sheet, el('settings-btn')) } else { state.wakeLock.enable() connectAll() @@ -2919,7 +2920,7 @@ function buildSettingsSheet() { }) - el('ss-close').addEventListener('click', () => { sheet.hidden = true }) + el('ss-close').addEventListener('click', () => closeSheet(sheet, el('settings-btn'))) // The brokers page lives inside the settings sheet (#554), so a tap in it is // a tap inside the sheet for the outside-click dismissal below. @@ -2958,7 +2959,7 @@ function buildSettingsSheet() { // Replaces the old topbar "?" button (#281): closes the sheet so the // walkthrough it re-opens isn't hidden behind it. el('ss-about-howto').addEventListener('click', () => { - el('settings-sheet').hidden = true + closeSheet(el('settings-sheet'), el('settings-btn')) state.showOnboarding = true refreshSplash() }) @@ -3926,42 +3927,39 @@ window.addEventListener('DOMContentLoaded', async () => { }) el('filter-pill').addEventListener('click', () => { const sheet = el('filter-sheet') - sheet.hidden = !sheet.hidden - if (!sheet.hidden) { - el('settings-sheet').hidden = true - el('target-sheet').hidden = true - renderIgnoreList(el('ss-ignore-list')) - } + if (!sheet.hidden) { closeSheet(sheet, el('filter-pill')); return } + sheet.hidden = false + closeSheet(el('settings-sheet'), el('settings-btn')) + closeSheet(el('target-sheet'), el('target-chip')) + renderIgnoreList(el('ss-ignore-list')) }) el('settings-btn').addEventListener('click', () => { const sheet = el('settings-sheet') - sheet.hidden = !sheet.hidden - if (!sheet.hidden) { - el('filter-sheet').hidden = true - el('target-sheet').hidden = true - // Always open on the tabs, not on wherever the brokers page was left. - state.brokerSheet.close() - refreshConnState() - refreshAccount() - checkForUpdate() - refreshWhatsNewBadge() - // Before the badge refresh this would read the flag the previous open - // left behind; after it, state.unseenChangelog is the current answer. - settingsSelectTab(initialSettingsTab(state)) - } + if (!sheet.hidden) { closeSheet(sheet, el('settings-btn')); return } + sheet.hidden = false + closeSheet(el('filter-sheet'), el('filter-pill')) + closeSheet(el('target-sheet'), el('target-chip')) + // Always open on the tabs, not on wherever the brokers page was left. + state.brokerSheet.close() + refreshConnState() + refreshAccount() + checkForUpdate() + refreshWhatsNewBadge() + // Before the badge refresh this would read the flag the previous open + // left behind; after it, state.unseenChangelog is the current answer. + settingsSelectTab(initialSettingsTab(state)) }) // Target chip tap → open the target dropdown el('target-chip').addEventListener('click', () => { const sheet = el('target-sheet') - sheet.hidden = !sheet.hidden - if (!sheet.hidden) { - el('filter-sheet').hidden = true - el('settings-sheet').hidden = true - el('ts-clear').hidden = !state.filter.sender - state.targetList.reset() - } + if (!sheet.hidden) { closeSheet(sheet, el('target-chip')); return } + sheet.hidden = false + closeSheet(el('filter-sheet'), el('filter-pill')) + closeSheet(el('settings-sheet'), el('settings-btn')) + el('ts-clear').hidden = !state.filter.sender + state.targetList.open() }) // Tap outside an open sheet (on the map/backdrop) closes it — standard @@ -3982,7 +3980,7 @@ window.addEventListener('DOMContentLoaded', async () => { for (const { sheet, toggle } of dismissableSheets) { if (sheet.hidden) continue if (sheet.contains(e.target) || toggle.contains(e.target)) continue - sheet.hidden = true + closeSheet(sheet, toggle) } syncPopoverTriggers() }) diff --git a/app/src/sheetfocus.js b/app/src/sheetfocus.js new file mode 100644 index 00000000..3df87cbc --- /dev/null +++ b/app/src/sheetfocus.js @@ -0,0 +1,10 @@ +// closeSheet hides a bottom sheet, and when the focus was inside it hands +// the focus back to the sheet's toggle (#714), as web/multiselect.js does: +// not left on a hidden field, with the caret and on a phone the keyboard, +// and not dropped on the body, where a keyboard user loses their place. +// Every path that hides a sheet goes through here: its close button, its +// toggle, another sheet opening, a tap outside. +export function closeSheet(sheet, toggle, doc = document) { + if (sheet.contains(doc.activeElement)) toggle.focus() + sheet.hidden = true +} diff --git a/app/src/targetlist.js b/app/src/targetlist.js index a254a745..97994bd8 100644 --- a/app/src/targetlist.js +++ b/app/src/targetlist.js @@ -117,13 +117,28 @@ export function createTargetList(listEl, { onSelect, pinnedEl, pinnedLabelEl, se listEl.replaceChildren(...items.map((rec) => row(rec, nowMs, onSelect, lastSelected))) } - // Reset back to the first page — call when the sheet is (re)opened. + // Back to the first page. function reset() { visible = PAGE_SIZE _lastSig = null _lastPinnedSig = null } + // open: what the sheet does each time it opens (#714). The first page of + // the full list, an empty query, and the caret in the field, so the next + // keystroke searches. A query left from the last open used to come back + // with a list narrowed by it and nothing saying why. The list repaints at + // once when there was a query, not on the next tick, so the narrowed list + // never shows. The focus has to land in the tap that opened the sheet: a + // phone raises its keyboard only then. + function open() { + const hadQuery = !!(searchEl && searchEl.value) + if (searchEl) searchEl.value = '' + reset() + if (hadQuery) render(lastRows, lastIgnore, Date.now(), lastSelected) + if (searchEl) searchEl.focus({ preventScroll: true }) + } + // Typing is a new list: page from the top of the matches rather than from // wherever the unfiltered list had been scrolled to. if (searchEl) { @@ -144,5 +159,5 @@ export function createTargetList(listEl, { onSelect, pinnedEl, pinnedLabelEl, se render(lastRows, lastIgnore, Date.now(), lastSelected) }) - return { render, reset } + return { render, open } } diff --git a/changelog.d/2026-09-27-07-picker-search-focus.json b/changelog.d/2026-09-27-07-picker-search-focus.json new file mode 100644 index 00000000..323a8701 --- /dev/null +++ b/changelog.d/2026-09-27-07-picker-search-focus.json @@ -0,0 +1,7 @@ +{ + "id": "2026-09-27-picker-search-focus", + "date": "2026-09-27", + "where": "both", + "title": "Type straight away in the target picker", + "body": "Opening the target picker puts the cursor in its search. In the app it opens on an empty search every time." +} diff --git a/web/changelog.json b/web/changelog.json index b2ca7c2a..e80778f8 100644 --- a/web/changelog.json +++ b/web/changelog.json @@ -20,6 +20,13 @@ "title": "A larger audio buffer", "body": "Sound uses a larger output buffer, against ticking while the app is busy right after start." }, + { + "id": "2026-09-27-picker-search-focus", + "date": "2026-09-27", + "where": "both", + "title": "Type straight away in the target picker", + "body": "Opening the target picker puts the cursor in its search. In the app it opens on an empty search every time." + }, { "id": "2026-09-26-broker-presets-shown", "date": "2026-09-26", diff --git a/web/e2e/targetpicker.spec.js b/web/e2e/targetpicker.spec.js index e2bc122b..9cd0a03d 100644 --- a/web/e2e/targetpicker.spec.js +++ b/web/e2e/targetpicker.spec.js @@ -22,6 +22,24 @@ test('opening the picker lists senders from the currently loaded points', async await expect(page.locator('#tp-list')).toContainText('Charlie') }) +// #714: the caret is in the search the moment the picker opens, so the next +// keystroke searches without a press on the field; Escape hands the focus back +// to the toggle rather than leaving it on a hidden field. +test('opening the picker puts the caret in its search, and closing hands it back', async ({ page }) => { + await page.route('**/api/points*', (r) => r.fulfill({ json: { points: [A, B] } })) + await page.goto('/?mode=points') + await openPicker(page, '#sp-toggle', '#sender-picker') + await expect(page.locator('#f-sender')).toBeFocused() + await page.keyboard.type('cc') + await expect(page.locator('#f-sender')).toHaveValue('cc') + await page.keyboard.press('Escape') + await expect(page.locator('#sender-picker')).toBeHidden() + await expect(page.locator('#sp-toggle')).toBeFocused() + // Escape in a search field clears it in Chromium and WebKit; here it only + // closes, so the typed prefix, which filters the map, stays. + await expect(page.locator('#f-sender')).toHaveValue('cc') +}) + const sendersOf = (u) => new URL(u).searchParams.getAll('senders') test('a picked selection is sent to the server as repeated senders= params', async ({ page }) => { diff --git a/web/map.js b/web/map.js index 9cb6a87b..de7c0eee 100644 --- a/web/map.js +++ b/web/map.js @@ -2490,6 +2490,9 @@ syncTargetToggleLabel() wirePopover({ toggleEl: spToggle, panelEl: senderPicker, wrapEl: spToggle.closest('.ms-wrap'), wrapSelector: '.ms-wrap', onOpen: () => { targetPicker.reset(); refresh() }, // back to page 1; the next redraw repopulates + // The caret in the search (#714). Its value stays: on web it is the sender + // filter itself, bound to ?sender=. + focusEl: document.getElementById('f-sender'), }) // Hunter picker (#290): generalizes the sender picker's pattern to #f-hunter, diff --git a/web/multiselect.js b/web/multiselect.js index 0f6d113f..f9144152 100644 --- a/web/multiselect.js +++ b/web/multiselect.js @@ -221,13 +221,16 @@ export function placePopover(toggleEl, panelEl, { align = 'left', viewport } = { // can't tell them apart, so this takes the actual element and requires the // click's nearest wrapSelector ancestor to be THIS wrap, not merely any wrap. // onOpen lets a caller reset paging / refresh data each time the panel opens. +// focusEl (#714) is the field the caret goes to on open, so the next keystroke +// searches; closing with the focus inside hands it back to the toggle rather +// than leaving it on a hidden field. // // Click detection is capture-phase, not bubble: a row click's own handler // replaces the clicked button via listEl.replaceChildren() synchronously, so // by the time a bubble-phase document listener would run, e.target is already // detached and closest(wrapSelector) wrongly returns null, closing the panel // after every pick. Capture runs before that mutation happens. -export function wirePopover({ toggleEl, panelEl, wrapEl, wrapSelector, onOpen, align = 'left' }) { +export function wirePopover({ toggleEl, panelEl, wrapEl, wrapSelector, onOpen, focusEl, align = 'left' }) { function open() { panelEl.hidden = false toggleEl.setAttribute('aria-expanded', 'true') @@ -235,10 +238,15 @@ export function wirePopover({ toggleEl, panelEl, wrapEl, wrapSelector, onOpen, a // After onOpen: it repopulates the rows, so the panel's height is only // final once it has run (#372). placePopover(toggleEl, panelEl, { align }) + // In the same tap as the open, which a phone needs to raise its keyboard. + // preventScroll: the panel is placed already, the page must not jump. + if (focusEl) focusEl.focus({ preventScroll: true }) } function close() { + const inside = panelEl.contains(document.activeElement) panelEl.hidden = true toggleEl.setAttribute('aria-expanded', 'false') + if (inside) toggleEl.focus() } // #bar wraps, on a resize or when late content grows it, and that moves // the toggle to another row, so the panel has to follow (#405: the one bar @@ -253,6 +261,8 @@ export function wirePopover({ toggleEl, panelEl, wrapEl, wrapSelector, onOpen, a if (e.target.closest(wrapSelector) === wrapEl) return close() }, true) - document.addEventListener('keydown', (e) => { if (e.key === 'Escape' && !panelEl.hidden) close() }) + // preventDefault: with the caret in a search field (#714), the browser's own + // Escape would clear it, and a cleared search is a changed filter. + document.addEventListener('keydown', (e) => { if (e.key === 'Escape' && !panelEl.hidden) { e.preventDefault(); close() } }) return { open, close } } diff --git a/web/multiselect.test.js b/web/multiselect.test.js index 30967cfd..0805f185 100644 --- a/web/multiselect.test.js +++ b/web/multiselect.test.js @@ -1,5 +1,5 @@ -import { describe, it, expect } from 'vitest' -import { bulkAction } from './multiselect.js' +import { describe, it, expect, afterEach } from 'vitest' +import { bulkAction, wirePopover } from './multiselect.js' // #628. The control above a picker's list is one button, and its label is what // a tap does. The rest of the picker is DOM and lives in e2e/hunterpicker.spec.js. @@ -12,3 +12,59 @@ describe('bulkAction', () => { expect(bulkAction(30)).toEqual({ action: 'clear', label: 'Clear selection' }) }) }) + +// #714: opening the sender picker left nothing focused, so reaching a node by +// name took a press on the field between the toggle and the first keystroke. +// web/ has no jsdom; the elements are the fakes wirePopover touches. +describe('wirePopover puts the caret in the search field (#714)', () => { + const rect = { left: 0, top: 0, right: 100, bottom: 30, width: 100, height: 30 } + function fakes() { + const own = {}, doc = [] + globalThis.window = { innerWidth: 400, innerHeight: 800 } + globalThis.document = { activeElement: null, addEventListener: (type, fn) => doc.push({ type, fn }) } + const focus = (el) => (opts) => { el.focusedWith = opts || {}; globalThis.document.activeElement = el } + const toggleEl = { setAttribute() {}, addEventListener: (type, fn) => { own[type] = fn }, getBoundingClientRect: () => rect } + toggleEl.focus = focus(toggleEl) + const field = { value: 'ab12' } + field.focus = focus(field) + const panelEl = { hidden: true, style: {}, getBoundingClientRect: () => rect, contains: (el) => el === field } + const popover = wirePopover({ toggleEl, panelEl, wrapEl: {}, wrapSelector: '.ms-wrap', focusEl: field }) + const key = (k) => { + const e = { key: k, defaultPrevented: false, preventDefault() { this.defaultPrevented = true } } + doc.filter((l) => l.type === 'keydown').forEach((l) => l.fn(e)) + return e + } + return { toggleEl, panelEl, field, popover, press: () => own.click(), key } + } + afterEach(() => { delete globalThis.window; delete globalThis.document }) + + it('focuses the field on open, without scrolling the page, and keeps what it holds', () => { + const f = fakes() + f.press() + expect(f.panelEl.hidden).toBe(false) + expect(globalThis.document.activeElement).toBe(f.field) + expect(f.field.focusedWith).toEqual({ preventScroll: true }) + // On web the field is the sender filter itself, bound to ?sender=. + expect(f.field.value).toBe('ab12') + }) + + it('hands focus back to the toggle when it closes with the caret inside', () => { + const f = fakes() + f.press() + const e = f.key('Escape') + expect(f.panelEl.hidden).toBe(true) + expect(globalThis.document.activeElement).toBe(f.toggleEl) + // The browser's own Escape in a search field clears it: a changed filter. + expect(e.defaultPrevented).toBe(true) + expect(f.field.value).toBe('ab12') + }) + + it('leaves focus alone when it closes with the focus elsewhere', () => { + const f = fakes() + f.press() + const elsewhere = {} + globalThis.document.activeElement = elsewhere + f.press() + expect(globalThis.document.activeElement).toBe(elsewhere) + }) +})