Amphtml: Flying Carpet collapsing empty ad slot is broken

[bradfrizzell]

The spec'd behavior of flying carpet is that when a child element collapses due to no-content, the flying carpet is supposed to also collapse. For instance, when an amp-ad gets an empty response and attempts to collapse (when outside viewport) the flying carpet should also collapse. However, this functionality is broken. In my testing just now, it seems that mutateWorkViaResources is not working as intended. Let me describe the flow.

WHAT SHOULD HAPPEN

1. Below-viewport Amp-ad no-fills
2. Amp-ad calls uiHandler.applyNoContentUI()
3. Amp-ad collapses
4. Flying carpet collapses

WHAT IS ACTUALLY HAPPENING

1. Below-viewport Amp-ad no-fills
2. Amp-ad calls uiHandler.applyNoContentUI()
3. Mutate work via resources gets the layout box for the amp-ad, which it sees as being above the viewport
4. Amp-ad collapse is rejected
5. flying carpet never attempts collapse

It seems to me that the way that flying carpet works was perhaps changed? At this stage, the amp-ad inside the flying carpet is position fixed so that it's always in the viewport (even when the flying carpet window is out of viewport / you can't see any of the amp-ad)

[zhouyx]

Thank you @bradfrizzell and @coreybyrnes

I am able to reproduce the issue and locate the cause. The reason why <amp-ad> collapse request is rejected is because the ad is applied position:fixed in the background. This is how we implemented the flying carpet effect. Because of that, AMP runtime will not be able to collapse the ad because it always thinks the ad is within viewport.

One thing I can do is to always collapse the ad in this case, because collapsing a position fixed element won't cause a page jump. However this won't work if the <amp-fx-flying-carpet> contains more than one childNodes. As collapsing one nodes, could lead to content jump within the flying carpet.

For this specific use case. I don't believe there are multiple child elements involved. (As <amp-ad> is the only child of <amp-fx-flying-carpet>), and we could create a solution for this specific case. I propose we force collapse an ad if it is the only child of the flying carpet component in the case of no fill.

@jridgewell Does that sounds good to you? Thank you

[coreybyrnes]

@zhouyx THANK YOU! I've been trying to accomplish this for over a year. Much appreciated.

here's a test page with a no-fill as the only child of the flying-carpet:
http://rev.cbsi.com/corey/test/amp/amp_demo_flying-carpet_nofill.html

[zhouyx]

Thank you @coreybyrnes for the test page.

Here's the example use case

<amp-fx-flying-carpet layout="fixed-height" height="50vh">
  <amp-ad>...</amp-ad>
</amp-fx-flying-carpet>

@jridgewell If you agree with proposal, do you think we should force collapse all component if it is the only child of <amp-fx-flying-carpet> or only <amp-ad>? Thanks

[jridgewell]

Sounds alright to me. Flying carpet container is a special case since it won't cause total page adjustments.

[zhouyx]

Thank you @jridgewell.

And thank you @torch2424 for offering to help : ) @coreybyrnes We are prioritizing the fix. Will keep you updated.

[coreybyrnes]

@zhouyx @torch2424 I see this was closed just now but I'm not seeing a no-fill Flying Carpet collapse on my demo page:
http://rev.cbsi.com/corey/test/amp/amp_demo_flying-carpet_nofill.html

[torch2424]

@coreybyrnes Thanks for reaching out and noticing! 😄

So we cut releases once a week, but firs they go through one week of canary, and then on the second week go to prod.

But since this is the holiday time, I know we are going to have some short freezes. But on average, it takes about 2 weeks from when an issue is closed / PR is merged, before hitting production 😄

Let me know if you have any more questions, feel free to reach out!

[coreybyrnes]

Thanks @torch2424! I'm still not seeing the bug fixed on my demo page, so I want to make sure it didn't push to prod yet and to get an idea of when it might push. Thanks!

[torch2424]

@coreybyrnes You are welcome!

So our holiday code freeze ends at 1/2 (today). So changes / fixes should be hitting prod around 1/8. Feel free to reach out around then, if the change still isn't working for you 😄 Thanks!

[zhouyx]

@coreybyrnes In the meanwhile, you should be able to test canary by opt-in to our dev channel here. Thank you

[coreybyrnes]

@zhouyx Thanks that's really helpful. I enabled the dev channel and was able to verify that it works!

[coreybyrnes]

It's working in production! We can finally run a Flying Carpet! This is fantastic. Thanks @bradfrizzell for diagnosing the issue and getting this ticket rolling. Thanks to @torch2424 for implementing the fix.

[torch2424]

@coreybyrnes Thanks for reporting back! Glad it works for you! 🎉 You are welcome 😄