fix: reach the settings while a book is open

The panel was part of the library screen, and the library screen is
unmounted while you are reading. Ctrl+, went nowhere, and the only way
to change a setting was to leave the book first.

It belongs to the application shell, which is always there, so it now
opens over whichever screen is showing and closes back onto it. The
screens underneath stop reading the keyboard while it is up: an arrow
key meant for the panel was turning the page behind it.

Ctrl+F is now claimed by whichever screen is on, so it searches the
book while you are reading one instead of doing nothing; the menu item
says "Search" rather than "Search Library". The reader also stops
listening for it when it closes — the shelf's own listener was being
added again on every return from a book and never removed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

README.md

@@ -53,7 +53,8 @@ gh attestation verify <file> --repo chmouel/liseur-desktop
 
 There isn't one, until you ask: press Alt and it appears. Everything in it
 has a shortcut anyway — `Ctrl+O` to open a book, `Ctrl+Shift+O` to add a
-folder, `Ctrl+,` for the settings, `Ctrl+F` to search. On macOS the menu
+folder, `Ctrl+,` for the settings, `Ctrl+F` to search the shelf or the book you
+are in. On macOS the menu
 lives in the system bar as usual.
 
 ## Vim mode

docs/DESIGN.md

@@ -44,6 +44,11 @@ Library UX conventions carried over from Android:
 - Continue Reading card: cover + label + title + author + progress bar + percent
 - Book card: cover + badges (download/server/finished) + title + author
 
+The settings panel belongs to the application shell rather than to the
+library, and paints over whichever screen is showing. Anything scoped to one
+screen — the shelf's search, the reader's panels — stays with that screen and
+claims the menu accelerator only while it is mounted.
+
 ## Vim mode
 
 An optional keyboard grammar for people who already have one in their

src/main/menu.ts

@@ -39,7 +39,8 @@ export function setupMenu(): void {
       label: 'View',
       submenu: [
         {
-          label: 'Search Library',
+          // The shelf or the book, whichever is on screen.
+          label: 'Search',
           accelerator: 'CmdOrCtrl+F',
           click: () => send('liseur:menu:search'),
         },

src/renderer/app/App.tsx

@@ -1,8 +1,10 @@
-import { createSignal, onMount, Show, type JSX } from 'solid-js'
+import { createSignal, onCleanup, onMount, Show, type JSX } from 'solid-js'
 import { initTheme } from './theme'
 import { initLibrary, useLibraryStore } from '../library/store'
 import { LibraryScreen } from '../library/LibraryScreen'
 import { ReaderScreen } from '../reader/ReaderScreen'
+import { SettingsScreen } from '../settings/SettingsScreen'
+import { closeSettings, openSettings, settingsOpen } from '../settings/state'
 import { initVimMode } from '../vim/vim'
 import { observeLongTasks, perf } from '../perf/perf'
 
@@ -55,23 +57,32 @@ export function App(): JSX.Element {
     initVimMode()
     observeLongTasks()
     watchForNewServerBooks()
+    // Settings belong to the application, not to the shelf: Ctrl+, has to
+    // answer while a book is open too.
+    onCleanup(window.liseur.app.onMenu((action) => action === 'settings' && openSettings()))
     void maybeResumeLastBook(setOpenBookId)
     done()
   })
 
   return (
-    <Show when={openBookId()} fallback={<LibraryScreen onOpenBook={(id) => setOpenBookId(id)} />}>
-      {(bookId) => (
-        <ReaderScreen
-          bookId={bookId()}
-          onClose={() => {
-            setOpenBookId(null)
-            // Progress changed while reading: refresh what the user sees.
-            void library.refresh()
-            void library.refreshContinueReading()
-          }}
-        />
-      )}
-    </Show>
+    <>
+      <Show when={openBookId()} fallback={<LibraryScreen onOpenBook={(id) => setOpenBookId(id)} />}>
+        {(bookId) => (
+          <ReaderScreen
+            bookId={bookId()}
+            onClose={() => {
+              setOpenBookId(null)
+              // Progress changed while reading: refresh what the user sees.
+              void library.refresh()
+              void library.refreshContinueReading()
+            }}
+          />
+        )}
+      </Show>
+      {/* Above whichever screen is showing, so it opens over a book too. */}
+      <Show when={settingsOpen()}>
+        <SettingsScreen onClose={closeSettings} />
+      </Show>
+    </>
   )
 }

