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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Propagate <track> elements in amp-story #13751

Merged
merged 1 commit into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@iefserge
Contributor

iefserge commented Mar 1, 2018

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 loadedmetadata for 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.

@googlebot

This comment has been minimized.

googlebot commented Mar 1, 2018

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
  • Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
  • The email used to register you as an authorized contributor must also be attached to your GitHub account.

@googlebot googlebot added the cla: no label Mar 1, 2018

@@ -48,6 +50,31 @@ export class Sources {
element.setAttribute('src', this.srcAttr_);
}
// Wait for "loadedmetadata" before adding tracks
// Firefox issue workaround

This comment has been minimized.

@newmuis

newmuis Mar 2, 2018

Collaborator

Can you add more insight on what the issue is in Firefox?

This comment has been minimized.

@iefserge

iefserge Mar 2, 2018

Contributor

yep sure

@@ -48,6 +50,31 @@ export class Sources {
element.setAttribute('src', this.srcAttr_);
}
// Wait for "loadedmetadata" before adding tracks
// Firefox issue workaround
const addTracksHandler = () => {

This comment has been minimized.

@newmuis

newmuis Mar 2, 2018

Collaborator

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(...)

This comment has been minimized.

@iefserge

iefserge Mar 2, 2018

Contributor

This function uses this.trackEls to apply tracks, but inner forEach call can be moved outside to its own method in the class

this.applyTracksToElement_(element);
};
element.addEventListener('loadedmetadata', addTracksHandler);

This comment has been minimized.

@newmuis

newmuis Mar 2, 2018

Collaborator

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);
}

This comment has been minimized.

@iefserge

iefserge Mar 2, 2018

Contributor

nice catch, thank you!

@newmuis

newmuis approved these changes Mar 2, 2018

element.addEventListener('loadedmetadata', addTracksHandler);
}
}
Array.prototype.forEach.call(this.srcEls_,

This comment has been minimized.

@newmuis

newmuis Mar 2, 2018

Collaborator

nit: can you move this above the tracks block, to keep it together with the logic for the src attribute?

This comment has been minimized.

@iefserge

iefserge Mar 2, 2018

Contributor

ok

@iefserge

This comment has been minimized.

Contributor

iefserge commented Mar 2, 2018

@googlebot I signed it!

@googlebot

This comment has been minimized.

googlebot commented Mar 5, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Mar 5, 2018

@newmuis newmuis changed the title from 馃悰Propagate <track> elements in amp-story [wip] to 馃悰Propagate <track> elements in amp-story Mar 5, 2018

@newmuis newmuis merged commit a522b64 into ampproject:master Mar 5, 2018

4 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: JavaScript No alert changes
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details

RanAbram added a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

Propagate <track> elements in amp-story (ampproject#13751)
馃悰Propagate <track> elements in amp-story
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment