From 1d7762a283c7d4af8c503026ea619311cf508a51 Mon Sep 17 00:00:00 2001 From: Carlos Serrano Date: Wed, 5 Dec 2018 14:54:44 +0100 Subject: [PATCH 1/2] =?UTF-8?q?refactor:=20=F0=9F=92=A1=20use=20`about:bla?= =?UTF-8?q?nck`=20and=20document.write?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to set the content of the iframe, and use srcDoc as a fallback. Some VPAID ads use document.write and some browsers throw if you call document.write within an iframe which loaded its content with srcdoc. --- .../helpers/__tests__/createIframe.spec.js | 3 +- src/adContainer/helpers/createIframe.js | 45 ++++++++++--------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/adContainer/helpers/__tests__/createIframe.spec.js b/src/adContainer/helpers/__tests__/createIframe.spec.js index 2b0d1d05..c1779390 100644 --- a/src/adContainer/helpers/__tests__/createIframe.spec.js +++ b/src/adContainer/helpers/__tests__/createIframe.spec.js @@ -9,7 +9,6 @@ describe('createIframe', () => { url: 'https://www.example.com/' }); - // TODO: Test srcdoc once jsdom supports it. See https://github.com/jsdom/jsdom/pull/2389 for more info supportsSrcdoc.mockReturnValue(false); }); @@ -37,7 +36,7 @@ describe('createIframe', () => { try { await createIframe(document.createElement('div'), test); } catch (error) { - expect(error.message).toBe('Error creating iframe, the placeholder is probably not in the DOM'); + expect(error).toBeInstanceOf(Error); } }); }); diff --git a/src/adContainer/helpers/createIframe.js b/src/adContainer/helpers/createIframe.js index aceb8f80..7757d9bc 100644 --- a/src/adContainer/helpers/createIframe.js +++ b/src/adContainer/helpers/createIframe.js @@ -1,4 +1,3 @@ -import defer from '../../utils/defer'; import getContentDocument from './getContentDocument'; import getOrigin from './getOrigin'; import supportsSrcdoc from './supportsSrcdoc'; @@ -11,10 +10,8 @@ const iframeContent = (id, targetOrigin) => ` `; -const createIframe = (placeholder, id) => { - const deferred = defer(); +const createBaseIframe = () => { const iframe = document.createElement('IFRAME'); - const content = iframeContent(id, getOrigin()); iframe.sandbox = 'allow-forms allow-popups allow-scripts allow-same-origin'; iframe.style.margin = '0'; @@ -24,21 +21,31 @@ const createIframe = (placeholder, id) => { iframe.style.height = '0'; iframe.style.position = 'absolute'; - if (supportsSrcdoc()) { - iframe.src = 'about:srcdoc'; - iframe.srcdoc = content; - placeholder.appendChild(iframe); - } else { - iframe.src = 'about:blank'; - placeholder.appendChild(iframe); + return iframe; +}; - const iframeDocument = getContentDocument(iframe); +const createIframe = (placeholder, id) => new Promise((resolve, reject) => { + const content = iframeContent(id, getOrigin()); + let iframe; - if (iframeDocument) { - iframeDocument.write(content); + /* + NOTE: favor about:blank instead of srcdoc because some browsers + */ + try { + iframe = createBaseIframe(); + iframe.src = 'about:blank'; + placeholder.appendChild(iframe); + getContentDocument(iframe).write(content); + } catch (error) { + placeholder.removeChild(iframe); + + if (supportsSrcdoc()) { + iframe = createBaseIframe(); + iframe.src = 'about:srcdoc'; + iframe.srcdoc = content; + placeholder.appendChild(iframe); } else { - placeholder.removeChild(iframe); - deferred.reject(new Error('Error creating iframe, the placeholder is probably not in the DOM')); + reject(error); } } @@ -46,13 +53,11 @@ const createIframe = (placeholder, id) => { /* istanbul ignore else */ if (data === `${id}_ready`) { window.removeEventListener('message', handleMessage); - deferred.resolve(iframe); + resolve(iframe); } }; window.addEventListener('message', handleMessage, false); - - return deferred.promise; -}; +}); export default createIframe; From ad89aad0304257955ce08a0bcdd2b2034ef9669d Mon Sep 17 00:00:00 2001 From: Carlos Serrano Date: Wed, 5 Dec 2018 15:15:43 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20=F0=9F=90=9C=20finish=20vaid=20ad=20?= =?UTF-8?q?unit=20on=20adUserClose=20evt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/adUnit/VpaidAdUnit.js | 12 +++++++----- src/adUnit/__tests__/VpaidAdUnit.spec.js | 20 +++++++++++++++++--- src/tracker/linearEvents.js | 8 ++++++++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/adUnit/VpaidAdUnit.js b/src/adUnit/VpaidAdUnit.js index efccb90a..223dc35f 100644 --- a/src/adUnit/VpaidAdUnit.js +++ b/src/adUnit/VpaidAdUnit.js @@ -3,8 +3,7 @@ import linearEvents from '../tracker/linearEvents'; import { acceptInvitation, creativeView, - adCollapse, - close + adCollapse } from '../tracker/nonLinearEvents'; import {getClickThrough} from '../vastSelectors'; import {volumeChanged, adProgress} from './adUnitEvents'; @@ -61,7 +60,8 @@ const { midpoint, thirdQuartile, clickThrough, - error: errorEvt + error: errorEvt, + closeLinear } = linearEvents; // eslint-disable-next-line id-match @@ -170,10 +170,12 @@ class VpaidAdUnit extends VideoAdUnit { }); }, [adUserClose]: () => { - this.emit(close, { + this.emit(closeLinear, { adUnit: this, - type: close + type: closeLinear }); + + this[_protected].finish(); }, [adUserMinimize]: () => { this.emit(adCollapse, { diff --git a/src/adUnit/__tests__/VpaidAdUnit.spec.js b/src/adUnit/__tests__/VpaidAdUnit.spec.js index 4f1b985b..1e9d27de 100644 --- a/src/adUnit/__tests__/VpaidAdUnit.spec.js +++ b/src/adUnit/__tests__/VpaidAdUnit.spec.js @@ -54,13 +54,13 @@ import linearEvents, { resume, clickThrough, iconClick, + closeLinear, iconView } from '../../tracker/linearEvents'; import { acceptInvitation, creativeView, - adCollapse, - close + adCollapse } from '../../tracker/nonLinearEvents'; import addIcons from '../helpers/icons/addIcons'; import retrieveIcons from '../helpers/icons/retrieveIcons'; @@ -609,6 +609,20 @@ describe('VpaidAdUnit', () => { expect(callback).toHaveBeenCalledTimes(1); }); + + test('must be called if the user closes the ad unit', async () => { + const callback = jest.fn(); + + adUnit.onFinish(callback); + + await adUnit.start(); + + expect(callback).not.toHaveBeenCalled(); + + adUnit.creativeAd.emit(adUserClose); + + expect(callback).toHaveBeenCalledTimes(1); + }); }); describe('onError', () => { @@ -747,7 +761,7 @@ describe('VpaidAdUnit', () => { vpaidEvt: adUserMinimize }, { - vastEvt: close, + vastEvt: closeLinear, vpaidEvt: adUserClose }, { diff --git a/src/tracker/linearEvents.js b/src/tracker/linearEvents.js index 4569d10e..3b52e457 100644 --- a/src/tracker/linearEvents.js +++ b/src/tracker/linearEvents.js @@ -165,8 +165,16 @@ export const iconClick = 'iconClick'; */ export const iconView = 'iconView'; +/** + * The viewer has chosen to close the linear ad unit. This is currently inuse by some of the largest mobile SDKs to mark the dismissal of the end card companion that follows the video, as well as a close of the video itself, if applicable + * + * @event LinearEvents#closeLinear + */ +export const closeLinear = 'closeLinear'; + const linearEvents = { clickThrough, + closeLinear, complete, error, exitFullscreen,