Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed subtitles styling in firefox. #5737

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

venkat-karasani
Copy link
Contributor

@venkat-karasani venkat-karasani commented Jun 27, 2024

Changes
There are two approaches to fix this:

  1. Use ::cue to style the subtitles in Firefox. For this approach, except font-size everything else are working fine.
  2. Create a custom subtitles element. All styles are working fine.

Code for both of these approaches is already present, went with the custom subtitles element approach.

Issues
Fixes #5602

@dmitrylyzo
Copy link
Contributor

For reference, this was changed in #3897.

In Firefox 126, font family and shadow Uniform work, text size is much smaller than it should be.

subs-1
subs-2

It looks like it is now required to apply 170% to the video element.
subs-3

But it doesn't seem to work well - Normal size looks weird.
subs-4

@venkat-karasani
Copy link
Contributor Author

venkat-karasani commented Jun 27, 2024

Thanks for sharing the information.

setCueAppearance is responsible for rendering the subtitles natively and it is not being called because of firefox related checks inside enableNativeTrackSupport. Even if we change it, we get the issue you mentioned i.e. subtitle font-size is very small in firefox when compared to other browsers. So, using custom subtitle element should be better.

@thornbill thornbill added the bug Something isn't working label Jul 11, 2024
@thornbill
Copy link
Member

I wonder if this deserves an option in the subtitle settings to use native styling. It seems like subtitle styling is becoming more of an issue over time.

@venkat-karasani
Copy link
Contributor Author

I wonder if this deserves an option in the subtitle settings to use native styling. It seems like subtitle styling is becoming more of an issue over time.

yeah, It might be better to add an option to select between native or custom subtitle styling. Should I go ahead and try implementing it?

@thornbill
Copy link
Member

Sure go for it!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/components/subtitlesettings/subtitlesettings.js Outdated Show resolved Hide resolved
src/components/subtitlesettings/subtitlesettings.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
@venkat-karasani
Copy link
Contributor Author

venkat-karasani commented Sep 19, 2024

@dmitrylyzo

As suggested, we have three modes now i.e. Auto, Native and Custom. Auto will be the default.
The current implementation will remember the styling options per device not per user i.e.

  1. same user on multiple devices i.e. chrome or firefox can have different options as per their selection.
  2. All the users using same firefox brower or any other same entry point will have same option.

Is this fine or If you want me to try other approach, I am happy to implement that as well.

src/strings/en-us.json Outdated Show resolved Hide resolved
src/components/subtitlesettings/subtitlesettings.js Outdated Show resolved Hide resolved
src/components/subtitlesettings/subtitlesettings.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Show resolved Hide resolved
Copy link

sonarcloud bot commented Sep 20, 2024

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Sep 20, 2024

Cloudflare Pages deployment

Latest commit d292695
Status ✅ Deployed!
Preview URL https://8517e9fb.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@JamsRepos
Copy link

JamsRepos commented Nov 10, 2024

@venkat-karasani Would really appreciate some movement on this :)

@Brramble
Copy link

@dmitrylyzo seems like the PR creator is MIA, can you please resolve and merge?

@venkat-karasani
Copy link
Contributor Author

resolved the merge conflict.

src/strings/en-us.json Outdated Show resolved Hide resolved
src/components/subtitlesettings/subtitlesettings.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

Works as intended.
Recommend using "Squash and merge".

Copy link

sonarcloud bot commented Dec 1, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subtitles have a different shadow than my settings
6 participants