Skip to content

Commit 47d2618

Browse files
sampottscursoragent
andcommitted
fix(core): time slider valuetext at zero duration and CI docs
Use Number.isFinite for duration in time slider aria-valuetext so duration 0 still formats. Fix formatOptions JSDoc for api-docs schema. Cache optional control labels so impure label callbacks run once per state. Inline duration decomposition in formatDuration (remove redundant toDurationRecord filtering). Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 1856b0d commit 47d2618

9 files changed

Lines changed: 94 additions & 30 deletions

File tree

packages/core/src/core/ui/playback-rate-button/playback-rate-button-core.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { defaults } from '@videojs/utils/object';
33
import type { NonNullableObject } from '@videojs/utils/types';
44

55
import type { MediaPlaybackRateState } from '../../media/state';
6-
import { resolveOptionalControlLabel } from '../resolve-optional-control-label';
6+
import { createOptionalControlLabelCache } from '../resolve-optional-control-label';
77
import type { ButtonState, TranslationKeyOrString } from '../types';
88

99
export interface PlaybackRateButtonProps {
@@ -30,24 +30,26 @@ export class PlaybackRateButtonCore {
3030

3131
#props = { ...PlaybackRateButtonCore.defaultProps };
3232
#media: MediaPlaybackRateState | null = null;
33+
readonly #customLabel = createOptionalControlLabelCache<PlaybackRateButtonState>();
3334

3435
constructor(props?: PlaybackRateButtonProps) {
3536
if (props) this.setProps(props);
3637
}
3738

3839
setProps(props: PlaybackRateButtonProps): void {
3940
this.#props = defaults(props, PlaybackRateButtonCore.defaultProps);
41+
this.#customLabel.invalidate();
4042
}
4143

4244
getLabel(state: PlaybackRateButtonState): TranslationKeyOrString {
43-
const custom = resolveOptionalControlLabel(this.#props.label, state);
45+
const custom = this.#customLabel.resolve(this.#props.label, state);
4446
if (custom !== undefined) return custom;
4547

4648
return 'playbackRateAria';
4749
}
4850

4951
getLabelParams(state: PlaybackRateButtonState): { rate: number } | undefined {
50-
if (resolveOptionalControlLabel(this.#props.label, state) !== undefined) return undefined;
52+
if (this.#customLabel.resolve(this.#props.label, state) !== undefined) return undefined;
5153
return { rate: state.rate };
5254
}
5355

packages/core/src/core/ui/playback-rate-menu/playback-rate-menu-core.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { isUndefined } from '@videojs/utils/predicate';
44
import type { NonNullableObject } from '@videojs/utils/types';
55

66
import type { MediaPlaybackRateState } from '../../media/state';
7-
import { resolveOptionalControlLabel } from '../resolve-optional-control-label';
7+
import { createOptionalControlLabelCache } from '../resolve-optional-control-label';
88
import type { ButtonState, TranslationKeyOrString } from '../types';
99

1010
export interface PlaybackRateMenuProps {
@@ -42,23 +42,25 @@ export class PlaybackRateMenuCore {
4242

4343
#props = { ...PlaybackRateMenuCore.defaultProps };
4444
#media: MediaPlaybackRateState | null = null;
45+
readonly #customLabel = createOptionalControlLabelCache<PlaybackRateMenuState>();
4546

4647
constructor(props?: PlaybackRateMenuProps) {
4748
if (props) this.setProps(props);
4849
}
4950

5051
setProps(props: PlaybackRateMenuProps): void {
5152
this.#props = defaults(props, PlaybackRateMenuCore.defaultProps);
53+
this.#customLabel.invalidate();
5254
}
5355

5456
getLabel(state: PlaybackRateMenuState): TranslationKeyOrString {
55-
const custom = resolveOptionalControlLabel(this.#props.label, state);
57+
const custom = this.#customLabel.resolve(this.#props.label, state);
5658
if (custom !== undefined) return custom;
5759
return 'playbackRateAria';
5860
}
5961

6062
getLabelParams(state: PlaybackRateMenuState): { rate: number } | undefined {
61-
if (resolveOptionalControlLabel(this.#props.label, state) !== undefined) return undefined;
63+
if (this.#customLabel.resolve(this.#props.label, state) !== undefined) return undefined;
6264
return { rate: state.rate };
6365
}
6466

packages/core/src/core/ui/resolve-optional-control-label.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,26 @@ export function resolveOptionalControlLabel<State>(
1717
if (label) return label;
1818
return undefined;
1919
}
20+
21+
/** Per-instance cache so `getLabel` / `getLabelParams` share one resolution per state snapshot. */
22+
export function createOptionalControlLabelCache<State>(): {
23+
resolve(
24+
label: TranslationKeyOrString | ((state: State) => TranslationKeyOrString) | undefined,
25+
state: State
26+
): TranslationKeyOrString | undefined;
27+
invalidate(): void;
28+
} {
29+
let cache: { state: State; custom: TranslationKeyOrString | undefined } | null = null;
30+
31+
return {
32+
resolve(label, state) {
33+
if (cache?.state !== state) {
34+
cache = { state, custom: resolveOptionalControlLabel(label, state) };
35+
}
36+
return cache.custom;
37+
},
38+
invalidate() {
39+
cache = null;
40+
},
41+
};
42+
}

packages/core/src/core/ui/seek-button/seek-button-core.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { defaults } from '@videojs/utils/object';
33
import type { NonNullableObject } from '@videojs/utils/types';
44

55
import type { MediaTimeState } from '../../media/state';
6-
import { resolveOptionalControlLabel } from '../resolve-optional-control-label';
6+
import { createOptionalControlLabelCache } from '../resolve-optional-control-label';
77
import type { ButtonState, TranslationKeyOrString } from '../types';
88

99
export interface SeekButtonProps {
@@ -39,24 +39,26 @@ export class SeekButtonCore {
3939

4040
#props = { ...SeekButtonCore.defaultProps };
4141
#media: MediaTimeState | null = null;
42+
readonly #customLabel = createOptionalControlLabelCache<SeekButtonState>();
4243

4344
constructor(props?: SeekButtonProps) {
4445
if (props) this.setProps(props);
4546
}
4647

4748
setProps(props: SeekButtonProps): void {
4849
this.#props = defaults(props, SeekButtonCore.defaultProps);
50+
this.#customLabel.invalidate();
4951
}
5052

5153
getLabel(state: SeekButtonState): TranslationKeyOrString {
52-
const custom = resolveOptionalControlLabel(this.#props.label, state);
54+
const custom = this.#customLabel.resolve(this.#props.label, state);
5355
if (custom !== undefined) return custom;
5456

5557
return state.direction === 'backward' ? 'seekBackwardSeconds' : 'seekForwardSeconds';
5658
}
5759

5860
getLabelParams(state: SeekButtonState): { seconds: number } | undefined {
59-
if (resolveOptionalControlLabel(this.#props.label, state) !== undefined) return undefined;
61+
if (this.#customLabel.resolve(this.#props.label, state) !== undefined) return undefined;
6062
return { seconds: Math.abs(this.#props.seconds) };
6163
}
6264

packages/core/src/core/ui/seek-button/tests/seek-button-core.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,22 @@ describe('SeekButtonCore', () => {
124124
const core = new SeekButtonCore({ label: 'Skip' });
125125
expect(core.getLabelParams(createState())).toBeUndefined();
126126
});
127+
128+
it('shares one label callback invocation with getLabel for the same state', () => {
129+
let calls = 0;
130+
const core = new SeekButtonCore({
131+
seconds: 30,
132+
label: () => {
133+
calls += 1;
134+
return calls === 1 ? 'custom' : '';
135+
},
136+
});
137+
const state = createState({ direction: 'forward' });
138+
139+
expect(core.getLabel(state)).toBe('custom');
140+
expect(core.getLabelParams(state)).toBeUndefined();
141+
expect(calls).toBe(1);
142+
});
127143
});
128144

129145
describe('getAttrs', () => {

packages/core/src/core/ui/tests/resolve-optional-control-label.test.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, it } from 'vitest';
22

3-
import { resolveOptionalControlLabel } from '../resolve-optional-control-label';
3+
import { createOptionalControlLabelCache, resolveOptionalControlLabel } from '../resolve-optional-control-label';
44

55
describe('resolveOptionalControlLabel', () => {
66
const state = { x: 1 as const };
@@ -26,3 +26,28 @@ describe('resolveOptionalControlLabel', () => {
2626
expect(resolveOptionalControlLabel(() => '', state)).toBeUndefined();
2727
});
2828
});
29+
30+
describe('createOptionalControlLabelCache', () => {
31+
const state = { x: 1 as const };
32+
33+
it('reuses resolution for the same state snapshot', () => {
34+
let calls = 0;
35+
const cache = createOptionalControlLabelCache<typeof state>();
36+
const label = () => {
37+
calls += 1;
38+
return calls === 1 ? 'first' : '';
39+
};
40+
41+
expect(cache.resolve(label, state)).toBe('first');
42+
expect(cache.resolve(label, state)).toBe('first');
43+
expect(calls).toBe(1);
44+
});
45+
46+
it('invalidates cached resolution', () => {
47+
const cache = createOptionalControlLabelCache<typeof state>();
48+
49+
expect(cache.resolve('A', state)).toBe('A');
50+
cache.invalidate();
51+
expect(cache.resolve('B', state)).toBe('B');
52+
});
53+
});

packages/core/src/core/ui/time-slider/time-slider-core.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export interface TimeSliderProps extends SliderProps {
1515
max?: number | undefined;
1616
/** Leading+trailing throttle (ms) for `onValueChange` during drag. */
1717
changeThrottle?: number | undefined;
18-
/** Options for {@link formatDuration} when building the slider thumb's `aria-valuetext`. */
18+
/** Options for `formatDuration` when building the slider thumb `aria-valuetext`. */
1919
formatOptions?: TimeFormatOptions | undefined;
2020
}
2121

@@ -83,7 +83,7 @@ export class TimeSliderCore extends SliderCore {
8383
const formatOptions = this.#props.formatOptions;
8484
const currentPhrase = formatDuration(announceValue, formatOptions);
8585
const durationPhrase = formatDuration(state.duration, formatOptions);
86-
const valuetext = durationPhrase ? `${currentPhrase} of ${durationPhrase}` : currentPhrase;
86+
const valuetext = Number.isFinite(state.duration) ? `${currentPhrase} of ${durationPhrase}` : currentPhrase;
8787

8888
return {
8989
...base,

packages/core/src/core/ui/time/time-core.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ export interface TimeProps {
1616
negativeSign?: string | undefined;
1717
/** Custom label for accessibility. */
1818
label?: TranslationKeyOrString | ((state: TimeState) => TranslationKeyOrString) | undefined;
19-
/** Options for {@link formatDuration} when building {@link TimeState#phrase} and screen reader text (not clock digits). */
19+
/** Options for `formatDuration` when building spoken-duration copy (`phrase` state and screen readers), not digital clock text. */
2020
formatOptions?: TimeFormatOptions | undefined;
2121
}
2222

packages/utils/src/time/format.ts

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -131,21 +131,6 @@ export function secondsToIsoDuration(seconds: number): string {
131131
return duration;
132132
}
133133

134-
function toDurationRecord(totalSeconds: number): Record<'hours' | 'minutes' | 'seconds', number> {
135-
const sec = Math.floor(Math.abs(totalSeconds));
136-
const hours = Math.floor(sec / 3600);
137-
const minutes = Math.floor((sec % 3600) / 60);
138-
const seconds = sec % 60;
139-
140-
const record = { hours: 0, minutes: 0, seconds: 0 };
141-
if (hours > 0) record.hours = hours;
142-
if (minutes > 0) record.minutes = minutes;
143-
if (seconds > 0 || (hours === 0 && minutes === 0)) {
144-
record.seconds = seconds;
145-
}
146-
return record;
147-
}
148-
149134
/**
150135
* Human-readable duration using {@link Intl.DurationFormat} when available.
151136
*
@@ -159,11 +144,15 @@ export function formatDuration(seconds: number, options?: TimeFormatOptions): st
159144

160145
const negative = seconds < 0;
161146
const positiveSeconds = Math.abs(seconds);
162-
const { hours, minutes, seconds: sec } = toDurationRecord(positiveSeconds);
147+
const totalSeconds = Math.floor(positiveSeconds);
148+
const hours = Math.floor(totalSeconds / 3600);
149+
const minutes = Math.floor((totalSeconds % 3600) / 60);
150+
const secondsPart = totalSeconds % 60;
151+
163152
const record: Partial<{ hours: number; minutes: number; seconds: number }> = {};
164153
if (hours > 0) record.hours = hours;
165154
if (minutes > 0) record.minutes = minutes;
166-
if (sec > 0 || (hours === 0 && minutes === 0)) record.seconds = sec;
155+
if (secondsPart > 0 || (hours === 0 && minutes === 0)) record.seconds = secondsPart;
167156

168157
let body: string;
169158
type DurationFormatConstructor = new (
@@ -185,6 +174,11 @@ export function formatDuration(seconds: number, options?: TimeFormatOptions): st
185174
body = formatTimeAsPhrase(positiveSeconds);
186175
}
187176

177+
// Some ICU builds return an empty string for a zero-length duration; fall back to the phrase formatter.
178+
if (!body.trim()) {
179+
body = formatTimeAsPhrase(positiveSeconds);
180+
}
181+
188182
if (negative) {
189183
const suffix = options?.translate?.('remaining') ?? 'remaining';
190184
return `${body} ${suffix}`;

0 commit comments

Comments
 (0)