From e90a603d57836f6b8962a661b4668fd81c9b72a4 Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Tue, 6 Mar 2018 17:27:59 +0100 Subject: [PATCH 1/7] Removed a double this.enabled variable and updated a comment in ads.js. Also made sure the adsmanager promise also can fail, so we can use it to wait for getting the advertisement ready when someone clicks the play button. Otherwise there it can look glitchy when the actual video starts playing and the video ad plays a few seconds later because the vast tag was slow to retrieve. Also fixed a typo. --- src/js/defaults.js | 2 +- src/js/plugins/ads.js | 13 +++++-------- src/js/plyr.js | 13 +++++++------ 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/js/defaults.js b/src/js/defaults.js index 2a438d13..f7738afc 100644 --- a/src/js/defaults.js +++ b/src/js/defaults.js @@ -259,7 +259,7 @@ const defaults = { // Ads 'adsloaded', 'adscontentpause', - 'adsconentresume', + 'adscontentresume', 'adstarted', 'adsmidpoint', 'adscomplete', diff --git a/src/js/plugins/ads.js b/src/js/plugins/ads.js index 0faf0f2f..9318e01d 100644 --- a/src/js/plugins/ads.js +++ b/src/js/plugins/ads.js @@ -36,9 +36,8 @@ class Ads { this.playing = false; this.initialized = false; this.blocked = false; - this.enabled = utils.is.url(player.config.ads.tag); - // Check if a tag URL is provided. + // Check if ads are enabled. if (!this.enabled) { return; } @@ -83,14 +82,12 @@ class Ads { // thing doesn't resolve within our set time; we bail this.startSafetyTimer(12000, 'ready()'); - // Setup a simple promise to resolve if the IMA loader is ready - this.loaderPromise = new Promise(resolve => { - this.on('ADS_LOADER_LOADED', () => resolve()); - }); - // Setup a promise to resolve if the IMA manager is ready - this.managerPromise = new Promise(resolve => { + this.managerPromise = new Promise((resolve, reject) => { + // The ad is pre-loaded and ready this.on('ADS_MANAGER_LOADED', () => resolve()); + // Ads failed + this.on('ERROR', () => reject()); }); // Clear the safety timer diff --git a/src/js/plyr.js b/src/js/plyr.js index 7f8ad18c..9b73423e 100644 --- a/src/js/plyr.js +++ b/src/js/plyr.js @@ -309,14 +309,15 @@ class Plyr { * Play the media, or play the advertisement (if they are not blocked) */ play() { - // TODO: Always return a promise? if (this.ads.enabled && !this.ads.initialized && !this.ads.blocked) { - this.ads.play(); - return null; + this.ads.managerPromise.then(() => { + this.ads.play(); + }).catch(() => { + this.media.play(); + }); + } else { + this.media.play(); } - - // Return the promise (for HTML5) - return this.media.play(); } /** From 409b588458fd598a88d56f3922da83ff82d80f5f Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Wed, 7 Mar 2018 15:17:30 +0100 Subject: [PATCH 2/7] Made sure that cue points for midrolls are not displayed when the ad rule for a midroll doesn't exceed the total play time of a video. --- src/js/plugins/ads.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/plugins/ads.js b/src/js/plugins/ads.js index 9318e01d..0497c42c 100644 --- a/src/js/plugins/ads.js +++ b/src/js/plugins/ads.js @@ -206,7 +206,7 @@ class Ads { // Add advertisement cue's within the time line if available this.cuePoints.forEach(cuePoint => { - if (cuePoint !== 0 && cuePoint !== -1) { + if (cuePoint !== 0 && cuePoint !== -1 && cuePoint < this.player.duration) { const seekElement = this.player.elements.progress; if (seekElement) { From 819f7d1080ef2748fa1c123ad0f54d075c654ab9 Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Wed, 7 Mar 2018 15:43:48 +0100 Subject: [PATCH 3/7] Resizing the ad container while having it on display none will return offset width and height of 0, which will cause ads not to play when ad sizes are set within the clients DSP. Also making sure that the inner containers of the ad container are full size. The container is now hidden/ displayed using z-index. --- src/js/plugins/ads.js | 9 ++++----- src/sass/plugins/ads.scss | 14 +++++++++++++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/js/plugins/ads.js b/src/js/plugins/ads.js index 0497c42c..eca523f7 100644 --- a/src/js/plugins/ads.js +++ b/src/js/plugins/ads.js @@ -111,7 +111,6 @@ class Ads { // Create the container for our advertisements this.elements.container = utils.createElement('div', { class: this.player.config.classNames.ads, - hidden: '', }); this.player.elements.container.appendChild(this.elements.container); @@ -451,8 +450,8 @@ class Ads { * Resume our video. */ resumeContent() { - // Hide our ad container - utils.toggleHidden(this.elements.container, true); + // Hide the advertisement container + this.elements.container.style.zIndex = ''; // Ad is stopped this.playing = false; @@ -467,8 +466,8 @@ class Ads { * Pause our video */ pauseContent() { - // Show our ad container. - utils.toggleHidden(this.elements.container, false); + // Show the advertisement container + this.elements.container.style.zIndex = '3'; // Ad is playing. this.playing = true; diff --git a/src/sass/plugins/ads.scss b/src/sass/plugins/ads.scss index 21a28e02..98620508 100644 --- a/src/sass/plugins/ads.scss +++ b/src/sass/plugins/ads.scss @@ -9,7 +9,19 @@ position: absolute; right: 0; top: 0; - z-index: 3; // Above the controls + z-index: -1; // Hide it. + + // Make sure the inner container is big enough for the ad creative. + > div { + position: absolute; + width: 100%; + height: 100%; + iframe { + position: absolute; + width: 100%; + height: 100%; + } + } &::after { background: rgba($plyr-color-gunmetal, 0.8); From 69ffcbad27353bfd0238ba7e79e6caf3835b0efd Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Fri, 9 Mar 2018 11:17:24 +0100 Subject: [PATCH 4/7] Ad block detection would not work when calling play() right after creating the player instance, so the adsManager now also rejects on such a case. Also made sure that calling play() will wait for the adsManager promise to resolve or otherwise return the media.play() method. --- src/js/plugins/ads.js | 67 ++++++++++++++++++--------------------- src/js/plyr.js | 7 ++-- src/sass/plugins/ads.scss | 8 ++--- 3 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/js/plugins/ads.js b/src/js/plugins/ads.js index eca523f7..cc11fa13 100644 --- a/src/js/plugins/ads.js +++ b/src/js/plugins/ads.js @@ -35,35 +35,6 @@ class Ads { this.enabled = player.config.ads.enabled; this.playing = false; this.initialized = false; - this.blocked = false; - - // Check if ads are enabled. - if (!this.enabled) { - return; - } - - // Check if the Google IMA3 SDK is loaded or load it ourselves - if (!utils.is.object(window.google)) { - utils.loadScript( - player.config.urls.googleIMA.api, - () => { - this.ready(); - }, - () => { - // Script failed to load or is blocked - this.blocked = true; - this.player.debug.log('Ads error: Google IMA SDK failed to load'); - }, - ); - } else { - this.ready(); - } - } - - /** - * Get the ads instance ready. - */ - ready() { this.elements = { container: null, displayContainer: null, @@ -75,26 +46,50 @@ class Ads { this.safetyTimer = null; this.countdownTimer = null; - // Set listeners on the Plyr instance - this.listeners(); + if (this.enabled) { + // Check if the Google IMA3 SDK is loaded or load it ourselves + if (!utils.is.object(window.google)) { + utils.loadScript( + player.config.urls.googleIMA.api, + () => { + this.ready(); + }, + () => { + // Script failed to load or is blocked + this.handleEventListeners('ERROR'); + this.player.debug.log('Ads error: Google IMA SDK failed to load'); + }, + ); + } else { + this.ready(); + } + } - // Start ticking our safety timer. If the whole advertisement - // thing doesn't resolve within our set time; we bail - this.startSafetyTimer(12000, 'ready()'); - - // Setup a promise to resolve if the IMA manager is ready + // Setup a promise to resolve when the IMA manager is ready this.managerPromise = new Promise((resolve, reject) => { // The ad is pre-loaded and ready this.on('ADS_MANAGER_LOADED', () => resolve()); // Ads failed this.on('ERROR', () => reject()); }); + } + + /** + * Get the ads instance ready. + */ + ready() { + // Start ticking our safety timer. If the whole advertisement + // thing doesn't resolve within our set time; we bail + this.startSafetyTimer(12000, 'ready()'); // Clear the safety timer this.managerPromise.then(() => { this.clearSafetyTimer('onAdsManagerLoaded()'); }); + // Set listeners on the Plyr instance + this.listeners(); + // Setup the IMA SDK this.setupIMA(); } diff --git a/src/js/plyr.js b/src/js/plyr.js index 9b73423e..d4c3d392 100644 --- a/src/js/plyr.js +++ b/src/js/plyr.js @@ -309,14 +309,15 @@ class Plyr { * Play the media, or play the advertisement (if they are not blocked) */ play() { - if (this.ads.enabled && !this.ads.initialized && !this.ads.blocked) { + // Return the promise (for HTML5) + if (this.ads.enabled && !this.ads.initialized) { this.ads.managerPromise.then(() => { this.ads.play(); }).catch(() => { - this.media.play(); + return this.media.play(); }); } else { - this.media.play(); + return this.media.play(); } } diff --git a/src/sass/plugins/ads.scss b/src/sass/plugins/ads.scss index 98620508..89330611 100644 --- a/src/sass/plugins/ads.scss +++ b/src/sass/plugins/ads.scss @@ -12,15 +12,11 @@ z-index: -1; // Hide it. // Make sure the inner container is big enough for the ad creative. - > div { + > div, + > div iframe { position: absolute; width: 100%; height: 100%; - iframe { - position: absolute; - width: 100%; - height: 100%; - } } &::after { From ba8d7831a7eb374e4ccf31e83a731019e900c7a3 Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Fri, 9 Mar 2018 12:50:57 +0100 Subject: [PATCH 5/7] Made sure play() returns a promise. --- src/js/plyr.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/js/plyr.js b/src/js/plyr.js index d4c3d392..7b4ddd5b 100644 --- a/src/js/plyr.js +++ b/src/js/plyr.js @@ -311,10 +311,10 @@ class Plyr { play() { // Return the promise (for HTML5) if (this.ads.enabled && !this.ads.initialized) { - this.ads.managerPromise.then(() => { + return this.ads.managerPromise.then(() => { this.ads.play(); }).catch(() => { - return this.media.play(); + this.media.play(); }); } else { return this.media.play(); From 7adc2bc6c8eeb7ae0651d536aa561cec2e37a830 Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Fri, 9 Mar 2018 13:21:19 +0100 Subject: [PATCH 6/7] Unneeded else has been removed within the play() method. --- src/js/plyr.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/js/plyr.js b/src/js/plyr.js index 7b4ddd5b..04913046 100644 --- a/src/js/plyr.js +++ b/src/js/plyr.js @@ -316,9 +316,9 @@ class Plyr { }).catch(() => { this.media.play(); }); - } else { - return this.media.play(); } + + return this.media.play(); } /** From 6a2ca534d219233b20941bfc987f7a4a488502c7 Mon Sep 17 00:00:00 2001 From: Arthur Hulsman Date: Fri, 9 Mar 2018 14:29:37 +0100 Subject: [PATCH 7/7] Removed redundant wrappers within the adsmanager promises. --- src/js/plugins/ads.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/js/plugins/ads.js b/src/js/plugins/ads.js index cc11fa13..62162e84 100644 --- a/src/js/plugins/ads.js +++ b/src/js/plugins/ads.js @@ -68,9 +68,9 @@ class Ads { // Setup a promise to resolve when the IMA manager is ready this.managerPromise = new Promise((resolve, reject) => { // The ad is pre-loaded and ready - this.on('ADS_MANAGER_LOADED', () => resolve()); + this.on('ADS_MANAGER_LOADED', resolve); // Ads failed - this.on('ERROR', () => reject()); + this.on('ERROR', reject); }); } @@ -503,7 +503,7 @@ class Ads { // Re-set our adsManager promises this.managerPromise = new Promise(resolve => { - this.on('ADS_MANAGER_LOADED', () => resolve()); + this.on('ADS_MANAGER_LOADED', resolve); this.player.debug.log(this.manager); });