✨ [Story system-layer] Add CC Icon that toggles captions #37884
Conversation
|
Hey @gmajoulet, @newmuis! These files were changed: |
|
@gmajoulet for |
| @@ -569,6 +585,8 @@ export class AmpStoryStoreService { | |||
| [StateProperty.MUTED_STATE]: true, | |||
| [StateProperty.PAGE_ATTACHMENT_STATE]: false, | |||
| [StateProperty.PAGE_HAS_AUDIO_STATE]: false, | |||
| [StateProperty.PAGE_HAS_CAPTIONS_STATE]: false, | |||
| [StateProperty.CAPTIONS_STATE]: true, | |||
There was a problem hiding this comment.
(following up from a conversation with @gmajoulet )
Captions are currently on by default. I think it makes sense to keep this field true so the expected UX does not change.
There was a problem hiding this comment.
We want them on if provided by the publisher (current default), but off when provided by Monti (for now, till we hear back from product). I think this code will be OK for this PR, we can turn them off for Monti later. Eventually we'll unify their defaults
There was a problem hiding this comment.
Makes some kind of sense but this is a setting that's kept throughout the story, so what would happen if there are videos with publisher captions and videos with cache captions in the same story? We just turn it on by default (as long as there's one track in the story provided by the publisher)?
There was a problem hiding this comment.
As I understand we are leaving captions on by default - even after Monti captions are launched.
@gmajoulet please confirm
| * @return {!Promise<boolean>} | ||
| * @private | ||
| */ | ||
| hasVideoWithAudio_() { | ||
| hasLoadedVideoWith_(func) { |
There was a problem hiding this comment.
As a first read, I really disagree with this code, can you correct me if I'm wrong?
amp-video.buildCallback() will only resolve when the video cache call resolved and is processed (sources are appended, audio attr is set, and track is appended).
waitForElementsWithUnresolvedAudio retrieves the component impl through getImpl() which sounds unnecessary, it should rely on a post buildCallback signal. Same to wait for tracks to be appended.
If you don't want to write new code you can rely on waitForPlaybackMediaLayout_ but technically you could even do sooner than that.
On top of being too complicated for no good reason, and building on top of it, this code is also wrong and doesn't rely on the media selectors that we built to look within friendly iframes (ads).
I'd really want this code to re-use the existing helpers and not build a custom broken solution.
There was a problem hiding this comment.
getImpl() is the promise that resolves when buildCallback finishes, which is why we can use it to wait for the tracks to be appended. I'll try to refactor this so it's easier to follow and more streamlined
There was a problem hiding this comment.
I agree that waiting for waitForPlaybackMediaLayout resolves too late, we could wait for whenUpgradedToCustomElement(mediaEl).then((el) => el.signals().whenSignal(BUILD)) but it's longer than getImpl() and does the same thing. I think renaming the method we use to waitForVideosWithCachedSources makes it more explicit and then we can do the logic to find if any of the videos have tracks
| * @private | ||
| */ | ||
| initializeCaptionsListener_() { | ||
| this.hasLoadedVideoWith_((video) => video.querySelector('track')).then( |
There was a problem hiding this comment.
(also, his helper doesn't even check if the video has loaded at all)
There was a problem hiding this comment.
It waits for the buildCallback to finish due to the getImpl() call for cached videos with audio, which is what Loaded was trying to mean in this context. I'll try to clean this logic up anyways, so it's more clear
| @@ -774,6 +777,7 @@ export class AmpVideo extends AMP.BaseElement { | |||
| if (!captionsElement) { | |||
| return; | |||
| } | |||
| this.captionsAreDelegated_ = true; | |||
There was a problem hiding this comment.
This architecture is wrong, you're mixing and tying up different concepts / features together, which is very hurtful long term. It becomes really hard to untangle as specific features evolve, or for new engs to understand.
What you mean by captionsAreDelegated is "we're using amp-story-captions.
What we want is amp-story components will toggle their captions on/off, this is not related to what system we use to render the captions.
Please use this.isManagedByPool_() to know if you're in a story context.
There was a problem hiding this comment.
you're mixing and tying up different concepts / features together
All the code in this file is written in the methods setupCaptions and toggleCaptions, can you explain better how that's tying up other concepts?
What we want is amp-story components will toggle their captions on/off, this is not related to what system we use to render the captions.
Currently amp-video toggles it's captions when the method toggleCaptions is called, but as I understand you're suggesting that amp-story-page (or similar) should toggle the captions. However, if we don't know whether the captions are managed by amp-story-captions or not, then we couldn't tell whether to show the captions (if the video uses native captions) or hide them (so amp-story-captions can still receive them to display them) when enabling the captions. We need to have the code in amp-video because we want for this to work without amp-story-captions, and we can't have this in amp-story-page because it would be very weird to wait for buildCallback then get the tracks from the video then update them. What do you suggest then?
Please use this.isManagedByPool_() to know if you're in a story context.
That wouldn't work because inside a story the video could show native captions (if there are no amp-story-captions) or custom captions. We need to know if (captions-id is present and points to a valid element) { enable/disable captions} else {hide/disable captions} which is
Adds a CC icon on pages with videos that have closed captions.
The PR does 3 main things:
resume_()) it will activate or deactivate the captions of the videos inside it. It will also toggle when the button is clicked.Note: this PR increases the bundle size by 1kB (mostly due to the large SVGs), so my suggestion is to hold off until we have more captions on videos (currently about 1000 stories have captions, which is not very significant). I also don't know why the auto-ads size changes so much.