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

Detect browser width instead of the screen width #837

Merged
merged 1 commit into from
Dec 15, 2016

Conversation

agabel
Copy link
Contributor

@agabel agabel commented Nov 29, 2016

Type of change

  • Bugfix

Description of change

Currently the size mapping is detecting the user's screen width instead of the browser. This change will use the browser width to determine which size mapping should be used.

@mkendall07
Copy link
Member

It looks like this was the original code I had written. We noticed that this wasn't working with device emulation on chrome. I'll revisit this.

@mkendall07
Copy link
Member

Thanks for the PR. The problem here is that I'm confused with how innerWidth is calculated on mobile. For example, visit http://client-test.devnxs.net/demos/display.html from your phone (fullscreen browser) and see the differences in measurement.

@agabel
Copy link
Contributor Author

agabel commented Dec 5, 2016

I see what you mean. I'll take another look into this and see if there is a better way to handle it.

@mkendall07
Copy link
Member

It seems to be related to the mobile "visual viewport" : http://www.quirksmode.org/mobile/viewports2.html

I haven't digested all of this yet, but we might need to just pick one and go with it. I'm not sure which way is more "standard".

@agabel
Copy link
Contributor Author

agabel commented Dec 6, 2016

I have been looking into this further and it appears to me that the current PR should be the preferred way to detect viewport size. The innerWidth method will return the browser viewport. A fallback to the clientWidth method is acceptable to detect the width of the document itself. There are cases where the document could expand past the browser's actual viewport. The viewport meta tag should be used to set the viewport width to match the device width. The http://client-test.devnxs.net/demos/display.html site does not set a viewport meta tag so it will use the browsers default layout width. In Safari on iPhone that is 980px, but other browsers will vary. The innerWidth/clientWidth method would give us this value which would still be technically correct in regard to the page width.

@mkendall07
Copy link
Member

@agabel
Ok thanks, I tend to agree with you. This LGTM.

@mkendall07 mkendall07 merged commit f20cbb1 into prebid:master Dec 15, 2016
@agabel agabel deleted the browserwidth branch December 15, 2016 19:44
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-official-0.17.0 to release/1.12.0

* commit 'bce975978c2c088d301d120b949d38080ca9d314':
  Add changelog entry.
  Prebid Release 0.17.0
  remove hb_pb targeting key for deal bids with no cpm (prebid#881)
  Fix for bug prebid#866 (prebid#867)
  Allow changing of 'ga' global variable (prebid#832)
  emit auction end event before bidsBackHandler callback (prebid#884)
  Add Widespace adapter (prebid#846)
  prevent rubicon adapter from registering two bids on exceptions (prebid#854)
  Code Refactoring - Upgrading end point. (prebid#826)
  Ignore test html pages (prebid#878)
  Detect browser width instead of the screen width (prebid#837)
  New aardvark adapter with support for aliasing (prebid#875)
  Add Fidelity adapter (prebid#862)
  Adkernel adapter aliasing (prebid#857)
  0.16.1-pre
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…12.0 to master

* commit '728f1763ce2a282a757546e934199313a4771a21':
  Fix failing unit test on CI.
  Add changelog entry.
  Prebid Release 0.17.0
  remove hb_pb targeting key for deal bids with no cpm (prebid#881)
  Fix for bug prebid#866 (prebid#867)
  Allow changing of 'ga' global variable (prebid#832)
  emit auction end event before bidsBackHandler callback (prebid#884)
  Add Widespace adapter (prebid#846)
  prevent rubicon adapter from registering two bids on exceptions (prebid#854)
  Code Refactoring - Upgrading end point. (prebid#826)
  Ignore test html pages (prebid#878)
  Detect browser width instead of the screen width (prebid#837)
  New aardvark adapter with support for aliasing (prebid#875)
  Add Fidelity adapter (prebid#862)
  Adkernel adapter aliasing (prebid#857)
  0.16.1-pre
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants