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

test: act on review — and one of the seven turned out to be undemonstrable

Browse source 
Six real fixes and one honest retraction.

The control is now a SIBLING of the heading inside a flex row, not a child of
it.  Inside the `<h3>` it joined the heading's accessible name, so screen reader
heading navigation announced "Root CA Certificate Help".  The row is what keeps
them on one line, and it is only rendered when there is something to sit beside,
so the markup for the other 46 call sites is unchanged.

`style` now lands on the wrapper.  It used to reach the outer div; going through
Header put it on the inner heading, where a margin sets space INSIDE the wrapper
and only worked at all through margin collapse -- which stops the moment the
wrapper gains padding or a border.  Status's "Drive Bandwidth" is the live case:
verified 16px on the block, 0 on the h2.  Text properties still arrive, because
colour and text-align inherit; the `color` PROP stays on the heading.

A `<label>` only when `for_` names a field.  Several call sites pass an
InfoHeader as the `label` prop of InputForm/ToggleForm, which wraps it in a
label of its own -- an unconditional one nested labels, which is invalid and
makes clicking the text ambiguous.  Verified: zero `label label` on
/videos/settings, where "Video Resolutions" keeps its label and "Video File
Format" no longer has one.

The help controls carried `iconStyle: {margin: '0.5em'}`, which separated a
free-floating sibling and, inside the row, added to the gap on every side.
Zeroed; the row's gap is the spacing.

The browser test mounted a plain Button in an `after` slot rather than a real
HelpHeader, so the default icon chrome above was exactly what it could not see.
It mounts the real wrapper now.  Two of my assertions were also too weak to
catch what they claimed: the gap was measured to `.wrolpi-header-after`, a flex
container whose own rect does not move when a child gains a margin, so it had to
measure the button; and the squeeze case used a title of ordinary words, which
simply wraps and never puts the row under pressure.

Renamed the suites -- "keeps the icon on its own line" read as the opposite of
the fix.

The retraction: `flex-shrink: 0` is declared but has NO test. Three attempts
could not find a width where removing it changes what is painted -- the row's
only other item is a heading, and its automatic minimum size absorbs the
pressure; the help control is an ActionIcon with a min-width it will not go
below either.  The declaration stays as a statement of intent, with a comment
saying it is unproven, rather than a green test that does not discriminate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-08-01 19:00:32 -06:00
parent 5491a82361
commit f153d7c8d8
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
6 changed files with 174 additions and 43 deletions
Download patch file Download diff file Expand all files Collapse all files

26
app/src/components/Common.js
View file

