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

Apply matchMedia to the top frame #3612

Merged
merged 3 commits into from
Mar 11, 2019
Merged

Conversation

benjaminclot
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
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

In case Prebid.js is called from within an iFrame, matchMedia should be applied to window.top, not the containing iFrame.
This PR falls back to the legacy behavior in case of unfriendly iFrames.

In case Prebid.js is called from within an iFrame, matchMedia should be applied to window.top, not the containing iFrame.
This PR falls back to the legacy behavior in case of unfriendly iFrames.
@benjaminclot
Copy link
Contributor Author

CI fails because of a Yieldbot adapter problem on Chrome 61.0.3163 (Windows 10.0.0)? 🤔

@mike-chowla mike-chowla self-assigned this Mar 5, 2019
@benjaminclot
Copy link
Contributor Author

benjaminclot commented Mar 11, 2019

@mike-chowla It would be great if this PR could make it into the next release. This is quite an annoying bug, as it also impacts postbid.

@mike-chowla
Copy link
Contributor

I'm doubtful it can get in the new next release because it needs to two reviews due to being a core change.

What scenarios do you expect this to fix? I'm trying the think through all cases to understand why it makes sense that the media query is applied to top window rather than the iframe?

@benjaminclot
Copy link
Contributor Author

benjaminclot commented Mar 11, 2019

What scenarios do you expect this to fix? I'm trying the think through all cases to understand why it makes sense that the media query is applied to top window rather than the iframe?

Why would you apply it to an iframe? Isn't the goal to know the full width of a window?

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.

Seems like a reasonable thing to me.

@mike-chowla mike-chowla self-requested a review March 11, 2019 23:25
Copy link
Contributor

@mike-chowla mike-chowla left a comment

Choose a reason for hiding this comment

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

Since Kendall is good with it and I don't see any code issues, I'm approving and merging

@mike-chowla mike-chowla merged commit 0b39298 into prebid:master Mar 11, 2019
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