src/renderer/library/LibraryScreen.tsx

@@ -3,7 +3,7 @@ import type { LibraryFilter, LibrarySortKey } from '@shared/domain/types'
 import { useLibraryStore } from './store'
 import { VirtualBookGrid } from './VirtualBookGrid'
 import { ContinueReading } from './ContinueReading'
-import { SettingsScreen } from '../settings/SettingsScreen'
+import { openSettings, settingsOpen } from '../settings/state'
 import { StatsScreen } from '../stats/StatsScreen'
 import { useTheme } from '../app/theme'
 import { computeColumns, GRID_CARD_WIDTH, GRID_GAP, GRID_ROW_HEIGHT } from './virtualize'
@@ -26,7 +26,6 @@ export function LibraryScreen(props: { onOpenBook: (bookId: string) => void }):
 
   const [searchOpen, setSearchOpen] = createSignal(false)
   const [selectedIndex, setSelectedIndex] = createSignal(-1)
-  const [settingsOpen, setSettingsOpen] = createSignal(false)
   const [statsOpen, setStatsOpen] = createSignal(false)
   const [sortMenuOpen, setSortMenuOpen] = createSignal(false)
   const [helpOpen, setHelpOpen] = createSignal(false)
@@ -88,10 +87,9 @@ export function LibraryScreen(props: { onOpenBook: (bookId: string) => void }):
       }
       return
     }
-    if (settingsOpen()) {
-      if (e.key === 'Escape' || (vimMode() && e.key === 'q')) setSettingsOpen(false)
-      return
-    }
+    // The settings panel is the application's, not the shelf's, and it
+    // handles its own keys — including the ones that close it.
+    if (settingsOpen()) return
     if (statsOpen()) {
       if (e.key === 'Escape' || (vimMode() && e.key === 'q')) setStatsOpen(false)
       return
@@ -244,7 +242,7 @@ export function LibraryScreen(props: { onOpenBook: (bookId: string) => void }):
         setStatsOpen(true)
         break
       case 'settings':
-        setSettingsOpen(true)
+        openSettings()
         break
       case 'help':
         setHelpOpen(true)
@@ -265,11 +263,11 @@ export function LibraryScreen(props: { onOpenBook: (bookId: string) => void }):
     props.onOpenBook(book.id)
   }
 
-  // Menu accelerators from main (Ctrl/Cmd+F, Ctrl/Cmd+,).
-  window.liseur.app.onMenu((action) => {
-    if (action === 'search') openSearch()
-    if (action === 'settings') setSettingsOpen(true)
-  })
+  // Ctrl/Cmd+F searches whatever is on screen, so each screen claims it for
+  // itself; settings are the application's and are handled in the shell.
+  // Unsubscribing matters: this screen is mounted again every time a book is
+  // closed, and the listeners would otherwise pile up.
+  onCleanup(window.liseur.app.onMenu((action) => action === 'search' && openSearch()))
 
   // Document-level key handling: Solid delegates element handlers, and keys
   // targeted at <body> (e.g. after a form closes) never pass through the
@@ -373,17 +371,13 @@ export function LibraryScreen(props: { onOpenBook: (bookId: string) => void }):
             class="icon-button"
             aria-label="Settings"
             title="Settings (Ctrl/Cmd+,)"
-            onClick={() => setSettingsOpen(true)}
+            onClick={openSettings}
           >
             ⋮
           </button>
         </div>
       </header>
 
-      <Show when={settingsOpen()}>
-        <SettingsScreen onClose={() => setSettingsOpen(false)} />
-      </Show>
-
       <Show when={statsOpen()}>
         <StatsScreen onClose={() => setStatsOpen(false)} />
       </Show>

src/renderer/reader/ReaderScreen.tsx

