Skip to content

fix: update text-track-cue styles on useractive#9023

Merged
Essk merged 3 commits intovideojs:mainfrom
tsi:fix/subtitles-position
Apr 15, 2025
Merged

fix: update text-track-cue styles on useractive#9023
Essk merged 3 commits intovideojs:mainfrom
tsi:fix/subtitles-position

Conversation

@tsi
Copy link
Copy Markdown
Contributor

@tsi tsi commented Apr 6, 2025

Description

This is an attempt to fix #6207
Added event handlers for 'useractive' and 'userinactive' events to update text tracks display position so that they reposition when the control bar is shown or hidden

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.31%. Comparing base (c7298d4) to head (9789446).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9023      +/-   ##
==========================================
- Coverage   84.88%   84.31%   -0.58%     
==========================================
  Files         120      120              
  Lines        8152     8154       +2     
  Branches     1964     1964              
==========================================
- Hits         6920     6875      -45     
- Misses       1232     1279      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

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

Looks good. The jdsoc for updateDisplay()could be updated too.

@tsi
Copy link
Copy Markdown
Contributor Author

tsi commented Apr 8, 2025

Thanks @mister-ben, done.

Copy link
Copy Markdown
Contributor

@Essk Essk left a comment

Choose a reason for hiding this comment

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

LGTM :)

@Essk Essk merged commit 8e1889c into videojs:main Apr 15, 2025
10 of 11 checks passed
@jrochkind
Copy link
Copy Markdown

This is not in a release yet, correct? Sorry, I'm not sure how to figure out if a given PR is in a release yet!

@Essk
Copy link
Copy Markdown
Contributor

Essk commented Jun 4, 2025

@jrochkind

Yep it went out in v8.23.1 - you can can check the changelog to see what's been released:

* update text-track-cue styles on useractive ([#9023](https://github.com/videojs/video.js/issues/9023)) ([8e1889c](https://github.com/videojs/video.js/commit/8e1889c))

@jrochkind
Copy link
Copy Markdown

jrochkind commented Jun 4, 2025

Thank you!

Do you know why npm shows 8.22.0 as being latest release?

https://www.npmjs.com/package/video.js?activeTab=versions

That was part of my confusion, I was updating what npm thought was latest release, and not seeing the fix; I do see it when I explicitly update to actual latest 8.23.3. I wonder why npm does not know about this, so for instance yarn upgrade won't upgrade to it?

It looks like maybe 8.22.0 is tagged with "latest" in npm despite newer releases?

jrochkind added a commit to sciencehistory/scihist_digicoll that referenced this pull request Jun 4, 2025
Has a fix for subtitle display with control bar that we wanted. videojs/video.js#9023

Had to edit package.json instead of just running `yarn upgrade video.js` because for some reason npm believes 8.23 is a pre-release it won't update to unless specifically listed in package.json.
@Essk
Copy link
Copy Markdown
Contributor

Essk commented Jun 4, 2025

Ah, we usually promote a release from next to latest within a week or two, but it's a manual job and sometimes gets overlooked. I'll do it now.

@jrochkind
Copy link
Copy Markdown

jrochkind commented Jun 4, 2025

Thanks!

So this fix works well and I think improves the display.

However, there is an oddity -- when the control bar is fading out, the text track is repositioned immediately, as the control bar takes a while to fade out. So there are some moments where they are overlapping, and the text track is hard to read.

I wanted to have a reproducible example or at least screencap video for you, but ran into trouble trying to create a simple 'hello world' to demonstrate. I think it should be easily demonstratble though?

I imagine this was possible before too, if the fade out happened at exactly the wrong time with switcing cues? But was a lot less likely of a race condition, whereas now it's pretty much every time control bar fades out with a text track showing.

One work-around I might use locally is simply shortening or disabling the fade out transition.

@Essk
Copy link
Copy Markdown
Contributor

Essk commented Jun 5, 2025

Thanks @jrochkind! In order to keep things organised, this feedback should be raised in a new issue rather than a closed PR (although feel free to link back to this conversation and the original issue for context), and if you're able to create & link a minimal sample with steps to reproduce, that will make it more likely to be investigated.

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.

Subtitles gets hidden behind the control bar in Chrome, Firefox & Edge

4 participants