Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS Referrer fix #996

Merged
merged 5 commits into from
Mar 7, 2017
Merged

iOS Referrer fix #996

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6dd9c1e
fixed leading / omission on ie11
ckbo3hrk Feb 17, 2017
6a41602
srcdoc rendering approach to avoid http-referrer omission #977
ckbo3hrk Feb 17, 2017
a95ab43
Merge remote-tracking branch 'upstream/master' into referrer_fix
ckbo3hrk Feb 17, 2017
968591d
missing unit test for url.parse (leading slash in pathname)
ckbo3hrk Mar 6, 2017
85f6f25
move isSrcdocSupported to utils
ckbo3hrk Mar 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions src/prebid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ $$PREBID_GLOBAL$$.adUnits = $$PREBID_GLOBAL$$.adUnits || [];

/**
* Command queue that functions will execute once prebid.js is loaded
* @param {function} cmd Annoymous function to execute
* @param {function} cmd Anonymous function to execute
* @alias module:$$PREBID_GLOBAL$$.que.push
*/
$$PREBID_GLOBAL$$.que.push = function (cmd) {
Expand Down Expand Up @@ -133,6 +133,15 @@ function setRenderSize(doc, width, height) {
}
}

/**
* Check if parent iframe of passed document supports content rendering via 'srcdoc' property
* @param {HTMLDocument} doc document to check support of 'srcdoc'
*/
function isSrcdocSupported(doc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move into utils.js please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

//Firefox is excluded due to https://bugzilla.mozilla.org/show_bug.cgi?id=1265961
return !!doc.defaultView && 'srcdoc' in doc.defaultView.frameElement && !/firefox/i.test(navigator.userAgent);
}

//////////////////////////////////
// //
// Start Public APIs //
Expand Down Expand Up @@ -277,8 +286,9 @@ $$PREBID_GLOBAL$$.allBidsAvailable = function () {
};

/**
* This function will render the ad (based on params) in the given iframe document passed through. Note that doc SHOULD NOT be the parent document page as we can't doc.write() asynchrounsly
* @param {object} doc document
* This function will render the ad (based on params) in the given iframe document passed through.
* Note that doc SHOULD NOT be the parent document page as we can't doc.write() asynchronously
* @param {HTMLDocument} doc document
* @param {string} id bid id to locate the ad
* @alias module:$$PREBID_GLOBAL$$.renderAd
*/
Expand All @@ -299,15 +309,19 @@ $$PREBID_GLOBAL$$.renderAd = function (doc, id) {
var width = adObject.width;
var url = adObject.adUrl;
var ad = adObject.ad;

if (doc===document || adObject.mediaType === 'video') {
utils.logError('Error trying to write ad. Ad render call ad id ' + id + ' was prevented from writing to the main document.');
if (doc === document || adObject.mediaType === 'video') {
utils.logError(`Error trying to write ad. Ad render call ad id ${id} was prevented from writing to the main document.`);
} else if (ad) {
doc.write(ad);
doc.close();
if (isSrcdocSupported(doc)) {
doc.defaultView.frameElement.srcdoc = ad;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like srcdoc completely wipes out any other content present in the iframe. This is causing DFP to fail to report on viewable impressions. I will be reverting this change until we can find a proper solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckbo3hrk fyi

Copy link
Contributor Author

@ckbo3hrk ckbo3hrk Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkendall07, I'd like to investigate this bug. Could you please provide me a sample dfp tag to reproduce this issue ?

Copy link
Member

@mkendall07 mkendall07 Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckbo3hrk
Here is an example:
Working (filter by "activeview" in network)
[working updating links]
Not working:

} else {
doc.write(ad);
doc.close();
}
setRenderSize(doc, width, height);
} else if (url) {
doc.write('<IFRAME SRC="' + url + '" FRAMEBORDER="0" SCROLLING="no" MARGINHEIGHT="0" MARGINWIDTH="0" TOPMARGIN="0" LEFTMARGIN="0" ALLOWTRANSPARENCY="true" WIDTH="' + width + '" HEIGHT="' + height + '"></IFRAME>');
doc.write(`<IFRAME SRC="${url}" FRAMEBORDER="0" SCROLLING="no" MARGINHEIGHT="0" MARGINWIDTH="0" TOPMARGIN="0" LEFTMARGIN="0" ALLOWTRANSPARENCY="true" WIDTH="${width}" HEIGHT="${height}"></IFRAME>`);
doc.close();
setRenderSize(doc, width, height);
} else {
Expand Down Expand Up @@ -483,7 +497,7 @@ $$PREBID_GLOBAL$$.offEvent = function (event, handler, id) {
/**
* Add a callback event
* @param {String} eventStr event to attach callback to Options: "allRequestedBidsBack" | "adUnitBidsBack"
* @param {Function} func function to execute. Paramaters passed into the function: (bidResObj), [adUnitCode]);
* @param {Function} func function to execute. Parameters passed into the function: (bidResObj), [adUnitCode]);
* @alias module:$$PREBID_GLOBAL$$.addCallback
* @returns {String} id for callback
*/
Expand Down Expand Up @@ -586,7 +600,7 @@ $$PREBID_GLOBAL$$.loadScript = function (tagSrc, callback, useCache) {
};

/**
* Will enable sendinga prebid.js to data provider specified
* Will enable sending a prebid.js to data provider specified
* @param {object} config object {provider : 'string', options : {}}
*/
$$PREBID_GLOBAL$$.enableAnalytics = function (config) {
Expand Down Expand Up @@ -679,7 +693,7 @@ $$PREBID_GLOBAL$$.buildMasterVideoTagFromAdserverTag = function (adserverTag, op

/**
* Set the order bidders are called in. If not set, the bidders are called in
* the order they are defined wihin the adUnit.bids array
* the order they are defined within the adUnit.bids array
* @param {string} order - Order to call bidders in. Currently the only possible value
* is 'random', which randomly shuffles the order
*/
Expand Down
2 changes: 1 addition & 1 deletion src/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export function parse(url) {
protocol: (parsed.protocol || '').replace(/:$/, ''),
hostname: parsed.hostname,
port: +parsed.port,
pathname: parsed.pathname,
pathname: parsed.pathname.replace(/^(?!\/)/,'/'),
search: parseQS(parsed.search || ''),
hash: (parsed.hash || '').replace(/^#/, ''),
host: parsed.host
Expand Down
2 changes: 1 addition & 1 deletion test/spec/url_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('helpers.url', () => {
});

it('extracts the pathname', () => {
expect(['/pathname/', 'pathname/']).to.include(parsed.pathname);
expect(parsed).to.have.property('pathname', '/pathname/');
});

it('extracts the search query', () => {
Expand Down