rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

ui: name the directory fields, announce their help text, let long prefixes shrink

Browse source 
Greptile's three findings on #590, all confirmed real before fixing, and each one
verified red against the original code afterwards.

P1, the directory fields had no accessible names.  All five rendered their
visible label as a bare <label> with no htmlFor while passing no label prop, so
their inputs had a description ("/media/wrolpi/") and no name at all -- five
fields in one grid, holding similar relative paths, indistinguishable to a
screen reader.  The labels now belong to the inputs, InfoPopup content and all.
This predates PathInput; the old TextInput call sites had the same gap, and
moving to a component that supports labels is what made it fixable.

P1, description and error were rendered but never referenced.  aria-describedby
pointed only at the prefix, so the help text listing the variables a path
accepts, and the reason a path was rejected, were both invisible to assistive
technology.  Both are now in the description, and an errored field reports
aria-invalid.

P2 was worse than reported: at `flex: none` a long prefix took the entire row
and left the input eighteen pixels wide -- not "consumes the editable area" but
a field nobody can type in.  The media directory is whatever the API reports, so
its length was never ours to assume.  The prefix now shrinks and ellipsises at
half the control, with the full path on `title` and in the accessible
description.

The naming fault could not have been caught where the others were.  jsdom cannot
see layout, and the Settings page needs enough mocking that it had no test at
all -- which is why these five fields were uncovered when they broke.  The new
e2e spec intercepts the API and lets the real page render, so it measures the
names and the geometry a user actually gets.  Reverting one field to a bare
label fails it; a long prefix fails the browser spec at 18px.

962 jest, 15 browser component, 46 e2e, clean cache-busted build, tsc clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-07-30 15:46:54 -06:00 • committed by lrnselfreliance
parent c100ffb378
commit 510b24c298
7 changed files with 262 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

117
app/cypress/e2e/settings/special-directories.cy.js Normal file
View file

@ -0,0 +1,117 @@
/*
* The five Special Directories fields on /admin/settings.
*
* These are the fields that shipped printing their own prefix over their own value, and the
* ones whose visible labels were never associated with their inputs. Both faults were
* invisible to the jest suite: the overlap because jsdom computes no layout, and the naming
* because the page needs enough provider and API mocking that it had no test at all.
*
* Intercepting the API is not mocking the component -- the page really renders, so the
* accessible names and the real geometry are the ones a user gets.
*/
describe('Special Directories settings', () => {
const mockSettings = {
archive_destination: 'archive/%(domain_tag)s/%(domain)s',
download_manager_disabled: false,
download_manager_stopped: false,
download_on_startup: true,
download_timeout: 0,
download_wait: 20,
download_window_start: null,
download_window_end: null,
hotspot_device: 'wlan0',
hotspot_on_startup: true,
hotspot_password: 'wrolpi hotspot',
hotspot_ssid: 'WROLPi',
check_for_upgrades: true,
ignore_outdated_zims: false,
log_level: 'info',
map_destination: 'map',
nav_color: 'violet',
media_directory: '/media/wrolpi',
playlists_destination: 'playlists',
tags_directory: true,
throttle_on_startup: false,
version: '1.0.0',
videos_destination: 'videos/%(channel_tag)s/%(channel_name)s',
wrol_mode: false,
zims_destination: 'zims',
save_ffprobe_json: true,
};
const directories = [
['Archive Directory', 'archive/%(domain_tag)s/%(domain)s'],
['Videos Directory', 'videos/%(channel_tag)s/%(channel_name)s'],
['Map Directory', 'map'],
['Zims Directory', 'zims'],
['Playlists Directory', 'playlists'],
];
beforeEach(() => {
cy.intercept('GET', '/api/status', {
statusCode: 200,
body: {
version: '1.0.0',
flags: {},
cpu_percent: 10,
memory_percent: 30,
downloads: {
pending: 0, recurring: 0, disabled: false, stopped: false,
outside_download_window: false,
},
},
}).as('getStatus');
cy.intercept('GET', '/api/tags', {statusCode: 200, body: {tags: []}}).as('getTags');
cy.intercept('GET', '/api/events', {statusCode: 200, body: {events: []}}).as('getEvents');
cy.intercept('POST', '/api/search/suggestions', {
statusCode: 200,
body: {fileGroups: 0, zimsEstimates: [], channels: [], domains: []},
}).as('getSuggestions');
cy.intercept('GET', '/api/settings', {statusCode: 200, body: {...mockSettings}})
.as('getSettings');
cy.visit('/admin/settings');
cy.wait('@getSettings');
});
it('names every directory field, so they can be told apart', () => {
/*
* All five fields sit in one grid, hold similar-looking relative paths, and share the
* same prefix. Without a name attached to the input, a screen reader user hears five
* fields described only as "/media/wrolpi/" and cannot tell which one saves videos.
*/
directories.forEach(([label, value]) => {
cy.contains('label', label)
.invoke('attr', 'for')
.should('be.a', 'string')
.then((inputId) => {
cy.get(`#${CSS.escape(inputId)}`)
.should('have.value', value)
.and('match', 'input');
});
});
});
it('shows the media directory beside each value, never over it', () => {
// The reported bug: `a/media/wrolpi/hain_tag)s/%(domain)s`.
cy.get('.wrolpi-path-input').should('have.length', directories.length);
cy.get('.wrolpi-path-input').each(($field) => {
const prefix = $field.find('.wrolpi-path-input-prefix')[0].getBoundingClientRect();
const input = $field.find('input')[0].getBoundingClientRect();
expect(prefix.right, 'prefix ends where the input begins')
.to.be.at.most(input.left + 0.5);
expect(input.width, 'input is wide enough to read and edit').to.be.greaterThan(60);
});
});
it('keeps the prefix out of the value the user edits and saves', () => {
cy.contains('label', 'Archive Directory').invoke('attr', 'for').then((inputId) => {
cy.get(`#${CSS.escape(inputId)}`)
.should('have.value', 'archive/%(domain_tag)s/%(domain)s')
.and('not.have.value', '/media/wrolpi/archive/%(domain_tag)s/%(domain)s');
});
cy.get('.wrolpi-path-input-prefix').first().should('have.text', '/media/wrolpi/');
});
});