@ -820,11 +820,24 @@ export function InfoHeader({
* The label stays, now inside the heading, because `for_` associates the text with a
* field on three of these call sites.
*/
/*
* A `<label>` ONLY when `for_` names a field. Several call sites pass an InfoHeader as
* the `label` prop of InputForm/ToggleForm, which wraps whatever it is given in a
* `<label>` of its own -- an unconditional label here nests one inside another, which is
* invalid HTML and confuses what clicking the text does. That predates this change; it
* is fixed here rather than carried forward.
*
* `margin: 0` on the icon because the row's `gap` does the spacing now. The 0.5em
* default was there to separate a free-floating sibling, and inside the row it adds to
* the gap on every side.
*/
return <Header as={headerSize} {...props} after={
<InfoPopup content={popupContent} iconSize={iconSize} icon={icon} position={popupPosition}
{...popupProps}/>
iconStyle={{margin: 0}} {...popupProps}/>
}>
<label htmlFor={for_}>{headerContent} {required && <RequiredAsterisk/>}</label>
{for_
? <label htmlFor={for_}>{headerContent} {required && <RequiredAsterisk/>}</label>
: <>{headerContent} {required && <RequiredAsterisk/>}</>}
</Header>
}
@ -864,12 +877,15 @@ export function HelpHeader({
required = false,
...props
}) {
// See the note in InfoHeader: the icon belongs in the heading's row, not beside it.
// See the notes in InfoHeader: the control belongs in the heading's row, a label only
// when it names a field, and the row's gap is the spacing.
return <Header as={headerSize} {...props} after={
<HelpModal helpPath={helpPath} iconSize={iconSize} icon={icon} title={helpTitle}
{...helpModalProps}/>
iconStyle={{margin: 0}} {...helpModalProps}/>
}>
<label htmlFor={for_}>{headerContent} {required && <RequiredAsterisk/>}</label>
{for_
? <label htmlFor={for_}>{headerContent} {required && <RequiredAsterisk/>}</label>
: <>{headerContent} {required && <RequiredAsterisk/>}</>}
</Header>
}

38
app/src/components/Common.test.js
View file

@ -623,7 +623,7 @@ describe('healthRole', () => {
});
});
describe('a heading with a help icon keeps the icon on its own line', () => {
describe('a heading keeps its help icon on the same row as the text', () => {
/*
* `HelpHeader` and `InfoHeader` put their icon in a sibling `<span>` beside a `<label>`,
* held on one line by `.inline-header h1..h5 {display: inline-block}` in App.css. That
@ -635,20 +635,36 @@ describe('a heading with a help icon keeps the icon on its own line', () => {
* jsdom can judge; that the two actually share a line is asserted in ui-layout.cy.js,
* since jsdom has no layout at all.
*/
it('renders the help control inside the heading element', () => {
render(<HelpHeader headerSize='h3' headerContent='Root CA Certificate'
helpPath='/system/certificates/'/>);
it("renders the help control in the heading's row", () => {
const {container} = render(
<HelpHeader headerSize='h3' headerContent='Root CA Certificate'
helpPath='/system/certificates/'/>);
const heading = screen.getByRole('heading', {name: /Root CA Certificate/});
expect(heading).toContainElement(screen.getByRole('button', {name: 'Help'}));
expect(container.querySelector('.wrolpi-header-row'))
.toContainElement(screen.getByRole('button', {name: 'Help'}));
// And not inside the heading itself, which would put it in the accessible name.
expect(screen.getByRole('heading')).toHaveAccessibleName('Root CA Certificate');
});
it('renders the info popup inside the heading element', () => {
render(<InfoHeader headerSize='h4' headerContent='Video Resolutions'
popupContent='Highest available is tried first.'/>);
it("renders the info popup in the heading's row", () => {
const {container} = render(
<InfoHeader headerSize='h4' headerContent='Video Resolutions'
popupContent='Highest available is tried first.'/>);
const heading = screen.getByRole('heading', {name: /Video Resolutions/});
expect(heading.querySelector('svg')).not.toBeNull();
expect(container.querySelector('.wrolpi-header-after svg')).not.toBeNull();
});
it('renders no label at all when there is no field to name', () => {
/*
* Several call sites pass an InfoHeader as the `label` prop of InputForm/ToggleForm,
* which wraps it in a `<label>` of its own. An unconditional label here nested one
* inside another -- invalid HTML, and it makes clicking the text ambiguous.
*/
const {container} = render(
<InfoHeader headerSize='h5' headerContent='Video File Format' popupContent='...'/>);
expect(container.querySelector('label')).toBeNull();
expect(screen.getByRole('heading', {name: /Video File Format/})).toBeInTheDocument();
});
it('keeps the label association, which is what `for_` is for', () => {

59
app/src/components/ui/Surfaces.tsx
View file

@ -28,11 +28,14 @@ export interface HeaderProps extends Omit<React.HTMLAttributes<HTMLHeadingElemen
*/
color?: string;
/**
* Trailing content on the heading's own line — a help button, a count, a small
* control. It goes inside the heading's flex row, which is the only place it can
* sit beside the text: the wrapper is a block, so a sibling placed after it wraps
* to the next line. That is exactly what happened to every help icon in the app
* when these headings stopped being a bare `<h3>`.
* Trailing content on the heading's row — a help button, a count, a small control.
*
* It is a SIBLING of the heading element inside a flex row, not a child of it: a
* control inside an `<h3>` becomes part of the heading's accessible name, so screen
* reader heading navigation announces "Root CA Certificate Help". The row is what
* keeps the two on one line; a sibling placed after the block wrapper wraps instead,
* which is what happened to every help icon in the app when these headings stopped
* being a bare `<h3>`.
*/
after?: React.ReactNode;
}
@ -48,19 +51,39 @@ export function Header({
as = 'h3', icon, subheader, dividing, color, after, className, style, children, ...props
}: HeaderProps) {
const Tag = as;
return <div className={['wrolpi-header', dividing ? 'wrolpi-header-dividing' : '', className]
.filter(Boolean).join(' ')}>
<Tag
className={`wrolpi-header-text wrolpi-header-${as}`}
style={color ? {color: `var(--${color})`, ...style} : style}
{...props}
>
{icon && (typeof icon === 'string'
? <Icon name={icon} size='medium'/>
: <Icon component={icon} size='medium'/>)}
<span>{children}</span>
{after && <span className='wrolpi-header-after'>{after}</span>}
</Tag>
const heading = <Tag
className={`wrolpi-header-text wrolpi-header-${as}`}
style={color ? {color: `var(--${color})`} : undefined}
{...props}
>
{icon && (typeof icon === 'string'
? <Icon name={icon} size='medium'/>
: <Icon component={icon} size='medium'/>)}
<span>{children}</span>
</Tag>;
/*
* `style` lands on the WRAPPER, because every layout use of it is about the header as a
* block -- `marginBottom` on Status's "Drive Bandwidth", `marginTop` on the extension
* page's steps. On the inner heading those set a margin inside the wrapper and depended
* on margin collapse to have any effect at all, which stops the moment the wrapper gains
* padding or a border. Text properties passed this way still reach the heading: `color`
* and `text-align` inherit. A `color` PROP stays on the heading, since that is about the
* text rather than the box.
*/
return <div
className={['wrolpi-header', dividing ? 'wrolpi-header-dividing' : '', className]
.filter(Boolean).join(' ')}
style={style}
>
{/* The row exists only when there is something to sit beside; without it the
markup for the other 46 call sites is unchanged. */}
{after
? <div className='wrolpi-header-row'>
{heading}
<span className='wrolpi-header-after'>{after}</span>
</div>
: heading}
{subheader && <div className='wrolpi-header-sub'>{subheader}</div>}
</div>
}

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

@ -4,7 +4,7 @@ import {
Message, Placeholder, Statistic, StatisticGroup, Status, TabBar, Table, tabClassName,
TextInput,
} from './index';
import {contrastingColor, LoadStatistic} from '../Common';
import {contrastingColor, HelpHeader, LoadStatistic} from '../Common';
import {Notifications} from '@mantine/notifications';
import {clearToasts, toast} from './toast';
import {monochromeThemes, themeNames} from '../../themes/names';
@ -398,8 +398,41 @@ describe('a heading keeps its trailing control on its own line', () => {
* there, and containment alone (which the jest tests assert) does not prove two boxes
* share a line.
*/
/*
* The REAL wrapper, not a plain Button in an `after` slot. A hand-built fixture would
* miss the thing most likely to be wrong: `HelpModal` and `InfoPopup` carry their own
* `iconStyle` default of `margin: 0.5em`, which was there to separate a free-floating
* sibling and adds to the row's gap on every side once it is inside the row.
*/
themeNames.forEach((theme) => {
it(`sits the control beside the text in ${theme}`, () => {
it(`sits a real help control beside the text in ${theme}`, () => {
cy.mountUI(
<HelpHeader headerSize='h3' headerContent='Root CA Certificate'
helpPath='/system/certificates/'/>,
{theme},
);
cy.get('.wrolpi-header-after').should(($after) => {
const control = $after[0].getBoundingClientRect();
const text = $after[0].previousElementSibling.getBoundingClientRect();
expect(control.top, 'control starts above the text baseline')
.to.be.lessThan(text.bottom);
expect(text.top, 'text starts above the control baseline')
.to.be.lessThan(control.bottom);
expect(control.left, 'control is after the text').to.be.at.least(text.right);
/*
* Measured to the BUTTON, not to `.wrolpi-header-after`. That wrapper is a
* flex container, so a margin on the button sits inside it and the wrapper's
* own rect does not move -- measuring the wrapper cannot see the margin at
* all, which is how this assertion first passed with it restored.
*/
const button = $after[0].querySelector('button').getBoundingClientRect();
expect(button.left - text.right, 'the row gap alone, no icon margin on top')
.to.be.at.most(12);
});
});
it(`sits an arbitrary control beside the text in ${theme}`, () => {
cy.mountUI(
<Header as='h3' after={<Button role='cancel' size='xs'>Help</Button>}>
Root CA Certificate

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

@ -756,13 +756,28 @@ html[data-theme="night"] .wrolpi-tag::before {
margin-top: 22px;
}
/* The heading and its trailing control share one row. */
.wrolpi-header-row {
display: flex;
align-items: center;
gap: 8px;
}
/*
* Trailing content keeps the heading's baseline but not its size: a help button rendered
* at heading scale is a 26px glyph next to a 17px word.
* Trailing content keeps the heading's row but not its size: a help button rendered at
* heading scale is a 26px glyph next to a 17px word.
*
* `flex-shrink: 0` states the intent -- a control is not the thing that should give way,
* a long title should wrap instead. Declared, but NOT covered by a test: with the row's
* only other item being a heading, the heading's automatic minimum size absorbs the
* pressure and no width could be found where removing this changes what is painted. It
* is here for the day the slot holds something beside a heading that can shrink, not
* because a case exists today.
*/
.wrolpi-header-after {
display: inline-flex;
align-items: center;
flex-shrink: 0;
font-size: 13px;
font-weight: 400;
}

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

@ -347,18 +347,46 @@ describe('Header', () => {
expect(screen.getByText('Paste this into the extension')).toBeInTheDocument();
});
it('puts trailing content inside the heading, not after its wrapper', () => {
it("puts trailing content in the heading's row, not after its wrapper", () => {
/*
* The whole point of the slot. `.wrolpi-header` is a block, so anything placed after
* it starts a new line -- which is what happened to every help icon in the app when
* these headings stopped being a bare `<h3>`. Inside the heading element it joins
* the existing flex row.
* these headings stopped being a bare `<h3>`. A shared flex row holds them together.
*/
const {container} = renderUI(
<Header as='h3' after={<button type='button'>Help</button>}>Root CA Certificate</Header>);
const heading = container.querySelector('h3');
expect(heading).toContainElement(screen.getByRole('button', {name: 'Help'}));
const row = container.querySelector('.wrolpi-header-row');
expect(row).toContainElement(container.querySelector('h3'));
expect(row).toContainElement(screen.getByRole('button', {name: 'Help'}));
});
it("keeps the control out of the heading's accessible name", () => {
// Inside the <h3> the control becomes part of the name, so heading navigation
// announces "Root CA Certificate Help".
renderUI(<Header as='h3' after={<button type='button'>Help</button>}>Root CA Certificate</Header>);
expect(screen.getByRole('heading')).toHaveAccessibleName('Root CA Certificate');
});
it('puts layout styles on the block, not on the heading inside it', () => {
/*
* `style` is used for spacing the header as a whole -- marginBottom on Status's
* "Drive Bandwidth", marginTop on the extension page. On the inner heading those set
* a margin inside the wrapper and worked only through margin collapse, which stops as
* soon as the wrapper gains padding or a border.
*/
const {container} = renderUI(
<Header as='h3' style={{marginBottom: '1em'}}>Drive Bandwidth</Header>);
expect(container.querySelector('.wrolpi-header')).toHaveStyle({marginBottom: '1em'});
expect(container.querySelector('h3').style.marginBottom).toBe('');
});
it('still colours the heading text from the colour prop', () => {
const {container} = renderUI(<Header as='h4' color='danger'>2 items to remove</Header>);
expect(container.querySelector('h4')).toHaveStyle({color: 'var(--danger)'});
});
it('renders nothing extra when there is no trailing content', () => {