feat(packages): add chaptered time sliders - #2043
Conversation
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 Bundle Size Report🎨 @videojs/html
Small changes (35, ≤ 300 B)
Presets (7)
Media (13)
Players (5)
Skins (30)
UI Components (41)
⚛️ @videojs/react
Small changes (36, ≤ 300 B)
Presets (7)
Media (12)
Skins (27)
UI Components (33)
🧩 @videojs/core
Small changes (1, ≤ 300 B)
Entries (76)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — 2 small size changes
Entries (13)
📦 @videojs/jsx — no changesEntries (3)
📦 @videojs/media — no changesEntries (15)
📦 @videojs/spf — no changesEntries (7)
ℹ️ How to interpretEach entry is independently bundled, minified, and brotli-compressed. Initial size includes its static import graph; lazy dynamic chunks are reported separately. Entries are not additive because their dependency graphs overlap. Preset rows represent realistic combined bundles. Changes of 300 B or less across initial, lazy, and total size are collapsed, not discarded. Run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b105422. Configure here.
b105422 to
5126230
Compare
| </TimeSlider.Track> | ||
| <TimeSlider.Chapters | ||
| className={slider.chapters} | ||
| renderChapter={(props) => ( |
There was a problem hiding this comment.
I kept it renderChapter as it's not really rendering the element for TimeSlider.Chapters itself but I can change that if you preferred.
mihar-22
left a comment
mihar-22
left a comment
There was a problem hiding this comment.
Overall this is looking really good 😎 Great work! Thank you for taking in all the feedback and finding a workaround on the single track issue.
I did a AI review locally and it came up with these findings - might be worth having your agent review:
Core Assessment
Core ownership is directionally correct:
- Text-track discovery remains in the existing DOM store feature.
- Cue normalization and generic segment geometry are framework-neutral.
- React and HTML use the same data attributes and CSS-variable names.
- Registration uses narrow
safeDefineentry points; classes do not self-register. - The segment collection is decorative while the existing slider thumb retains the interactive ARIA contract.
Core is not yet doing enough of the feature derivation. HTML and React still independently own chapter lookup, buffer localization, fallback rules, and normalization scheduling. In HTML, both the segment collection and title renormalize on slider updates, including pointer movement.
I would make TimeSliderChaptersCore own:
- normalized ranges and
hasChapters; - raw pointer-to-chapter lookup;
- local fill/buffer calculation;
- the final per-chapter state.
The platform layers could then limit themselves to store subscriptions and rendering. That would remove the React root-state workaround and make HTML/React parity easier to enforce.
The public documentation also needs a chaptered anatomy/example. The current [TimeSlider reference](
v10/site/src/content/docs/reference/time-slider.mdx
Lines 24 to 46 in 5126230
Chapters, ChapterTitle, renderChapter, the HTML template/fallback contract, synthetic gaps, CSS variables, or data attributes.
Local AI Review
-
Medium — The final chapter disappears at the slider’s exact right edge
Both adapters publish
pointerValueusingvalueFromPercent(), which clamps and then rounds to the slider step. With the demo’s487.626sduration and default one-second step,100%becomes488, outside the normalized chapter domain. Consequently, neitherfindActive()norSliderSegmentsCoreconsiders the last chapter active.I reproduced this in the deployed HTML and React previews: hovering at 80% showed “Track Performance”; pressing End or pointing at the right edge produced an empty chapter title.
Relevant code: [slider-core.ts](
), [React context](v10/packages/core/src/core/ui/slider/slider-core.ts
Lines 145 to 154 in 5126230
), [HTML context](v10/packages/react/src/ui/time-slider/time-slider-root.tsx
Lines 120 to 125 in 5126230
).v10/packages/html/src/ui/time-slider/time-slider-element.ts
Lines 173 to 178 in 5126230
Smallest fix: clamp again after rounding. Better for chapter hit-testing: expose/use
rawValueFromPercent()so hover containment is continuous rather than quantized to the seek step. Add a test wheredurationis not divisible bystep. -
Medium — An invalid HTML template silently removes the entire track
The HTML component hides the fallback as soon as chapter cues exist, then silently skips a chapter if the template does not contain exactly one
HTMLElement. A template with two roots, an SVG root, or only text therefore results in no rendered chapters and no fallback.The one-HTMLElement constraint is also absent from the public JSDoc. Relevant ordering: [time-slider-chapters-element.ts](
).v10/packages/html/src/ui/time-slider/time-slider-chapters-element.ts
Lines 48 to 70 in 5126230
Smallest fix: validate the template once before hiding the fallback, retaining the fallback and emitting a development warning when invalid. Alternatively, support a fragment/multiple roots. Tests should cover missing, empty, multiple-root, and non-HTML templates.
-
Medium — React’s collection-level state is arbitrarily the first chapter
TimeSliderChaptersPropsexposesclassName,style, andrenderas functions ofTimeSliderChapterState, butSliderSegmentssupplies the state ofgeometry[0]to those collection-level callbacks.That means:
- The collection’s state always describes the first chapter.
- Moving playback into another chapter does not make the collection callback describe that chapter.
- The fallback path fabricates a pseudo chapter state.
- There is no documented way for consumers to know why this particular chapter was selected.
Relevant code: [public props](
), [first-segment selection](v10/packages/react/src/ui/time-slider/time-slider-chapters.tsx
Lines 18 to 29 in 5126230
).v10/packages/react/src/ui/time-slider/slider-segments.tsx
Lines 61 to 70 in 5126230
Smallest fix: give the root a collection-level state such as
{ hasChapters, chapters }, or use the parent slider state. ReserveTimeSliderChapterStateexclusively forrenderChapter. -
Low — Chapter information remains pointer-only and is not coherently exposed to keyboard/screen-reader users
The preview is revealed only by
data-pointing; keyboard focus and arrow/Home/End interaction do not reveal the chapter title. The slider’saria-valuetextcontinues to contain only time. Meanwhile, because the opacity-zero title is not hidden from accessibility APIs, the browser accessibility tree includes the preview title as detached child text—potentially stale at the default pointer position.Relevant CSS: [default slider.css](
). The title is givenv10/packages/skins/src/default/css/components/slider.css
Lines 117 to 184 in 5126230
aria-live="off"but no explicit association with the slider: [React title]().v10/packages/react/src/ui/time-slider/time-slider-chapter-title.tsx
Lines 43 to 50 in 5126230
Consider showing the preview on keyboard focus, resolving against current time when no pointer interaction exists, and deliberately including/associating the chapter title with the slider’s accessible value. Otherwise mark the visual preview hidden from accessibility APIs.
There was a problem hiding this comment.
we should move time slider chapters in a separate directory time-slider-chapters to follow conventions, separate component
There was a problem hiding this comment.
including the core.ts file too
There was a problem hiding this comment.
are you sure? it seems like media-time-slider is still the parent. or do you mean a new folder in the time-slider folder?
There was a problem hiding this comment.
i was thinking originally top-level, but maybe you're right this is directly tied to time-slider so a subdir would make more sense 👍
There was a problem hiding this comment.
Trying to differentiate between standard parts (track, fill, buffer, thumb) from opt-in stuff like chapters which are heavy
| /** CSS geometry and progress local to each chapter. */ | ||
| export const TimeSliderChapterCSSVars = { | ||
| start: '--media-slider-chapter-start', | ||
| end: '--media-slider-chapter-end', | ||
| width: '--media-slider-chapter-width', | ||
| fill: '--media-slider-chapter-fill', | ||
| buffer: '--media-slider-chapter-buffer', | ||
| } as const; |
There was a problem hiding this comment.
Separate file to follow conventions time-slider-chapters-css-vars.ts. I think we can do a follow up to simplify these names to not include the component name, so just data-attrs.ts, core.ts, and css-vars.ts
| export interface TimeSliderChapterRange extends SliderSegmentRange { | ||
| /** Authored chapter cue, or `null` for an uncovered interval. */ | ||
| cue: MediaTextCue | null; | ||
| } |
There was a problem hiding this comment.
maybe move to a sibling types.ts - not important
| className={typeof className === 'function' ? (state) => className(getChapterState(state)) : className} | ||
| style={typeof style === 'function' ? (state) => style(getChapterState(state)) : style} | ||
| render={ | ||
| typeof render === 'function' ? (renderProps, state) => render(renderProps, getChapterState(state)) : render | ||
| } |
There was a problem hiding this comment.
we have isFunction utility we can use here
There was a problem hiding this comment.
i dont know for sure but isn't this normally abstracted into core
There was a problem hiding this comment.
I've moved it to core 👍🏼
| @@ -0,0 +1,29 @@ | |||
| /** Finds the active item in a collection ordered by its start value. */ | |||
| export function findActive<Item>( | |||
There was a problem hiding this comment.
Name wasn't feeling clear, spiked with AI quickly to refactor into separate utils:
function findLastIndexAtOrBefore<Item>(
items: readonly Item[],
value: number,
getValue: (item: Item) => number
): number {
let low = 0;
let high = items.length - 1;
let index = -1;
while (low <= high) {
const mid = (low + high) >>> 1;
if (getValue(items[mid]!) <= value) {
index = mid;
low = mid + 1;
} else {
high = mid - 1;
}
}
return index;
}
export function findLastAtOrBefore<Item>(
items: readonly Item[],
value: number,
getValue: (item: Item) => number
): Item | undefined {
const index = findLastIndexAtOrBefore(items, value, getValue);
return index < 0 ? undefined : items[index];
}
export function findRangeAt<Range>(
ranges: readonly Range[],
value: number,
getStart: (range: Range) => number,
getEnd: (range: Range) => number
): Range | undefined {
const index = findLastIndexAtOrBefore(ranges, value, getStart);
if (index < 0) return undefined;
const range = ranges[index]!;
const end = getEnd(range);
const isLastRange = index === ranges.length - 1;
if (value < end || (isLastRange && value === end)) {
return range;
}
return undefined;
}
5126230 to
9ab3522
Compare
9ab3522 to
4965ec2
Compare
4965ec2 to
90fa7d8
Compare
90fa7d8 to
0f0359e
Compare
3 participants
Summary
Reworks the chaptered time slider based on the Notion review. It uses independently rendered HTML/CSS chapter segments rather than an SVG mask, while keeping the regular slider as a fallback until chapter cues are available.
Changes
Testing
pnpm -F @videojs/html test src/define/tests/registration.test.tspnpm -F @videojs/react test src/ui/time-slider/tests/time-slider.test.tsxpnpm -F @videojs/skins buildpnpm --dir apps/sandbox buildNote
Medium Risk
Touches the core time-slider interaction and Default/Minimal skins across HTML and React, so visual and seek UX regressions are possible. Changes are additive with a regular-track fallback until chapter cues load.
Overview
Adds chaptered time sliders that render independent HTML/CSS chapter segments instead of an SVG mask, with the regular track kept as a fallback until chapter cues are available.
Core introduces
SliderSegmentsCoreandTimeSliderChaptersCoreto normalize cues (gaps, overlaps, clamping), expose per-chapter geometry/fill/buffer state, and drive chapter titles at the pointer or keyboard position.HTML and React add
TimeSlider.Chapters/media-time-slider-chaptersplus chapter title parts, wired into Default and Minimal skins. Previews now show chapter titles alongside thumbnails/time, with highlight and gap styling.Sandbox and docs demos include a chapters VTT source (with a deliberate gap) so the new segmented seek bar can be exercised end-to-end.
Reviewed by Cursor Bugbot for commit 0f0359e. Bugbot is set up for automated code reviews on this repo. Configure here.