mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 19:01:39 -04:00
ui: three silent library defects the agents' reports surfaced
Each of these was reported by an agent working around it rather than patched in one file, which is why all three are fixed once, for every call site. Button accepted an `href` and dropped it. Mantine renders an anchor only when it also gets `component='a'`; Semantic spelled that `as='a'`. So a migrated call site that kept just the href produced something that looks like a download link and navigates nowhere -- silently, with no warning. Button and IconButton now infer the anchor from the href. The media filter ran twice on nested media. A `.media` wrapper filters its subtree as one unit -- the audio player needs that, because its control glyphs are SVG and the element-type rule does not reach them -- but the poster image inside also matched that rule. Two passes of a luminance-to-red matrix is not one: the second takes 0.2126 of an already-red pixel, so the poster came out markedly darker than everything around it. Leaf media inside a filtered wrapper is now left alone. Progress had no indeterminate state. Semantic's `indicating` animated a stripe when the size was unknown; without it an upload that has not reported yet sits at 0% and reads as stalled -- and `aria-valuenow="0"` tells a screen reader the same wrong thing. It now animates, reports no value, and falls back to a static fill under prefers-reduced-motion. Also: night and amber now override an inline `--label-color`. A call site may legitimately need many distinguishable colours -- the Flasher's chip badges use a twelve-colour palette no four tokens can express -- but on a night-adapted screen a green or blue chip badge defeats the entire mode. The labels lose their distinctness there, which is the right trade.
This commit is contained in:
parent
d981b2506b
commit
4b7a195220
7 changed files with 136 additions and 5 deletions
|
|
@ -410,6 +410,9 @@ export function ThemeSamplePage() {
|
|||
<Row>
|
||||
<Button role='cancel' icon='eye' onClick={() => setModalOpen(true)}>Open a modal</Button>
|
||||
</Row>
|
||||
<div style={{marginTop: 12, maxWidth: 320}}>
|
||||
<Progress indeterminate label='Uploading…'/>
|
||||
</div>
|
||||
<p style={{fontSize: 12, color: 'var(--muted)', marginBottom: 0, marginTop: 10}}>
|
||||
Semantic's compound shape is kept — Modal.Header, Modal.Content,
|
||||
Modal.Actions — so the 34 call sites written against it migrate by import.
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ export interface ButtonProps extends Omit<MButtonProps, 'leftSection' | 'rightSe
|
|||
type?: 'button' | 'submit' | 'reset';
|
||||
href?: string;
|
||||
target?: string;
|
||||
component?: any;
|
||||
}
|
||||
|
||||
const renderIcon = (icon?: string | React.ComponentType<any>) => {
|
||||
|
|
@ -69,6 +70,13 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>((
|
|||
const fromRole = role ? roleProps[role] : undefined;
|
||||
return <MButton
|
||||
ref={ref}
|
||||
/*
|
||||
* An `href` makes this an anchor. Mantine needs `component='a'` to render one, and
|
||||
* without it the href is accepted and silently dropped — the control looks like a
|
||||
* link and navigates nowhere. Semantic used `as='a'` for this, so every migrated
|
||||
* call site that carried `href` alone was quietly broken.
|
||||
*/
|
||||
component={props.component ?? (props.href ? 'a' : undefined)}
|
||||
// An explicit color/variant still wins, so a call site can deviate when it must.
|
||||
color={props.color ?? fromRole?.color}
|
||||
variant={props.variant ?? fromRole?.variant}
|
||||
|
|
@ -88,6 +96,9 @@ export interface IconButtonProps extends Omit<ActionIconProps, 'children'> {
|
|||
label: string;
|
||||
onClick?: React.MouseEventHandler<HTMLButtonElement>;
|
||||
type?: 'button' | 'submit' | 'reset';
|
||||
href?: string;
|
||||
target?: string;
|
||||
component?: any;
|
||||
}
|
||||
|
||||
/** A square, icon-only button. */
|
||||
|
|
@ -97,6 +108,8 @@ export const IconButton = forwardRef<HTMLButtonElement, IconButtonProps>((
|
|||
const fromRole = role ? roleProps[role] : undefined;
|
||||
return <ActionIcon
|
||||
ref={ref}
|
||||
// See Button: an href without `component='a'` renders a non-navigating button.
|
||||
component={props.component ?? (props.href ? 'a' : undefined)}
|
||||
aria-label={label}
|
||||
title={label}
|
||||
color={props.color ?? fromRole?.color}
|
||||
|
|
|
|||
|
|
@ -86,6 +86,11 @@ export function Label({color = 'grey', icon, children, onClick, className}: Labe
|
|||
export interface ProgressProps {
|
||||
/** 0-100. Values outside the range are clamped. */
|
||||
percent?: number;
|
||||
/**
|
||||
* Work is happening but its size is unknown — an upload that has not reported yet.
|
||||
* Animates instead of sitting at 0%, which otherwise reads as stalled.
|
||||
*/
|
||||
indeterminate?: boolean;
|
||||
/** Show the percentage inside the bar. */
|
||||
showPercent?: boolean;
|
||||
color?: SemanticColorName;
|
||||
|
|
@ -94,18 +99,26 @@ export interface ProgressProps {
|
|||
className?: string;
|
||||
}
|
||||
|
||||
export function Progress({percent = 0, showPercent = true, color = 'blue', label, className}: ProgressProps) {
|
||||
export function Progress({
|
||||
percent = 0, showPercent = true, color = 'blue', label, indeterminate, className,
|
||||
}: ProgressProps) {
|
||||
const clamped = Math.min(100, Math.max(0, Number.isFinite(percent) ? percent : 0));
|
||||
return <div
|
||||
className={['wrolpi-progress', className].filter(Boolean).join(' ')}
|
||||
className={['wrolpi-progress', indeterminate ? 'wrolpi-progress-indeterminate' : '', className]
|
||||
.filter(Boolean).join(' ')}
|
||||
role='progressbar'
|
||||
aria-valuenow={Math.round(clamped)}
|
||||
// An indeterminate bar reports no value, which is what tells assistive tech the
|
||||
// amount is unknown rather than zero.
|
||||
aria-valuenow={indeterminate ? undefined : Math.round(clamped)}
|
||||
aria-valuemin={0}
|
||||
aria-valuemax={100}
|
||||
>
|
||||
<div
|
||||
className='wrolpi-progress-fill'
|
||||
style={{width: `${clamped}%`, ['--progress-color' as string]: `var(--${color})`}}
|
||||
style={{
|
||||
width: indeterminate ? undefined : `${clamped}%`,
|
||||
['--progress-color' as string]: `var(--${color})`,
|
||||
}}
|
||||
/>
|
||||
{(showPercent || label) && <span className='wrolpi-progress-text'>
|
||||
{label ?? `${Math.round(clamped)}%`}
|
||||
|
|
|
|||
|
|
@ -499,6 +499,22 @@ html[data-theme="night"] .wrolpi-label {
|
|||
margin-top: 3px;
|
||||
}
|
||||
|
||||
/*
|
||||
* Night and amber are single-hue by definition, and a call site may legitimately want
|
||||
* many distinguishable colours — the Flasher's chip-family badges set `--label-color`
|
||||
* inline from a twelve-colour palette that no four tokens can express.
|
||||
*
|
||||
* In those themes the palette is overridden back to the theme's own text colour, so a
|
||||
* label cannot put a green or blue pixel on a night-adapted screen. `!important` is
|
||||
* required because the palette arrives as an inline custom property, which a normal
|
||||
* rule cannot outrank. The labels stop being distinguishable by colour, which is the
|
||||
* correct trade: the theme's whole purpose is that no other hue reaches the eye.
|
||||
*/
|
||||
html[data-theme="night"] .wrolpi-label,
|
||||
html[data-theme="amber"] .wrolpi-label {
|
||||
--label-color: var(--text) !important;
|
||||
}
|
||||
|
||||
/* --------------------------------------------------------------- Messages */
|
||||
|
||||
.wrolpi-message {
|
||||
|
|
@ -581,6 +597,34 @@ html[data-theme="night"] .wrolpi-label {
|
|||
}
|
||||
}
|
||||
|
||||
/*
|
||||
* Indeterminate: a sliding band, for work whose size is not known yet. Semantic called
|
||||
* this `indicating`; without it an upload that has not reported its size sits at 0% and
|
||||
* reads as stalled.
|
||||
*/
|
||||
.wrolpi-progress-indeterminate .wrolpi-progress-fill {
|
||||
width: 35%;
|
||||
animation: wrolpi-progress-slide 1.4s ease-in-out infinite;
|
||||
}
|
||||
|
||||
@keyframes wrolpi-progress-slide {
|
||||
from {
|
||||
margin-left: -35%;
|
||||
}
|
||||
to {
|
||||
margin-left: 100%;
|
||||
}
|
||||
}
|
||||
|
||||
/* Motion is the only signal here, so when it is suppressed the bar fills instead. */
|
||||
@media (prefers-reduced-motion: reduce) {
|
||||
.wrolpi-progress-indeterminate .wrolpi-progress-fill {
|
||||
animation: none;
|
||||
width: 100%;
|
||||
opacity: 0.5;
|
||||
}
|
||||
}
|
||||
|
||||
.wrolpi-progress-text {
|
||||
position: absolute;
|
||||
inset: 0;
|
||||
|
|
|
|||
|
|
@ -152,6 +152,30 @@ describe('Button', () => {
|
|||
expect(screen.getByRole('button', {name: 'New'})).toHaveAttribute('data-size', 'sm');
|
||||
});
|
||||
|
||||
it('renders an anchor when given an href, without being told twice', () => {
|
||||
/*
|
||||
* Mantine drops `href` unless it also gets `component='a'`, so a button carrying
|
||||
* only an href looked like a link and navigated nowhere. Semantic spelled this
|
||||
* `as='a'`, so every migrated call site that kept just the href was silently
|
||||
* broken — a download button that downloads nothing.
|
||||
*/
|
||||
renderUI(<>
|
||||
<Button href='/media/thing.pdf'>Download</Button>
|
||||
<IconButton icon='download' label='Save file' href='/media/other.pdf'/>
|
||||
</>);
|
||||
|
||||
const link = screen.getByRole('link', {name: 'Download'});
|
||||
expect(link.tagName).toBe('A');
|
||||
expect(link).toHaveAttribute('href', '/media/thing.pdf');
|
||||
expect(screen.getByRole('link', {name: 'Save file'}).tagName).toBe('A');
|
||||
});
|
||||
|
||||
it('is still a button when there is no href', () => {
|
||||
renderUI(<Button onClick={jest.fn()}>Save</Button>);
|
||||
|
||||
expect(screen.getByRole('button', {name: 'Save'}).tagName).toBe('BUTTON');
|
||||
});
|
||||
|
||||
it('gives icon-only buttons an accessible name', () => {
|
||||
renderUI(<IconButton icon='trash' label='Delete channel'/>);
|
||||
|
||||
|
|
@ -270,6 +294,17 @@ describe('Progress', () => {
|
|||
expect(screen.getByRole('progressbar')).toHaveAttribute('aria-valuenow', '0');
|
||||
});
|
||||
|
||||
it('reports no value when the size is unknown', () => {
|
||||
// Semantic called this `indicating`. An upload that has not reported its size
|
||||
// would otherwise sit at 0% and read as stalled, and `aria-valuenow=0` would tell
|
||||
// a screen reader the same wrong thing.
|
||||
renderUI(<Progress indeterminate label='Uploading…'/>);
|
||||
|
||||
const bar = screen.getByRole('progressbar');
|
||||
expect(bar).not.toHaveAttribute('aria-valuenow');
|
||||
expect(bar).toHaveClass('wrolpi-progress-indeterminate');
|
||||
});
|
||||
|
||||
it('shows arbitrary label text instead of the percentage', () => {
|
||||
renderUI(<Progress percent={40} label='2.1 GB / 5.3 GB'/>);
|
||||
|
||||
|
|
|
|||
|
|
@ -204,6 +204,19 @@ html[data-media-filter="amber-mono"] :is(img, video, canvas, iframe, embed, obje
|
|||
filter: url(#wrolpi-amber-mono);
|
||||
}
|
||||
|
||||
/*
|
||||
* A `.media` wrapper filters its whole subtree as one unit, so the leaf media inside
|
||||
* it must not be filtered a second time. Two passes of a luminance-to-red matrix is
|
||||
* not the same as one: the second pass takes the luminance of an already-red pixel
|
||||
* (0.2126 of it) and the image comes out markedly darker than everything around it.
|
||||
*
|
||||
* The audio player's poster is the case that found this — the poster sits under
|
||||
* custom control glyphs, so the wrapper is filtered to keep the two tinted together.
|
||||
*/
|
||||
html[data-media-filter] .media :is(img, video, canvas, iframe, embed, object) {
|
||||
filter: none;
|
||||
}
|
||||
|
||||
/*
|
||||
* A filter on an ancestor stops applying once the browser promotes a video to
|
||||
* native fullscreen, so the filter is restated on the fullscreened element.
|
||||
|
|
|
|||
|
|
@ -103,7 +103,9 @@ describe('media filter rules', () => {
|
|||
.map(match => match[1]);
|
||||
|
||||
expect(filterRules.length).toBeGreaterThan(0);
|
||||
filterRules.forEach(selector => expect(selector).toMatch(/^data-media-filter=/));
|
||||
// With or without a value: the reset rule for nested media keys on the bare
|
||||
// attribute. What matters is that no filter rule keys on `data-theme`.
|
||||
filterRules.forEach(selector => expect(selector).toMatch(/^data-media-filter/));
|
||||
});
|
||||
|
||||
it('has a rule for every filter a theme offers', () => {
|
||||
|
|
@ -117,6 +119,14 @@ describe('media filter rules', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('does not filter leaf media twice inside a filtered wrapper', () => {
|
||||
// A `.media` wrapper filters its subtree as a unit; the img/video inside it also
|
||||
// matches the element-type rule, and two passes of the matrix take the luminance
|
||||
// of an already-red pixel, leaving that image much darker than its surroundings.
|
||||
expect(tokensCss).toMatch(
|
||||
/html\[data-media-filter] \.media :is\([^)]*\)\s*{\s*filter:\s*none/);
|
||||
});
|
||||
|
||||
it('never filters an ancestor, only leaf media', () => {
|
||||
// A CSS filter creates a containing block, which would break `position: fixed`
|
||||
// descendants — the nav bar and every modal.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue