Select media through iframe boundary #14981
Conversation
|
@gmajoulet I tested this manually, but spent a long time trying to write a unit test with no success 😭. Let me know if you have any ideas. |
|
Ok |
| if (iframeDoc) { | ||
| mediaSet = Array.from(mediaSet); | ||
| const fieMedia = scopedQuerySelectorAll(iframeDoc, PAGE_MEDIA_SELECTOR); | ||
| fieMedia.forEach(el => mediaSet.push(el)); |
There was a problem hiding this comment.
Nit: maybe using the ES6 syntax?
const fieMedia = scopedQuerySelectorAll(iframeDoc, PAGE_MEDIA_SELECTOR);
mediaArray = [...Array.from(mediaSet), ...fieMedia]
And since you're making it an array, we could replace the line below to mediaArray.map(...)
There was a problem hiding this comment.
We aren’t currently allowing the spread operator 😕
There was a problem hiding this comment.
Oh... Maybe a good old mediaArray = Array.from(mediaSet).concat(fieMedia) then?
There was a problem hiding this comment.
Yeah this is also weird because you can't concat a nodeList. It just pushes the whole nodeList into the end of the array (I only know this because I tried it yesterday). I will change this around a bit to always return an array as suggested below anyway.
There was a problem hiding this comment.
talked offline. Changed to concat for readability.
| @@ -328,7 +336,16 @@ export class AmpStoryPage extends AMP.BaseElement { | |||
| * @private | |||
| */ | |||
| getAllMedia_() { | |||
There was a problem hiding this comment.
I think this method should always return an array, and not sometimes an array, sometimes a NodeList
There was a problem hiding this comment.
Should we update getAllVideos_ too? It's used to know if there's a video loading, to maybe display the loading spinner indicator.
There was a problem hiding this comment.
Changed to always return same type. Updated getAllVideos_ with similar logic.
| @@ -324,21 +331,45 @@ export class AmpStoryPage extends AMP.BaseElement { | |||
|
|
|||
| /** | |||
| * Gets all media elements on this page. | |||
| * @return {!NodeList<!Element>} | |||
| * @return {Array<!Element>} | |||
There was a problem hiding this comment.
Nit: I think you need the !?
There was a problem hiding this comment.
I think it now always returns an array? Sometimes it is an empty array and sometimes its not.
There was a problem hiding this comment.
Talked offline. @gmajoulet was correct, changed to !Array<?Element>
| */ | ||
| const PAGE_MEDIA_SELECTOR = 'amp-audio, amp-video, amp-img, amp-anim'; | ||
| const SELECTORS = { |
There was a problem hiding this comment.
Nit: Selectors now that it's an enum
| */ | ||
| getMediaBySelector_(selector) { | ||
| const iframe = this.element.querySelector('iframe'); | ||
| const iframeDoc = iframe && iframe.contentDocument; |
There was a problem hiding this comment.
did you check if this line will generate console error in case of xorigin iframe?
* change selector * always return array * refactor * fix types * fix types
In
amp-storywe want to make sure that any media existing inside of afriendly-iframealso gets added to themedia-pooland managed by the story runtime.