-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 for TTML text alignment #573
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e973913
Fix to handle TTML textAlign
birme ba7b5f0
No need to use a function for the lineALign conversion. Use const ins…
birme d53ed72
Fix incorrect return type for lineAlignConvTable
birme f8a65a9
Handle center alignment
birme b1ef085
Merged the two align tests into one with two calls to verifyHelper in…
birme e698ca9
Renamed the linealign mapping table to textAlignToLineAlign
birme File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, Jonas, I missed this when I reviewed: what is the rationale between setting position if textAlign is center?
I might have to roll this back for now because it messes with Safari and Firefox tests (these browsers don't support 'auto' value for VTTCue.position), but I can totally address this and reintroduce setting position if there's a valid reason to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was that cue.align on Chrome does not support setting it as "center" though it should be valid. And using cue.align as "middle" doesn't have any effect without setting cue.position as auto. According to spec "auto" or a number should be a valid setting. If you can get it to center it on Chrome in another way I am happy to understand how. What happens without cue.position=auto is that the text is centered but on the left side of the screen. The most correct way probably but not supported from what I can see is to use VTTRegion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you. Let me file some bugs :)
Do I understand the problem with Chrome correctly: cue.align = 'center' is not supported and setting cue.align = 'middle' does not result in text being positioned in the middle of the cue box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! (And fixed the tests)
Links to the bugs for references:
http://crbug.com/663797
https://bugzilla.mozilla.org/1316134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A dev from Chrome got back to me asking for a vtt sample got the 'middle' bug. They're about to change it to 'center', but are having hard time reproducing the case where it doesn't position text in the center. Do you think you could share a sample that has the problem?
You can email it to me at ismena@google.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that they could confirm the cue.align issue. When thinking about this a bit more the cue.position might be working as intended however the difficult thing is that cue.position has a centered anchor when cue.align is middle/centered and that might very well be according to spec. And when cue.align is left aligned the cue.position anchor is to the left. In TTML they have separated this where you can define a cue box region with a size and position. For example position 2 would put the region 2 units from the left. And then having textAlign=centered would place the text centered within that box. So, when translating this from a TTML space to a VTT space I had to set position as "auto" to give the same effect.
If I have understood the vtt spec a similar concept is specified with VTTRegion however that is not yet supported in Chrome as far as I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, forgot to answer the sample request. I don't think it will be possible to share due to drm and geoblocking issues. However if you for example set cue.align=middle and cue.position=2 I believe you would get the same effect as some TTML would produce. Again, not necessarily that part is a bug in Chrome and maybe how it is intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I believe you're right and that might be the intended behavior (cue.position governs the position of the cue itself while align is about positioning the text inside the cue).
Thanks. I'll work with the Chrome team on fixing the align='center' issue.