@@ -28,6 +28,7 @@ import {
   MAX_MEASURE,
   type MarginPreset,
 } from './reader-theme'
+import { settingsOpen } from '../settings/state'
 
 /** Margin presets in the order they read: most text to least. */
 const MARGIN_NAMES = ['narrow', 'normal', 'wide'] as const
@@ -683,6 +684,11 @@ export function ReaderScreen(props: { bookId: string; onClose: () => void }): JS
   function onKeydown(e: ForwardedKeyEvent): void {
     // Any key press reveals the chrome (hidden-reading mode is pointer- AND
     // keyboard-dismissable).
+    // The settings panel opens over the book. While it is up it owns the
+    // keyboard: turning pages or closing the book underneath it is never
+    // what the keystroke was for.
+    if (settingsOpen()) return
+
     showChrome()
 
     if (helpOpen()) {
@@ -935,6 +941,11 @@ export function ReaderScreen(props: { bookId: string; onClose: () => void }): JS
     })()
   }
 
+  // Ctrl/Cmd+F means "search what I am looking at", and what you are looking
+  // at here is a book. The shelf claims the same accelerator while it is the
+  // one on screen; only one of the two is ever mounted.
+  onCleanup(window.liseur.app.onMenu((action) => action === 'search' && togglePopover('search')))
+
   // Key handling at document level: clicks land on iframes/zones and must
   // not swallow page turns. One listener only (registered here, not on the
   // root div — both would fire for the same event).

src/renderer/settings/SettingsScreen.tsx

@@ -45,7 +45,24 @@ export function SettingsScreen(props: { onClose: () => void }): JSX.Element {
     }
   }
 
