From 7fc3737998746aa3aa89c355dc7e392e8f5624d8 Mon Sep 17 00:00:00 2001 From: Bernard <63512176+BernardJen@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:57:11 +0200 Subject: [PATCH] fix: dual-view dropdown columns overflowed the panel (#258 regression) Reported as the dropdown items not fitting in dual view. They did not, and the cause turned out to be two separate mistakes in #258 -- one layout, one worse than layout. **The overflow.** Adding a key chip to each input row meant giving the name `white-space: nowrap` so it would ellipsise rather than wrap. `.column-layout` uses `grid-template-columns: 1fr 1fr`, and `1fr` is `minmax(auto, 1fr)` whose auto minimum is the item's MIN-CONTENT size. With wrapping text that is the longest word; with nowrap it is the entire label. So the tracks computed to 339.758px each inside a 358px panel and the columns spilled roughly 320px out of the dropdown. That is the same trap as flex `min-width: auto`, one level up -- and the flex one was already fixed in this very file, on the name span, which is how the grid one got missed. Fixed with `minmax(0, 1fr)`. **The chip should not have been there at all.** `1`-`4` call selectInput() with the default side='both' and set BOTH feeds. Clicking a row in the Left column calls selectInputForSide(id, 'left') and sets one. So a chip on a per-side row documented a key that does something different from the control it sat next to. Chips now render on the single-view list only, where one feed is shown and setting both is the same thing to the operator as setting that one. That also resolves the fit on its own terms: a dual column is ~173px against the single list's ~358px, and a name plus a chip left about 98px for the name -- enough to turn "Blackmagic UltraStudio Recorder 3G" into "Blackmagic Ul...", which is useless for telling two capture cards apart. Name truncation is scoped to the single list with the chip. Dual-column names wrap again, exactly as they did before #258. Measured after the fix, dual view at a 400px panel: tracks 173px + 173px + 12px gap against a 358px container, no chips in the columns, every name fully visible with nothing truncated, panel inside the viewport. Single view keeps its 1-4 chips at 322px rows, where the long Blackmagic label is not even truncated. Two new tests, 557 -> 559, both pinning halves of the fix that would each have hidden the other: that the grid uses minmax(0, 1fr) and not a bare 1fr, and that nowrap is scoped to the list that has a chip. Verified by reverting each in turn, plus putting the chip back on the dual columns. --- .claude/CLAUDE.md | 22 ++++- .../src/renderer/renderer.js | 21 +++-- input_viewer_electron/src/renderer/styles.css | 13 ++- .../test/shortcut-hints.test.js | 87 +++++++++++++++---- 4 files changed, 117 insertions(+), 26 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 8439fa0..4226b0d 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -96,7 +96,7 @@ Four consumers read from it, and none of them keeps its own copy: | Consumer | How | |---|---| | the keydown handler | `SHORTCUTS_BY_KEY.get(event.key.toLowerCase())`, then `SHORTCUT_ACTIONS[id]` | -| the dropdown | `renderShortcutHints()` labels Dual/Single; the input rows get `inputKeyFor(index)` | +| the dropdown | `renderShortcutHints()` labels Dual/Single; the **single-view** input rows get `inputKeyFor(index)` | | the Settings table | `renderShortcutHints()` fills `#shortcuts-table`, which ships empty | | `README.md` and `docs/USER_GUIDE.md` | still hand-written, but a test asserts every chip appears in both | @@ -122,6 +122,26 @@ Past the fourth input row `inputKeyFor()` returns null and no chip is drawn. The wall can have more capture devices than there are number keys, and labelling a fifth row `5` would promise a binding that does not exist. +**The dual columns carry no chip, and that is about correctness, not space.** +`1`-`4` call `selectInput()` with the default `side='both'` and set BOTH feeds; +clicking a row in the Left column calls `selectInputForSide(id, 'left')` and sets +one. A chip on a per-side row documents a key that does something different from +the control beside it. In single view one feed is shown, so setting both and +setting that one are the same thing to the operator, and the chip is honest. + +It was reported as a fit bug in dual view, and it was that as well. Two things had +to be fixed: + +- `.column-layout` needed `minmax(0, 1fr)`, not `1fr`. A bare `1fr` is + `minmax(auto, 1fr)` and the auto minimum is the item's **min-content** size, so a + `white-space: nowrap` name pinned the tracks open: they computed to 339.758px + each inside a 358px panel and spilled ~320px out of the dropdown. Same trap as + flex `min-width: auto`, one level up — and the flex one was already fixed in this + file, which is how the grid one got missed. +- Name truncation is scoped to `.single-input-option .input-option-name`, the only + list with a chip. A ~173px column has no room to both truncate and stay + readable, so dual-column names wrap as they did before the chips existed. + ### The test-mode launch flags (#248) | Flag | Effect | diff --git a/input_viewer_electron/src/renderer/renderer.js b/input_viewer_electron/src/renderer/renderer.js index 328d971..326a312 100644 --- a/input_viewer_electron/src/renderer/renderer.js +++ b/input_viewer_electron/src/renderer/renderer.js @@ -1535,14 +1535,25 @@ function renderDropdownInputLists() { // Name via textContent, never innerHTML: this string is a device label from // capture hardware or a user-entered rename. - const buildOption = (className, isActive, side) => { + // + // `showKey` is false for the dual columns, and that is a correctness point + // rather than a layout one. `1`-`4` call selectInput() with the default + // side='both', which sets BOTH feeds; clicking a row in the Left column calls + // selectInputForSide(id, 'left') and sets one. A chip on a per-side row would + // therefore document a key that does something different from the control it + // sits next to. In single view only one feed is shown, so setting both and + // setting that one are the same thing to the operator, and the chip is honest. + // + // It also fixes the fit: a dual column is ~173px against the single list's + // ~358px, and a name plus a chip left about 98px for the name. + const buildOption = (className, isActive, side, showKey) => { const option = document.createElement('div') option.className = `${className}${isActive ? ' selected' : ''}` const name = document.createElement('span') name.className = 'input-option-name' name.textContent = customName option.appendChild(name) - if (key) option.appendChild(shortcutKeyChip(key)) + if (key && showKey) option.appendChild(shortcutKeyChip(key)) option.addEventListener('click', () => { selectInputForSide(device.deviceId, side) }) @@ -1550,11 +1561,11 @@ function renderDropdownInputLists() { } elements.leftInputList.appendChild( - buildOption('input-option', isLeftActive, 'left')) + buildOption('input-option', isLeftActive, 'left', false)) elements.rightInputList.appendChild( - buildOption('input-option', isRightActive, 'right')) + buildOption('input-option', isRightActive, 'right', false)) elements.singleInputList.appendChild( - buildOption('single-input-option', isLeftActive, 'left')) + buildOption('single-input-option', isLeftActive, 'left', true)) }) } diff --git a/input_viewer_electron/src/renderer/styles.css b/input_viewer_electron/src/renderer/styles.css index 48a64a6..fcc4c08 100644 --- a/input_viewer_electron/src/renderer/styles.css +++ b/input_viewer_electron/src/renderer/styles.css @@ -813,7 +813,12 @@ kbd { .column-layout { display: grid; - grid-template-columns: 1fr 1fr; + /* minmax(0, 1fr), not 1fr. `1fr` is minmax(auto, 1fr) and the auto minimum is + the item's min-content size, so any descendant that cannot wrap pins the + track open. A `white-space: nowrap` device name did exactly that: the tracks + computed to 339.758px each inside a 358px panel and the columns overflowed + the dropdown by ~320px. Same trap as flex `min-width: auto`, one level up. */ + grid-template-columns: minmax(0, 1fr) minmax(0, 1fr); gap: 12px; margin-bottom: 16px; } @@ -1230,7 +1235,11 @@ kbd { /* The name takes the room the chip does not, so a long device label truncates instead of pushing the chip out of the row. */ -.input-option-name, +/* Truncation applies only where a chip shares the row -- the single-view list. + The dual columns carry no chip (see buildOption in renderer.js: the number keys + set both feeds, so a per-side row must not advertise one), so their names wrap + as they did before the chips existed. At ~173px a column has no room to both + truncate and stay readable. */ .single-input-option .input-option-name { /* min-width:0 is what actually lets a flex item shrink below its content width; without it the default min-width:auto keeps the name at full size and diff --git a/input_viewer_electron/test/shortcut-hints.test.js b/input_viewer_electron/test/shortcut-hints.test.js index 24aaaf1..a8a148e 100644 --- a/input_viewer_electron/test/shortcut-hints.test.js +++ b/input_viewer_electron/test/shortcut-hints.test.js @@ -10,7 +10,9 @@ * table can never again be a hand-maintained copy that drifts. */ import { describe, it, expect, beforeEach, vi } from 'vitest' -import { installRendererDom, device } from './helpers/renderer-dom.js' +import { readFileSync } from 'node:fs' +import path from 'node:path' +import { installRendererDom, device, projectRoot } from './helpers/renderer-dom.js' installRendererDom() @@ -122,34 +124,46 @@ describe('the view-mode buttons', () => { }) describe('the dropdown input rows', () => { - it('labels the first four rows with 1-4', () => { + // Chips go on the SINGLE-view list only. + // + // Not a layout compromise -- a correctness one. `1`-`4` call selectInput() with + // the default side='both' and set BOTH feeds; clicking a row in the Left column + // calls selectInputForSide(id, 'left') and sets one. A chip on a per-side row + // would document a key that does something different from the control beside it. + // In single view one feed is shown, so setting both and setting that one are the + // same thing to the operator. + // + // It was reported as a fit bug in dual view, and it was that too: a dual column + // is ~173px against the single list's ~358px. + + it('labels the first four rows of the single-view list with 1-4', () => { reset([device('a', 'Cam A'), device('b', 'Cam B'), device('c', 'Cam C'), device('d', 'Cam D')]) renderDropdownInputLists() - const chips = [...elements.leftInputList.querySelectorAll('kbd')] + const chips = [...elements.singleInputList.querySelectorAll('kbd')] .map(k => k.textContent) expect(chips).toEqual(['1', '2', '3', '4']) }) + it('puts no chip on the dual columns, where the key means something else', () => { + reset([device('a', 'Cam A'), device('b', 'Cam B')]) + renderDropdownInputLists() + expect(elements.leftInputList.querySelectorAll('kbd')).toHaveLength(0) + expect(elements.rightInputList.querySelectorAll('kbd')).toHaveLength(0) + // The rows themselves are still there and still named. + expect([...elements.leftInputList.children].map(r => r.textContent)) + .toEqual(['Cam A', 'Cam B']) + }) + it('leaves a fifth row unlabelled rather than promising a key', () => { reset(['a', 'b', 'c', 'd', 'e'].map(id => device(id, `Cam ${id}`))) renderDropdownInputLists() - const rows = [...elements.leftInputList.children] + const rows = [...elements.singleInputList.children] expect(rows).toHaveLength(5) expect(rows[4].querySelector('kbd')).toBeNull() expect(rows[3].querySelector('kbd').textContent).toBe('4') }) - it('labels all three lists, including single view', () => { - reset([device('a', 'Cam A'), device('b', 'Cam B')]) - renderDropdownInputLists() - for (const list of [elements.leftInputList, elements.rightInputList, - elements.singleInputList]) { - expect([...list.querySelectorAll('kbd')].map(k => k.textContent)) - .toEqual(['1', '2']) - } - }) - it('skips disabled inputs, so the numbering matches what is shown', () => { // selectInput indexes the enabled list, so a hidden disabled device must not // consume a number. @@ -160,7 +174,7 @@ describe('the dropdown input rows', () => { state.devices = [device('a', 'Cam A'), device('b', 'Cam B'), device('c', 'Cam C')] renderDropdownInputLists() - const rows = [...elements.leftInputList.children] + const rows = [...elements.singleInputList.children] expect(rows).toHaveLength(2) expect(rows.map(r => r.querySelector('kbd').textContent)).toEqual(['1', '2']) expect(rows[1].textContent).toContain('Cam C') @@ -171,9 +185,46 @@ describe('the dropdown input rows', () => { // elements with textContent for exactly this reason. reset([device('a', '')]) renderDropdownInputLists() - const row = elements.leftInputList.children[0] - expect(row.querySelector('img')).toBeNull() - expect(row.textContent).toContain('') + for (const list of [elements.leftInputList, elements.singleInputList]) { + const row = list.children[0] + expect(row.querySelector('img')).toBeNull() + expect(row.textContent).toContain('') + } + }) +}) + +describe('the dual columns cannot overflow the dropdown', () => { + // The reported bug: with a chip forcing `white-space: nowrap` on the name, the + // grid's `1fr` tracks -- which are minmax(auto, 1fr), and whose auto minimum is + // the item's MIN-CONTENT size -- computed to 339.758px each inside a 358px + // panel. The columns spilled ~320px out of the dropdown. + // + // jsdom does no layout, so this asserts the declaration rather than measuring. + // Both halves of the fix are pinned, because either alone would have hidden it. + const CSS = readFileSync( + path.resolve(projectRoot, 'src/renderer/styles.css'), 'utf8') + + const ruleBody = (selector) => { + const re = new RegExp( + `${selector.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*\\{([^}]*)\\}`, 'g') + const bodies = [...CSS.matchAll(re)].map(m => m[1]) + expect(bodies.length, `rule not found: ${selector}`).toBeGreaterThan(0) + return bodies.join('\n') + } + + it('uses minmax(0, 1fr) so a track can shrink below its content', () => { + const body = ruleBody('.column-layout') + expect(body).toMatch(/grid-template-columns:\s*minmax\(\s*0\s*,\s*1fr\s*\)/) + // A bare `1fr` is the bug. + expect(body).not.toMatch(/grid-template-columns:\s*1fr\s+1fr/) + }) + + it('scopes name truncation to the list that actually has a chip', () => { + // Applying nowrap to the dual columns is what pinned the tracks open. Their + // names wrap, as they did before the chips existed. + expect(ruleBody('.single-input-option .input-option-name')) + .toMatch(/white-space:\s*nowrap/) + expect(CSS).not.toMatch(/^\.input-option-name,/m) }) })