🐛Propagate <track> elements in amp-story #13751
Conversation
|
Thanks for your pull request. t looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
extensions/amp-story/0.1/sources.js
Outdated
| @@ -48,6 +50,31 @@ export class Sources { | |||
| element.setAttribute('src', this.srcAttr_); | |||
| } | |||
|
|
|||
| // Wait for "loadedmetadata" before adding tracks | |||
| // Firefox issue workaround | |||
There was a problem hiding this comment.
Can you add more insight on what the issue is in Firefox?
extensions/amp-story/0.1/sources.js
Outdated
| @@ -48,6 +50,31 @@ export class Sources { | |||
| element.setAttribute('src', this.srcAttr_); | |||
| } | |||
|
|
|||
| // Wait for "loadedmetadata" before adding tracks | |||
| // Firefox issue workaround | |||
| const addTracksHandler = () => { | |||
There was a problem hiding this comment.
Would be good to move this up outside of the class so that it's only declared once, rather than on every invocation of applyToElement(...)
There was a problem hiding this comment.
This function uses this.trackEls to apply tracks, but inner forEach call can be moved outside to its own method in the class
extensions/amp-story/0.1/sources.js
Outdated
| this.applyTracksToElement_(element); | ||
| }; | ||
|
|
||
| element.addEventListener('loadedmetadata', addTracksHandler); |
There was a problem hiding this comment.
I think this might be a race condition? What happens if the loadedmetadata event is fired before applyTracksToElement_ is called?
Maybe you can check first, like:
if (element.readyState >= 1 /* HAVE_METADATA */) {
this.applyTracksToElement_(element);
} else {
element.addEventListener('loadedmetadata', addTracksHandler);
}There was a problem hiding this comment.
nice catch, thank you!
extensions/amp-story/0.1/sources.js
Outdated
| element.addEventListener('loadedmetadata', addTracksHandler); | ||
| } | ||
| } | ||
|
|
||
| Array.prototype.forEach.call(this.srcEls_, |
There was a problem hiding this comment.
nit: can you move this above the tracks block, to keep it together with the logic for the src attribute?
a1719fa to
4c14db8
Compare
4c14db8 to
a4c5844
Compare
|
@googlebot I signed it! |
5d6f589 to
528839d
Compare
|
CLAs look good, thanks! |
528839d to
a4c5844
Compare
🐛Propagate <track> elements in amp-story
Work-in-progress attempt to propagate amp-video
<track>tags to enable subtitles in stories.Should resolve #13698.
Simply moving existing
<track>(s) doesn't seem to work, so this code creates new elements and adds those to the video.In addition it has to wait for
loadedmetadatafor subtitles to show in Firefox.Tested on:
Desktop: Chrome, Safari, Firefox
Mobile: Safari
I'd like some feedback if this approach makes sense, before adding tests/examples etc.