Always collapse sticky-ad

[zhouyx]

closes #6184

Only ad rendered in cross domain iframe has ad container value calculated before. Thus I decided to keep the code cleaner by always calculating container again.
I can also use the pre-calculate container value from baseInstance If anyone thinks the call of #getAdContainer is expensive https://github.com/ampproject/amphtml/blob/master/src/ad-helper.js#L77

@jridgewell I believe we will not force collapse an ad in flying-carpet?

[jridgewell]

I think this entire PR is unnecessary due to #7612. If there are issues with collapsing a fixed position ad (that won't cause a scroll adjustment), I'd rather update #attemptChangeSize (and by extension #attemptCollapse) to handle the case.

[jridgewell]

I can also use the pre-calculate container value from baseInstance

👍

[jridgewell]

We can extend this to amp-fx-flying-carpet, too.

[zhouyx]

I thought about always collapsing a fixed position ad (that won't cause a scroll adjustment) before. But in the case of scrollable lightbox, ad collapsing can still cause scroll adjustment. So restricting container to be either sticky-ad or flying-carpet is still required.
#attemptCollapse sounds like a good place to handle the case for me.

[zhouyx]

@jridgewell I thought about it a little more and started to think amp-ad-ui.js is the best place to add the check.
#attemptCollapse is a general method for all amp elements, I should not add the check in base-element.js. Then it is also not ideal that I override the method twice for a4a and 3pAd.
What do you think?

[jridgewell]

is a general method for all amp elements, I should not add the check in base-element.js

This also affects iframe's, etc. I've also argued <amp-sticky-ad> should allow generic children, so amp-ad-ui.js isn't enough. We need to consider where the fixed-position parent is, and whether there are any siblings as we traverse up to it. That's a generic Resources (Layers, eventually) problem.

[zhouyx]

Agree that layer manager will eventually does everything here! Will close the issue and wait for layers then 😄

[jridgewell]

This is implementable in Resources, though, and much sooner than I'm going to be able Layers.

// resources-impl.js
for (let i = 0; i < requestsChangeSize.length; i++) {
  // other resize rules

  let fixedPositionParent;
  for (let el = element; el; el = el.parentElement) {
    if (isFixed(el)) {
      fixedPositionParent = el;
      break;
    }
    if (el.nextSibling || el.previousSibling) {
      break;
    }
  }
  

  if (fixedPositionParent) {
    // case 7 or 8 or something
    // Resize allow because changing size of only element in fixed-position element
    resize = true;
  }

[zhouyx]

I thought about it before. However I think it is not always the case that we can resize an element in fixed layer. For example: element resize in amp-sidebar can lead to sidebar jump. element resize in the middle of lightbox can also lead to a page jump.
I would prefer to be more strict.

[jridgewell]

middle of lightbox

Isn't that taken care of by the #nextSibling and #previousSibling checks?

[zhouyx]

Ah you're right!
But I still think collapsing the only element will bring users the impression that the component is broken. (sticky-ad and flying-carpet are two special case as they are not click to display)
What do you think?

[jridgewell]

Maybe pass in an optional containers parameter. If an ancestor tag matches one of the containers, and there are no siblings to consider (the naive case until Layers comes) we can collapse without consequences.

[zhouyx]

Two approaches. 1: have the element decide to force resize or not. 2: have resources manager decide to force resize.

I used approach 1 in this PR because I figured it's a special case only for ads in amp-sticky-ad and amp-fx-flying-carpet and performance wise I didn't want to add the extra check to all other elements. However I agreed approach 2 is reasonable and it will eventually be our solution.

Since I don't have a strong preference, will move the container check to resourses manager then

[zhouyx]

@jridgewell I just realized we can't force collapse amp-fx-flying-carpet, otherwise it will introduce a page jump. So the only special case is amp-sticky-ad. I would then vote for approach 1 and have the special case handled in amp-ad-ui.js right now

[lannka]

I kinda feel this is not generic enough to fit in resource manager. It should only apply to amp-sticky-ad. It might not be good user experience to allow any other fix-positioned element to collapse when in viewport.

[jridgewell]

Alright, let's go with the original review then.

[zhouyx]

rewrite the #getAdContainer function. PTAL