-
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
Add new lines in TTML cues with nested <br> tags #584
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -216,6 +207,25 @@ shaka.media.TtmlTextParser.getLeafNodes_ = function(element) { | |
}; | ||
|
||
|
||
/** | ||
* Insert \n where <br> tags are found | ||
* | ||
* @param {!Node} element | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. {!Element} for consistency (instead of Node) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had that but it failed. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We solved this elsewhere in with an assertion and a cast:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, thanks, I appreciate the explanation. Will update. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
* | ||
|
@@ -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); | ||
|
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.
Let's rephrase to something like "Replaces
tags with /m characters"
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.
Strictly speaking it's not actually replacing tags. How about "Insert '\n' characters into
tags"
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.
Current comment looks good to me. Seems accurate.