Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(text): Inherit alignment from regions.
The recent changes to TTML parsing, to not inherit regions,
inadvertently ended up breaking text alignment in situations
where a region with alignment was on the p or div above a span.
Previously, we only inherited the text and display alignment
from a region on leaf nodes... which was a problem, since we
also didn't apply any styles to text nodes.

Change-Id: I62ac155bc4310a5f7da52c10ca2dd434f8015c97
  • Loading branch information
theodab committed Jan 25, 2022
1 parent 771619f commit e9df8fb
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 9 deletions.
8 changes: 3 additions & 5 deletions lib/text/ttml_text_parser.js
Expand Up @@ -379,7 +379,6 @@ shaka.text.TtmlTextParser = class {
parentCueElement, 'region', regionElements, /* prefix= */ '')[0];
}


shaka.text.TtmlTextParser.addStyle_(
cue,
cueElement,
Expand Down Expand Up @@ -527,14 +526,13 @@ shaka.text.TtmlTextParser = class {
}

const align = TtmlTextParser.getStyleAttribute_(
cueElement, region, styles, 'textAlign', shouldInheritRegionStyles);
cueElement, region, styles, 'textAlign', true);
if (align) {
cue.positionAlign = TtmlTextParser.textAlignToPositionAlign_[align];
cue.lineAlign = TtmlTextParser.textAlignToLineAlign_[align];

goog.asserts.assert(align.toUpperCase() in Cue.textAlign,
align.toUpperCase() +
' Should be in Cue.textAlign values!');
align.toUpperCase() + ' Should be in Cue.textAlign values!');

cue.textAlign = Cue.textAlign[align.toUpperCase()];
} else {
Expand All @@ -543,7 +541,7 @@ shaka.text.TtmlTextParser = class {
}

const displayAlign = TtmlTextParser.getStyleAttribute_(
cueElement, region, styles, 'displayAlign', shouldInheritRegionStyles);
cueElement, region, styles, 'displayAlign', true);
if (displayAlign) {
goog.asserts.assert(displayAlign.toUpperCase() in Cue.displayAlign,
displayAlign.toUpperCase() +
Expand Down
41 changes: 37 additions & 4 deletions test/text/ttml_text_parser_unit.js
Expand Up @@ -1827,8 +1827,8 @@ describe('TtmlTextParser', () => {
// Styles from regionStyle should apply only to the nested cue.
backgroundColor: '',
color: '',
displayAlign: Cue.displayAlign.AFTER, // displayAlign default value.
textAlign: Cue.textAlign.START, // textAlign default value.
displayAlign: Cue.displayAlign.CENTER,
textAlign: Cue.textAlign.CENTER,

nestedCues: [
{
Expand All @@ -1853,7 +1853,7 @@ describe('TtmlTextParser', () => {
// Styles inherited from backgroundStyle via regionStyle via
// spanStyle
displayAlign: Cue.displayAlign.CENTER,
textAlign: Cue.textAlign.CENTER,
textAlign: Cue.textAlign.END,
},
],
},
Expand All @@ -1867,7 +1867,7 @@ describe('TtmlTextParser', () => {
// spanStyle inherits attributes from regionStyle
' <style xml:id="pStyle" tts:fontSize="15px" />' +
' <style xml:id="spanStyle" style="regionStyle" ' +
' tts:backgroundColor="white" />' +
' tts:backgroundColor="white" tts:textAlign="end" />' +
// regionStyle inherits attributes from backgroundStyle
' <style xml:id="regionStyle" style="backgroundStyle" ' +
' tts:backgroundColor="transparent" tts:color="blue" />' +
Expand All @@ -1886,6 +1886,39 @@ describe('TtmlTextParser', () => {
{startTime: 0, endTime: 60});
});

it('inherits alignment from parent regions', () => {
verifyHelper(
[
{
startTime: 0,
endTime: 60,
payload: '',
fontSize: '',
textAlign: Cue.textAlign.END,
displayAlign: Cue.displayAlign.CENTER,
nestedCues: [
{
startTime: 0,
endTime: 60,
payload: 'Hello!',
textAlign: Cue.textAlign.END,
displayAlign: Cue.displayAlign.CENTER,
},
],
},
],
'<tt xmlns:tts="http://www.w3.org/ns/ttml#styling">' +
'<head><layout>' +
'<region xml:id="r1" tts:textAlign="end" tts:displayAlign="center" />' +
'</layout></head>' +
'<body><div><p begin="00:00" end="01:00" region="r1">' +
'<span>Hello!</span>' +
'</p></div></body></tt>',
{periodStart: 0, segmentStart: 0, segmentEnd: 0},
{startTime: 0, endTime: 60},
);
});

// Regression test for https://github.com/google/shaka-player/issues/3743
it('inherits styles from other styles on nestedCues', () => {
verifyHelper(
Expand Down

0 comments on commit e9df8fb

Please sign in to comment.