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

WebVTT::Builder: Add logic to escape special chars #4414

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

syeopite
Copy link
Member

No description provided.

@syeopite syeopite requested a review from a team as a code owner January 30, 2024 23:27
@syeopite syeopite requested review from unixfox and removed request for a team January 30, 2024 23:27
@syeopite syeopite added the need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something label Jan 30, 2024
Copy link
Member Author

@syeopite syeopite left a comment

Choose a reason for hiding this comment

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

WebVTT does allow some tags in the cue payload in some circumstances while this PR just blindly escapes everything. https://developer.mozilla.org/en-US/docs/Web/API/WebVTT_API#cue_payload_text_tags

Considering how WebVTT::Builder is used in Invidious though, I'm not entirely sure if that's necessary to implement in Invidious

@Fijxu
Copy link
Contributor

Fijxu commented Feb 12, 2024

Testing on https://debuginv.nadeko.net
Seems to work well. https://debuginv.nadeko.net/api/v1/captions/XlbPnihCM0E?label=English%20(auto-generated)
https://debuginv.nadeko.net/watch?v=XlbPnihCM0E

@SamantazFox SamantazFox added ready and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Feb 16, 2024
@SamantazFox SamantazFox merged commit 962ce23 into iv-org:master Feb 18, 2024
7 checks passed
@SamantazFox
Copy link
Member

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants