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

fix(WebVTT): Fix voices with styles and support to multiple styles #4922

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

avelad
Copy link
Member

@avelad avelad commented Jan 20, 2023

No description provided.

@avelad avelad added type: bug Something isn't working correctly component: WebVTT The issue involves WebVTT subtitles specifically priority: P2 Smaller impact or easy workaround labels Jan 20, 2023
@avelad avelad added this to the v4.4 milestone Jan 20, 2023
@avelad avelad requested review from theodab and joeyparrish January 20, 2023 11:13
@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2023

Incremental code coverage: 68.63%

propertyLines = payload[1].split(';');
for (const styleBlock of styleBlocks) {
let styleSelector = 'global';
// Look for what is within parentisesis. For example:
Copy link
Member

Choose a reason for hiding this comment

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

(old) typo: parentheses

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

cue.fontSize = value;
break;
case 'font-weight':
if (parseInt(value, 10) >= 700) {
Copy link
Member

Choose a reason for hiding this comment

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

What about non-integer values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added support to bold

cue.fontStyle = shaka.text.Cue.fontStyle.ITALIC;
break;
case 'oblique':
cue.backgroundColor = value;
Copy link
Member

Choose a reason for hiding this comment

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

This test case uses "background" instead of "background-color". Could you support that, too, even just making 'background' an alias to 'background-color' for now?

WEBVTT

STYLE
*|*::cue(b) {
  background: blue
}
|*::cue(i) {
  color: blue;
}
::cue(i) {
  background: white;
}
*::cue(b) {
  color: white;
}
::cue {
  font-size: 11px;
}

00:00:00.000 --> 00:00:05.000
This <i>is</i> a <b>test</b> subtitle

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

@avelad avelad requested a review from joeyparrish January 24, 2023 06:43
@avelad avelad merged commit 68968c1 into shaka-project:main Jan 24, 2023
@avelad avelad deleted the webvtt-voices-with-styles branch January 24, 2023 18:54
joeyparrish pushed a commit that referenced this pull request Jan 30, 2023
joeyparrish pushed a commit that referenced this pull request Jan 30, 2023
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: WebVTT The issue involves WebVTT subtitles specifically priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants