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

SizeConfig and Labels - unexpected behavior? #2043

Closed
rmloveland opened this issue Jan 17, 2018 · 5 comments
Closed

SizeConfig and Labels - unexpected behavior? #2043

rmloveland opened this issue Jan 17, 2018 · 5 comments
Assignees

Comments

@rmloveland
Copy link
Contributor

Type of issue

Bug

Description

I have test pages where mediaQuery should match the page size, and the labelAny targeting on the ad unit matches the labels on the sizeConfig object, but an ad is not displayed.

Steps to reproduce

See cases and links to test pages in the table below.

The table below describes the behavior I've observed around the intersection of media queries, supported ad sizes, and labels targeting. Each case links to a test page showing that case. What I'm seeing seems to contradict the documentation (but it's possibly I am totally misunderstanding how this is supposed to work!)

case number mediaQuery exists and/or matches? ad size is in list of sizesSupported? sizeConfig has labels? adUnit has labelAny targeting sizeConfig label? Ad is displayed? Labels/sizing warning emitted? JSFiddle link
1 yes no no no YES no sizeconfig-test-01
2 yes no yes yes NO yes sizeconfig-test-02
3 yes yes yes yes NO yes sizeconfig-test-03
4 no yes yes yes NO yes sizeconfig-test-04
5 yes yes yes no YES no sizeconfig-test-05
6 no yes yes no YES no sizeconfig-test-06
7 no no yes no YES no sizeconfig-test-07

Based on the above, I have questions:

  • If the mediaQuery matches the page size, and the labelAny targeting on the ad unit matches the label on the sizeConfig, isn't that supposed to work? Or is it not matching in an obvious way that I'm missing?

  • As far as I can tell the labels match (they are the same strings), and should fire with labelAny - what am I not seeing?

  • What is the exact nature of the interaction between labels and media queries? I would imagine they get ANDed together? Or?

  • It appears to be the case that if there is an ad that matches the ad unit's size, it doesn't matter if the media query matches and the ad size is not listed in the supported sizes. I think this is what is meant in the docs by

    So the adUnit.sizes is a subset of the sizes defined from the
    resulting intersection of sizesSupported sizes and adUnit.sizes.

  • The error message WARNING: callBids executed with no bidRequests. Were they filtered by labels or sizing? doesn't really help at all, since there are two interacting features, either of which could be failing to work. Is there some way to trace the operation of the matching and print something that shows me the steps? (AKA a backtrace? (for this mini pattern matching interpreter))

Expected results

Ad should serve when mediaQuery matches the page size, the ad size is in the list of supported sizes, and the labelAny array includes a label from the ad unit.

Actual results

Ad does not serve when the above conditions are in place.

@mkendall07
Copy link
Member

@snapwich mind taking a look when you get a chance?

@snapwich snapwich self-assigned this Jan 17, 2018
@snapwich
Copy link
Collaborator

@rmloveland At a glance I think all your test pages are setting sizeConfig incorrectly.

You have var sizeConfig = [ ... ]; pbjs.setConfig(sizeConfig) when it should be pbjs.setConfig({sizeConfig: sizeConfig});

If you're still having issues with sizeConfig after that change, let me know and I can dig in further.

@rmloveland
Copy link
Contributor Author

OK, fixing the setConfig syntax caused all but two of the examples above to work as I expected. This was narrowed to only one after reading the docs, which answered my question about #7 with this line:

If a label conditional (e.g. labelAny) doesn’t exist on an ad unit, it is automatically included in all requests for bids.

This leaves just #4 that I have questions about.


In this updated version of test case #4:

  • The prebid.org test ad's size is in sizesSupported (300x250)
  • The labelAny on the ad unit is targeting the label widescreen in the first object in the sizeConfig array
  • The first object in the sizeConfig array doesn't specify a mediaQuery (it's commented out)

Given the above, I would expect "no mediaQuery" to mean "match any size" and lead to relying on just the label targeting. However the match appears to fail, and no ad is shown. This leads me to believe mediaQuery is a required property in the sizeConfig object(s)? Uncommenting the mediaQuery in the fiddle and expanding the fiddle's output area gets the test ad to serve, which seems to confirm this.

Is this the expected behavior?

@snapwich
Copy link
Collaborator

snapwich commented Feb 6, 2018

I think that's expected behavior. We didn't cover the case of the mediaQuery property missing in the design doc (just went and re-read it), but I would consider that an error case.

I would rather someone explicitly state that they accept any screen size then have our code assume that to be the case since such a rule (match all screen sizes) isn't very useful in this module. The design doc says "We don't recommend structuring your sizeConfig such that multiple mediaQueries match, but it is possible.", and a rule that matches all would lead to that undesired scenario frequently.

It's possible we could throw an error for a missing mediaQuery... I could get behind that.

@rmloveland
Copy link
Contributor Author

It's possible we could throw an error for a missing mediaQuery... I could get behind that.

+1

I would definitely be in favor. Then we could update the documentation to note that the mediaQuery is required, and share the rationale you just laid out re: multiple queries matching.

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

No branches or pull requests

3 participants