Skip to content

fix(captions): size cue text by width on portrait players and let the box follow its content - #1088

Merged
tsi merged 10 commits into
masterfrom
fix/portrait-captions
Sep 16, 2026
Merged

tsi merged 10 commits into
masterfrom
fix/portrait-captions

Conversation

@tsi

@tsi tsi commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Captions on portrait (9:16) players rendered with a huge font, got cut off at the bottom of the player and jumped between cues; any font-size override made the text float in the middle instead. Reported for Tudor's training video (VIDEO-21222).

Root cause

  • vtt.js sets the cue font to 5% of the display height. At 390px width that is ~11px for 16:9 but ~35px for 9:16.
  • video.js 8 measures the cue and pixel-fixes its box (top/height/width) in WebVTT.processCues and only afterwards adds the vjs-text-track-cue class. Every rule we key on that class (Inter, 700, padding, inline-block) re-wraps the text after the box is frozen, so it spills out of the bottom.
  • The same freeze means a font-size override (page CSS or styledTextTracks.fontSize) shrinks the text inside a still-tall box: the text floats and moves with each cue's line count.

Changes

  • text-tracks.scss: container-type: size on the caption display and font-size: min(5cqh, 3.5cqw) on cues. Landscape is unchanged (the 5cqh term is vtt.js's own size); portrait is capped at 3.5% of the width.
  • text-tracks.scss / styled-text-tracks.scss: height: auto; top: auto on bottom-gravity cues (bottom: auto; height: auto for top gravity) so the box follows its content, the same trick video.js uses for its own fontPercent setting. Center gravity untouched.
  • docs/subtitles-and-captions.html: new "Portrait (9:16) player" section.
  • New e2e: plays the portrait demo and asserts over 3+ cues that the text stays inside the caption display and the font is at most 4% of the player width. It fails on master with cue "…" spills below the caption display.

Notes

  • VTT/SRT cues with an explicit line: position from the top keep their computed bottom edge and grow upward; only visible if the text re-wraps. Accepted trade-off.
  • Browsers without container-query units (Chrome < 105, Safari < 16, Firefox < 110) ignore the min() declaration and keep today's size, but still get the content-following box.
  • styledTextTracks.fontSize keeps working; percentages are now relative to the responsive base.

Testing

  • npx playwright test … subtitlesAndCaptionsPage.spec.ts: 2/2 (existing + new)
  • npm run test:unit: 129 passed
  • npm run lint: clean
  • Manually checked the karaoke demo (top gravity, 200% font) grows downward, and that the caption follows the control bar when it shows/hides.

Follow-ups

  • Embed page (player.cloudinary.com) sets font-family: Inter with no fallback stack, so iOS renders Times until the webfont loads. Separate repo.
  • specs/portrait-captions/ (spec + decision log) is here for review and will be removed before merge.

tsi added 4 commits September 14, 2026 17:17
… box follow its content

vtt.js sets the cue font to 5% of the display height and pixel-fixes the cue
box before video.js adds the .vjs-text-track-cue class, so our caption styles
re-wrapped the text after the box was frozen. On portrait players this made
captions huge, cut off at the bottom and jumping between cues; any font-size
override left the text floating inside a too-tall box.

- cap the cue font at 3.5% of the display width (min(5cqh, 3.5cqw)); landscape
  size is unchanged
- height/top auto on bottom-gravity cues (bottom/height auto for top gravity)
  so the box follows the text, as video.js does for its own fontPercent

VIDEO-21222
@tsi
tsi requested a review from a team as a code owner September 15, 2026 13:57
@netlify

netlify Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for cld-video-player ready!

Name Link
🔨 Latest commit 8781575
🔍 Latest deploy log https://app.netlify.com/projects/cld-video-player/deploys/6aaa5fe443a2620008712b6b
😎 Deploy Preview https://deploy-preview-1088--cld-video-player.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for cld-vp-esm-pages ready!

Name Link
🔨 Latest commit 8781575
🔍 Latest deploy log https://app.netlify.com/projects/cld-vp-esm-pages/deploys/6aaa5fe4fbc14b00081a4241
😎 Deploy Preview https://deploy-preview-1088--cld-vp-esm-pages.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

tsi added 4 commits September 15, 2026 16:58
Cues are now bottom-anchored, so lifting the caption area from 5em to 1em
when the control-bar hides made captions move on hover. Keep the
control-bar offset while playing; only players without controls use 1em.

VIDEO-21222
@adimiz1

adimiz1 commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Non-blocking review notes:

  1. Native fullscreen scope check — the fix targets the JS overlay (.vjs-text-track-display / .vjs-text-track-cue). iOS native fullscreen renders captions through ::cue / ::-webkit-media-text-track-display instead, a separate pipeline, untouched here. Was the original repro (Tudor's video) in native fullscreen or inline/JS-controls view? If native fullscreen, this may not fully resolve the reported issue — worth confirming before closing VIDEO-21222.

  2. videojs-default theme behavior change, font-size: min(5cqh, 3.5cqw) !important now applies unconditionally, including the videojs-default theme, whose purpose is matching vanilla video.js styling. Portrait/narrow players on that theme will now diverge from stock video.js sizing. Seems like a deliberate, reasonable call, but worth a one-line mention in the PR description so it's not a surprise later.

Smaller/non-essential:

  • text-tracks.scss: the &.vjs-user-inactive.vjs-playing .vjs-text-track-display branch duplicated into the base selector block is redundant, the base .vjs-text-track-display selector already covers it. Could just drop that state from the old override block instead.
  • styledTextTracks.box (custom width/height on the display) + the new container-type: size combo isn't explicitly covered by the manual testing notes — probably fine (explicit dims win) but worth a quick manual check.
  • The new e2e polls up to 45s for 3 distinct live cues from the prod cloud asset — some flake/CI-time risk, and couples the docs demo + test to that asset's captions never changing.

None of these block merge in my view — just flagging for awareness / possible follow-up.

Comment thread specs/portrait-captions/log.md Outdated
@@ -0,0 +1,70 @@
# Decision log — Portrait captions fix (VIDEO-21222)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont forget to remove before merge

Comment thread specs/portrait-captions/spec.md Outdated
@@ -0,0 +1,80 @@
# Feature Specification: Captions on portrait players

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont forget to remove before merge

@tsi
tsi merged commit 14b10d2 into master Sep 16, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants