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.

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

allow writing into current document if prebid is loaded inside an iframe #1066

Merged
merged 4 commits into from
Apr 12, 2017
Merged

allow writing into current document if prebid is loaded inside an iframe #1066

merged 4 commits into from
Apr 12, 2017

Conversation

surensilva
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

For PostBid implementations, the secondary iframe ('postbid_if') where the ad is inserted into can cause view-ability metrics to drop. This enhancement allows the passing of the current document to the renderAd provided that prebid is loaded inside an iframe.

Updated PostBid Example: https://gist.github.com/surensilva/bdeec34abf1027acd8a87eb48a79f144

@mkendall07
Copy link
Member

This is a great catch, thank you!

@mkendall07 mkendall07 self-assigned this Mar 30, 2017
src/prebid.js Outdated
@@ -301,7 +301,7 @@ $$PREBID_GLOBAL$$.renderAd = function (doc, id) {
var url = adObject.adUrl;
var ad = adObject.ad;

if (doc === document || adObject.mediaType === 'video') {
if (!utils.inIframe() || adObject.mediaType === 'video') {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this logic is wrong, it fails for the standard prebid case (always evaluates to true when not in an iframe).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes you are correct, this breaks my test page; we should have tested that.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

Please fix for non-iframe case

@@ -588,4 +588,12 @@ export function isSrcdocSupported(doc) {

export function cloneJson(obj) {
return JSON.parse(JSON.stringify(obj));
}

export function inIframe() {
Copy link
Member

Choose a reason for hiding this comment

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

I think the problem here is we need to check a passed in window param, which we don't have. I think we have to change the renderAd API to accept the window instead of doc :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm; I would hate to break backwards compatibility. Suren and I will brainstorm some techniques to see if we can come up with a backwards compatible solution that still solves this problem, and if so we'll update the pull-request. Thanks!

@snapwich
Copy link
Collaborator

snapwich commented Apr 4, 2017

@mkendall07 updated this to be more specific for our case so it doesn't break the general Prebid.js case. Also updated isSrcdocSupported as it was breaking when ran inside a cross-domain iFrame.

@snapwich
Copy link
Collaborator

@prebid/core can we get another review on this to see if it's good to go? We'd like to merge this sooner than later. Thanks!

@mkendall07
Copy link
Member

LGTM

@mkendall07 mkendall07 merged commit bb2a96f into prebid:master Apr 12, 2017
outoftime added a commit to Genius/Prebid.js that referenced this pull request Apr 19, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (38 commits)
  Add optional domain parameter to AdButler adapter (prebid#1078)
  Send transactionID to Criteo Services (prebid#1113)
  Fix `buildMasterVideoTagFromAdserverTag()` not selecting winning bid (prebid#1106)
  Remove placement size selection and filtering (prebid#1107)
  revert `srcdoc` change (prebid#1130)
  Add new Adapter- Beachfront Media (prebid#1062)
  Fixes SpringServe adapter (prebid#1101)
  Update Widespace request param (prebid#1098)
  - New Adapter: Innity (prebid#1074)
  Update Roxot prebid analytic adapter (prebid#1034)
  Yarn Package Manager (prebid#1109)
  allow writing into current document if prebid is loaded inside an iframe (prebid#1066)
  Adapter bug fix (prebid#1096)
  fix typo
  added pr review process and governance model (prebid#1103)
  added support for sampling in ga and base adapter, fixed up some tests (prebid#1011)
  Add Inneractive adapter (prebid#1048)
  Add alias freewheel-ssp to stickyadstv bidder adapter  (prebid#1043)
  Add Facebook Audience Network adapter (prebid#1068)
  Add Atomx support (prebid#1056)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
…ame (prebid#1066)

* allow writing into current document if prebid is loaded inside an iframe

* updated tests to use utils.isiFrame instead of doc === window.document

* only check doc === document in renderAd if not in iFrame

* check for frameElement to prevent error in cross-origin iFrame case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants