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.

Sign up for GitHub

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

Jump to bottom

Add new lines in TTML cues with nested <br> tags #584

Merged
merged 3 commits into from
Dec 6, 2016
Merged

Add new lines in TTML cues with nested <br> tags #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8388f74
Add new lines in TTML cues with nested <br> tags
Nov 11, 2016
425e7c0
Combine ttml text parser newline tests into one
Nov 16, 2016
8ecfe7e
Refactor ttml text parser conditional statement
Nov 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions lib/media/ttml_text_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,6 @@ shaka.media.TtmlTextParser.getLeafNodes_ = function(element) {
return result;

var childNodes = element.childNodes;
if (element.nodeName == 'p') {
// Replace <br> inside a <p> paragraph with a newline character.
// The <br> node is later on skipped
for (var j = 0; j < childNodes.length; j++) {
if (childNodes[j].nodeName == 'br' && j > 0) {
childNodes[j - 1].textContent += '\n';
}
}
}
for (var i = 0; i < childNodes.length; i++) {
// Currently we don't support styles applicable to span
// elements, so they are ignored
Expand All @@ -216,6 +207,25 @@ shaka.media.TtmlTextParser.getLeafNodes_ = function(element) {
};


/**
* Insert \n where <br> tags are found
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rephrase to something like "Replaces
tags with /m characters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking it's not actually replacing tags. How about "Insert '\n' characters into
tags"

Copy link
Member

Choose a reason for hiding this comment

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

Current comment looks good to me. Seems accurate.

*
* @param {!Node} element
Copy link
Contributor

Choose a reason for hiding this comment

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

{!Element} for consistency (instead of Node)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that but it failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The build failed? Ok, please leave a TODO above the param declaration (line 212) that having param Element here gives build problems and I'll look at it when we merge the PR.

Copy link
Member

Choose a reason for hiding this comment

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

At the call site, the type of cueElement is !Element, so using the same here should work. What was the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just re ran the build and looked more closely. The issue is that because this function calls itself to drill down and find nested tags, the nested nodes are not guaranteed to be elements. Initial call is always !Element but subsequent calls are not guaranteed.

/Users/sanbornh/Documents/shaka-player/lib/media/ttml_text_parser.js:223: ERROR - actual parameter 1 of shaka.media.TtmlTextParser.addNewLines_ does not match formal parameter
found   : Node
required: Element
      shaka.media.TtmlTextParser.addNewLines_(childNodes[i]);

Copy link
Member

Choose a reason for hiding this comment

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

We solved this elsewhere in with an assertion and a cast:

if (childNodes[i].nodeType == Node.ELEMENT_NODE && ...) {
  goog.asserts.assert(childNodes[i] instanceof Element,
                      'Node should be Element!');
  var leafChildren = shaka.media.TtmlTextParser.getLeafNodes_(
      /** @type {Element} */(childNodes[i]));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with the problem we are attempting to solve here. Can you describe why it is important to be using elements instead of nodes?

Copy link
Member

Choose a reason for hiding this comment

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

There's no runtime difference. It's just a matter of describing the types accurately and asserting to the compiler that it is that type.

Element is a subclass of Node, but childNodes contains all Nodes, even those that are not Elements. If we only want to walk through the Elements, we could use children, but some older browsers won't support that. Instead we use childNodes, and we have to both filter out the Elements by nodeType, and we have to tell the compiler that this is a safe conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks, I appreciate the explanation. Will update.

Copy link
Member

Choose a reason for hiding this comment

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

No problem. I'm sorry that the compiler is a bit of a burden in this instance. It's not perfect by any means, but I think on balance, it has helped to catch and prevent a lot of bugs.

* @private
*/
shaka.media.TtmlTextParser.addNewLines_ = function(element) {
var childNodes = element.childNodes;

for (var i = 0; i < childNodes.length; i++) {
if (childNodes[i].nodeName == 'br' && i > 0) {
childNodes[i - 1].textContent += '\n';
} else if (childNodes[i].childNodes.length > 0) {
shaka.media.TtmlTextParser.addNewLines_(childNodes[i]);
}
}
};


/**
* Parses an Element into a TextTrackCue or VTTCue.
*
Expand All @@ -239,6 +249,8 @@ shaka.media.TtmlTextParser.parseCue_ = function(
cueElement.textContent == '')
return null;

shaka.media.TtmlTextParser.addNewLines_(cueElement);

// Get time
var start = shaka.media.TtmlTextParser.parseTime_(
cueElement.getAttribute('begin'), rateInfo);
Expand Down
8 changes: 7 additions & 1 deletion test/media/ttml_text_parser_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,13 +386,19 @@ describe('TtmlTextParser', function() {
'</tt>');
});

it('inserts a newline on br in a p block', function() {
it('inserts newline characters into <br> tags', function() {
verifyHelper(
[
{start: 62.05, end: 3723.2, text: 'Line1\nLine2'}
],
'<tt><body><p begin="01:02.05" ' +
'end="01:02:03.200">Line1<br/>Line2</p></body></tt>');
verifyHelper(
[
{start: 62.05, end: 3723.2, text: 'Line1\nLine2'}
],
'<tt><body><p begin="01:02.05" ' +
'end="01:02:03.200"><span>Line1<br/>Line2</span></p></body></tt>');
});

it('parses cue alignment from textAlign attribute', function() {
Expand Down