+  // The panel owns the keyboard while it is up, whatever is behind it. Both
+  // screens listen for keys on the document, and a page must not turn — nor a
+  // book close — under an open settings panel. Escape closes it from inside a
+  // field too, which a ticked checkbox used to make impossible.
+  function onKeydown(e: KeyboardEvent): void {
+    const target = e.target as HTMLElement | null
+    const inField =
+      target?.tagName === 'INPUT' || target?.tagName === 'TEXTAREA' || target?.tagName === 'SELECT'
+    if (e.key === 'Escape' || (vimMode() && e.key === 'q' && !inField)) {
+      e.preventDefault()
+      e.stopPropagation()
+      props.onClose()
+    }
+  }
+
   onMount(() => {
+    document.addEventListener('keydown', onKeydown)
+    onCleanup(() => document.removeEventListener('keydown', onKeydown))
     void refresh()
     const off = window.liseur.sync.onStateChanged((s) => setState(s))
     onCleanup(off)
@@ -154,18 +171,7 @@ export function SettingsScreen(props: { onClose: () => void }): JSX.Element {
   }
 
   return (
-    <div
-      class="settings-overlay"
-      role="dialog"
-      aria-label="Settings"
-      // Escape closes the panel from inside it too. The library's key
-      // handler deliberately keeps its hands off anything focused in a
-      // field, which used to leave a ticked checkbox holding the keyboard
-      // with no way out but the mouse.
-      onKeyDown={(e) => {
-        if (e.key === 'Escape') props.onClose()
-      }}
-    >
+    <div class="settings-overlay" role="dialog" aria-label="Settings">
       <div class="settings-panel">
         <header class="settings-header">
           <h1>Settings</h1>

src/renderer/settings/state.ts

@@ -0,0 +1,24 @@
+import { createSignal } from 'solid-js'
+
+/**
+ * Whether the settings panel is up.
+ *
+ * It belongs to the application rather than to the shelf. It used to live
+ * inside the library screen, which is unmounted while a book is open — so
+ * Ctrl+, did nothing at all once you started reading, and the panel could
+ * only be reached by leaving the book first.
+ *
+ * The screens underneath read this too: both listen for keys on the
+ * document, and a page must not turn behind a settings panel.
+ */
+const [settingsOpen, setSettingsOpen] = createSignal(false)
+
+export { settingsOpen }
+
+export function openSettings(): void {
+  setSettingsOpen(true)
+}
+
+export function closeSettings(): void {
+  setSettingsOpen(false)
+}

tests/e2e/settings.spec.ts

@@ -0,0 +1,91 @@
+import { test, expect, type ElectronApplication, type Page } from '@playwright/test'
+import { rmSync } from 'node:fs'
+import { buildReaderEpub } from '../unit/epub-fixture'
+import { installBook, launchApp, makeTempDirs, openBookByTitle } from './helpers'
+
+/**
+ * Settings belong to the application, not to the shelf.
+ *
+ * They used to be part of the library screen, which is unmounted while a
+ * book is open: Ctrl+, did nothing at all once you started reading, and the
+ * only way to change anything was to leave the book first.
+ */
+
+let app: ElectronApplication
+let page: Page
+let dataDir: string
+let booksDir: string
+
+test.beforeAll(async () => {
+  ;({ dataDir, booksDir } = makeTempDirs())
+  await installBook(dataDir, booksDir, 'reader.epub', buildReaderEpub())
+  ;({ app, page } = await launchApp(dataDir))
+})
+
+test.afterAll(async () => {
+  await app.close()
+  rmSync(dataDir, { recursive: true, force: true })
+  rmSync(booksDir, { recursive: true, force: true })
+})
+
+const dialog = (): ReturnType<Page['getByRole']> => page.getByRole('dialog', { name: 'Settings' })
+
+test('the settings panel opens over the shelf and closes with Escape', async () => {
+  await page.getByRole('button', { name: 'Settings' }).click()
+  await expect(dialog()).toBeVisible()
+  await page.keyboard.press('Escape')
+  await expect(dialog()).not.toBeVisible()
+})
+
+test('the settings shortcut answers while a book is open', async () => {
+  await openBookByTitle(page, 'Reader Fixture')
+  await expect(page.locator('.reader-screen')).toBeVisible()
+
+  // What the menu item does, without going through a menu the window no
+  // longer shows: main sends this on Ctrl+, and the shell has to hear it
+  // wherever the reader happens to be.
+  await app.evaluate(({ BrowserWindow }) => {
+    BrowserWindow.getAllWindows()[0]!.webContents.send('liseur:menu:settings')
+  })
+  await expect(dialog()).toBeVisible()
+
+  // The book is still there behind it, not closed and not left behind.
+  await expect(page.locator('.reader-screen')).toBeVisible()
+})
+
+test('the book underneath does not read the keyboard through the panel', async () => {
+  const iframe = page.frameLocator('.reader-iframe')
+  await expect(iframe.locator('h1')).toHaveText('Chapter 1')
+  const footer = await page.locator('.reader-footer').textContent()
+
+  // Arrows turn pages in the reader and move between fields in a form. With
+  // the panel up they belong to the panel. The footer says which page of
+  // which chapter is showing, so it moves on any turn, not only on one that
+  // crosses a chapter.
+  for (let i = 0; i < 4; i++) await page.keyboard.press('ArrowRight')
+  await page.waitForTimeout(300)
+  expect(await page.locator('.reader-footer').textContent()).toBe(footer)
+  await expect(iframe.locator('h1')).toHaveText('Chapter 1')
+
+  // Escape closes the panel and stops there: it must not also close the book.
+  await page.keyboard.press('Escape')
+  await expect(dialog()).not.toBeVisible()
+  await expect(page.locator('.reader-screen')).toBeVisible()
+})
+
+test('settings changed in a book are still there on the shelf', async () => {
+  await app.evaluate(({ BrowserWindow }) => {
+    BrowserWindow.getAllWindows()[0]!.webContents.send('liseur:menu:settings')
+  })
+  const vim = dialog().getByLabel('Vim keys in the library and the reader')
+  await vim.check()
+  await page.keyboard.press('Escape')
+
+  await page.keyboard.press('Escape') // leave the book
+  await expect(page.locator('.library-screen')).toBeVisible()
+
+  await page.getByRole('button', { name: 'Settings' }).click()
+  await expect(dialog().getByLabel('Vim keys in the library and the reader')).toBeChecked()
+  await page.keyboard.press('Escape')
+  await expect(dialog()).not.toBeVisible()
+})