prebid / prebid-universal-creative

Apache License 2.0
43 stars 69 forks source link

Possible resize issue on AMP #119

Closed muotaz closed 2 years ago

muotaz commented 3 years ago

Hello

I am not sure if this an actual bug in the universal creative or in AMP runtime, but I hope that you could shed some light on this to be able to better understand the issue and solve it afterwards.

We have noticed on websites that use amp-ad element with multisize property, that the resize request issued by the universal creative to the AMP runtime is being requested more frequently. Our observations indicate that the resize request is rejected when there is a change in the width and the height of the amp-ad element (here: https://github.com/prebid/prebid-universal-creative/blob/f062693ed8ce4d4faf1b9641da12c2f4990c6ee4/src/renderingManager.js#L280).

A simple test, where a modified universal creative that eliminates any change in the width, greatly mitigates this issue:

    var f = s.split("x").map(Number);
                    !function(n, o) {
                        if (d.isSafeFrame()) {
                            var i = h.innerWidth
                              , a = h.innerHeight;
                            i === n && a === o || (h.$sf.ext.register(n, o, function(t) {
                                var e = n - i
                                  , r = o - a;
                                  console.log("t ", t, "sizes:", e,"x",r);
                                  if(e<0) {
                                      e=0;
                                      console.log("e is reset");
                                  }
                                h.$sf.ext.expand({
                                    r: e,
                                    b: r,
                                    push: !0
                                })
                            }),
                            console.log("t ", t, "sizes:", n,"x",o),

I appreciate your opinion on this regard.

premesh-freestar commented 3 years ago

Are u using safe frames for the the creative ? Im running into the same issue here. Looking into the UC code i see this https://github.com/prebid/prebid-universal-creative/blob/f062693ed8ce4d4faf1b9641da12c2f4990c6ee4/src/renderingManager.js#L268

makes it seem like safeframes are required for AMP ?

jsnellbaker commented 3 years ago

@robertrmartinez Could you take a look into this please?

andyblackwell commented 3 years ago

I just tested this on our setup with multisize and safeframe off, and can confirm all the prebid wins are rendering at size 1x1

premesh-freestar commented 3 years ago

So perhaps there is no issue? Just need to use safe frames since UC expects amp to uses the amp apis to resize the container?

muotaz commented 3 years ago

We are already using safe frame. The issue is that sometimes AMP runtime denies the resize request when there is a difference in both width and height. And indeed we are using multisize with safeframes.

premesh-freestar commented 3 years ago

Ok so I did see a few times the resize seemed to fail however if I tabbed away and back it would fire.. Almost seemed like the called queued and amp runtime was waiting for the opportunity to do the change?

On Wed, Sep 23, 2020, 2:21 AM Muotaz notifications@github.com wrote:

We are already using safe frame. The issue is that sometimes AMP runtime denies the resize request when there is a difference in both width and height. And indeed we are using multisize with safeframes.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/prebid/prebid-universal-creative/issues/119#issuecomment-697243432, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP3ROSFVOQ7ATFAREVN34E3SHG4YLANCNFSM4RLXA7HA .

muotaz commented 3 years ago

Yes I also noticed that, the calls for the resize piled up but AMP was almost always rejecting them. With this change, the resize request fails a couple of times before being executed when the ad is out of the viewport. The master creative there is 320x50 in size. Would changing it to 1x1 resolve the issue ?

bretg commented 2 years ago

@muotaz - is this still an issue for you?

Do you think it might help if the PUC did two resize calls - one for the height and one for the width. (?)

muotaz commented 2 years ago

Hello @bretg

Wow, quite a lot of time has passed since that! I don't think we have had that issue since the time we reported this. Let me check with our Adops dept and I will get back to you with info if this issue is still relevant.

Thanks!

patmmccann commented 2 years ago

@muotaz any luck confirming? Thanks!

muotaz commented 2 years ago

Hello @patmmccann Seems like this is no longer an issue, I will close this and reopen if this happens again. Thanks alot!

GeneGenie commented 9 months ago

Confirming this is still happening. I think that issue is in double resizing. eg: amp-ad has size preset of 300x250, when pbjs creative is loaded AMP resizes container to 1x1 (i suppose AMP does this), and following resize/expand request stays in some amp-reflow queue until window is resizes, or tab switched.

UPD: Changing amp-ad size to 1x1 didn't help, and made even worse, causing endless loop and stack overflows https://ibb.co/cFQyhTw

UPD2: this article probably has the best explanation of failure:

A request to expand is not guaranteed to succeed. You should register a callback with $sf.ext.register() that will execute on geometry changes, including from the results of expansion and collapse.

Expansion Successes and Failures

AMP has very strict rules in place about when an element can expand. These rules are in place to prevent reflow, which is when an element size changes and causes page content to shift, harming user experience. To prevent reflow for SafeFrame expansion, expand will only succeed in the following scenarios:

The expansion request is made while the SafeFrame is not in viewport. If the SafeFrame is not within the user's viewport at the time a valid expansion request is made, the request to expand will succeed. The AMP runtime is able to control scroll position to assure that reflow does not occur as a result of the expansion.

The expansion request is made for an expanded-size that is less than or equal to the size of the parent amp-ad element. All SafeFrames within AMP are nested within an amp-ad element. amp-ad elements are already laid-out within the page to a specific size. If a creative calls expand() such that the expanded size of the SafeFrame will still be entirely contained within the amp-ad, then expand will succeed regardless of whether the SafeFrame is within the viewport or not.

So, i guess conclusion is: if page ended all its reflows before ad was retrieved from PBS cache - register will not trigger any callbacks. And the best advice for publisher would be .... uh? "Do not place hb-rich amp-ads in first viewport and sticky units!" @muotaz could you assist with your experience on this question?!

Thanks