5
app/cypress/support/component.js
View file

@ -104,8 +104,9 @@ Cypress.Commands.add('shouldNotOverlap', {prevSubject: false}, (firstSelector, s
cy.get(secondSelector).then(($second) => {
const a = $first[0].getBoundingClientRect();
const b = $second[0].getBoundingClientRect();
const overlaps = a.right > b.left + 0.5 && b.right > a.left + 0.5
&& a.bottom > b.top + 0.5 && b.bottom > a.top + 0.5;
const overlapsHorizontally = (a.right > b.left + 0.5) && (b.right > a.left + 0.5);
const overlapsVertically = (a.bottom > b.top + 0.5) && (b.bottom > a.top + 0.5);
const overlaps = overlapsHorizontally && overlapsVertically;
expect(
overlaps,
`${firstSelector} [${a.left}, ${a.right}] must not overlap `

80
app/src/components/admin/Settings.js
View file

@ -759,25 +759,25 @@ export function SettingsPage() {
<Grid>
<Grid.Col span={{base: 12, sm: 6}}>
<label>
Archive Directory
<InfoPopup
w={340}
content={<>
<p>Variables:</p>
<ul>
<li><code>%(domain)s</code> - Domain name (e.g., "example.com")</li>
<li><code>%(domain_tag)s</code> - Tag name for the domain (empty if no
tag)
</li>
<li><code>%(tag)s</code> - Alias for the tag name</li>
<li><code>%(name)s</code> - Alias for the domain name</li>
</ul>
</>}
position='top'
/>
</label>
<PathInput
label={<>
Archive Directory
<InfoPopup
w={340}
content={<>
<p>Variables:</p>
<ul>
<li><code>%(domain)s</code> - Domain name (e.g., "example.com")</li>
<li><code>%(domain_tag)s</code> - Tag name for the domain (empty if no
tag)
</li>
<li><code>%(tag)s</code> - Alias for the tag name</li>
<li><code>%(name)s</code> - Alias for the domain name</li>
</ul>
</>}
position='top'
/>
</>}
prefix={mediaDirectoryLabel}
value={state.archive_destination}
disabled={!editSpecialDirectories}
@ -785,26 +785,26 @@ export function SettingsPage() {
/>
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>
Videos Directory
<InfoPopup
w={340}
content={<>
<p>Variables:</p>
<ul>
<li><code>%(channel_name)s</code> - Channel name</li>
<li><code>%(channel_tag)s</code> - Tag name for the channel (empty if no
tag)
</li>
<li><code>%(channel_domain)s</code> - Domain from the channel URL</li>
<li><code>%(tag)s</code> - Alias for the tag name</li>
<li><code>%(name)s</code> - Alias for the channel name</li>
</ul>
</>}
position='top'
/>
</label>
<PathInput
label={<>
Videos Directory
<InfoPopup
w={340}
content={<>
<p>Variables:</p>
<ul>
<li><code>%(channel_name)s</code> - Channel name</li>
<li><code>%(channel_tag)s</code> - Tag name for the channel (empty if no
tag)
</li>
<li><code>%(channel_domain)s</code> - Domain from the channel URL</li>
<li><code>%(tag)s</code> - Alias for the tag name</li>
<li><code>%(name)s</code> - Alias for the channel name</li>
</ul>
</>}
position='top'
/>
</>}
prefix={mediaDirectoryLabel}
value={state.videos_destination}
disabled={!editSpecialDirectories}
@ -812,8 +812,8 @@ export function SettingsPage() {
/>
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>Map Directory</label>
<PathInput
label='Map Directory'
prefix={mediaDirectoryLabel}
value={state.map_destination}
disabled={!editSpecialDirectories}
@ -821,8 +821,8 @@ export function SettingsPage() {
/>
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>Zims Directory</label>
<PathInput
label='Zims Directory'
prefix={mediaDirectoryLabel}
value={state.zims_destination}
disabled={!editSpecialDirectories}
@ -830,8 +830,8 @@ export function SettingsPage() {
/>
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>Playlists Directory</label>
<PathInput
label='Playlists Directory'
prefix={mediaDirectoryLabel}
value={state.playlists_destination}
disabled={!editSpecialDirectories}

29
app/src/components/ui/Inputs.tsx
View file

@ -90,6 +90,20 @@ export const PathInput = forwardRef<HTMLInputElement, PathInputProps>((
const generated = useId();
const inputId = id ?? `${generated}-input`;
const prefixId = `${generated}-prefix`;
const descriptionId = `${generated}-description`;
const errorId = `${generated}-error`;
/*
* Everything that explains the field, in reading order. Drawing the description and the
* error on screen does not announce them -- an unreferenced element is invisible to a
* screen reader, which would leave a user hearing the label and the prefix but never the
* variables the path accepts, nor the reason a path was just rejected.
*/
const describedBy = [
prefixId,
description ? descriptionId : null,
error ? errorId : null,
].filter(Boolean).join(' ');
return <div className={['wrolpi-path-input', className].filter(Boolean).join(' ')}>
{label && <label className='wrolpi-path-input-label' htmlFor={inputId}>
@ -100,20 +114,27 @@ export const PathInput = forwardRef<HTMLInputElement, PathInputProps>((
{/*
Described rather than hidden: a screen reader should hear which directory the
typed path is relative to, since that is the whole point of showing it.
`title` carries the full path because the prefix truncates when it is long --
the media directory is whatever the API reports, and a prefix that refused to
shrink took the entire row and left the input 18px wide.
*/}
<span className='wrolpi-path-input-prefix' id={prefixId}>{prefix}</span>
<span className='wrolpi-path-input-prefix' id={prefixId} title={prefix}>{prefix}</span>
<input
ref={ref}
id={inputId}
type='text'
required={required}
disabled={disabled}
aria-describedby={prefixId}
aria-describedby={describedBy}
aria-invalid={error ? true : undefined}
{...props}
/>
</div>
{description && <div className='wrolpi-path-input-description'>{description}</div>}
{error && <div className='wrolpi-path-input-error'>{error}</div>}
{description && <div className='wrolpi-path-input-description' id={descriptionId}>
{description}
</div>}
{error && <div className='wrolpi-path-input-error' id={errorId}>{error}</div>}
</div>
});
PathInput.displayName = 'PathInput';

26
app/src/components/ui/ui-layout.cy.js
View file

@ -63,6 +63,32 @@ describe('PathInput lays its prefix beside the value, never over it', () => {
});
});
it('still leaves somewhere to type when the media directory is a long path', () => {
/*
* The prefix is whatever the API reports as the media directory, so its length is not
* ours to assume. It is `/media/wrolpi` on a Pi, but a development box or an unusual
* Docker mount can report something far longer, and a prefix that refuses to shrink
* would take the whole row and leave the input at zero width -- a field that cannot
* be typed into at all.
*/
cy.mountUI(
<div style={{width: 320}}>
<PathInput
label='Archive Destination'
prefix='/mnt/storage/external/wrolpi-media-library-volume/'
defaultValue='archive/%(domain)s'
/>
</div>,
);
cy.shouldNotOverlap('.wrolpi-path-input-prefix', '.wrolpi-path-input-control input');
cy.get('.wrolpi-path-input-control input').should(($input) =>
expect($input[0].clientWidth, 'input keeps a usable width').to.be.greaterThan(60));
// The control must not spill out of the column it was given either.
cy.get('.wrolpi-path-input-control').should(($control) =>
expect($control[0].getBoundingClientRect().width).to.be.at.most(320));
});
it('detects the overlap that started this, so the check has teeth', () => {
/*
* The original defect, reproduced deliberately: Mantine turns leftSectionWidth into

17
app/src/components/ui/ui.css
View file

@ -429,11 +429,24 @@ html[data-theme="amber"] .wrolpi-path-input-control:focus-within {
background: var(--bg);
}
/* Muted and on the header surface: it reads as context, not as something to edit. */
/*
* Muted and on the header surface: it reads as context, not as something to edit.
*
* It shrinks and ellipsises rather than holding its full width. The prefix is whatever the
* API reports as the media directory -- `/media/wrolpi` on a Pi, but a development box or an
* unusual Docker mount can report a much longer path -- and at `flex: none` a long one took
* the whole row and left the input 18px wide, a field nobody could type into. Capping it at
* half the control keeps the editable part editable; the full path stays available through
* `title` and is read out in full by the accessible description either way.
*/
.wrolpi-path-input-prefix {
display: flex;
align-items: center;
flex: none;
flex: 0 1 auto;
min-width: 3ch;
max-width: 50%;
overflow: hidden;
text-overflow: ellipsis;
padding: 0 6px 0 9px;
background: var(--head);
border-right: 1px solid var(--border);

36
app/src/components/ui/ui.test.js
View file

@ -343,6 +343,42 @@ describe('PathInput', () => {
expect(screen.getByText('Not a directory')).toBeInTheDocument();
});
it('reads out its help text as well as the prefix', () => {
/*
* Rendering the description on screen is not the same as announcing it. Unless it is
* referenced, a screen reader user hears the label and the prefix and never learns
* what the field expects -- the help text is the part that explains the variables
* these paths are allowed to contain.
*/
renderUI(<PathInput
prefix='/media/wrolpi/'
label='Archive Directory'
description='Accepts %(domain)s and %(tag)s.'
value=''
onChange={jest.fn()}
/>);
expect(screen.getByLabelText(/Archive Directory/))
.toHaveAccessibleDescription('/media/wrolpi/ Accepts %(domain)s and %(tag)s.');
});
it('announces the reason a path was rejected', () => {
// The dashed border and red text carry the failure for sighted users; without the
// error in the accessible description nothing carries it for anyone else.
renderUI(<PathInput prefix='/media/wrolpi/' label='Zims Directory'
error='Not a directory' value='' onChange={jest.fn()}/>);
const input = screen.getByLabelText(/Zims Directory/);
expect(input).toHaveAccessibleDescription(/Not a directory/);
expect(input).toBeInvalid();
});
it('is not marked invalid when there is nothing wrong with it', () => {
renderUI(<PathInput prefix='/media/wrolpi/' label='Fine' value='' onChange={jest.fn()}/>);
expect(screen.getByLabelText(/Fine/)).toBeValid();
});
it('forwards a ref to the input, for callers that focus it', () => {
const ref = React.createRef();
renderUI(<PathInput ref={ref} prefix='/media/wrolpi/' label='Focusable'