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

Flying Carpet collapsing empty ad slot is broken #19297

Closed
bradfrizzell opened this Issue Nov 13, 2018 · 7 comments

Comments

6 participants
@bradfrizzell
Collaborator

bradfrizzell commented Nov 13, 2018

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

This comment has been minimized.

Collaborator

zhouyx commented Nov 14, 2018

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

This comment has been minimized.

coreybyrnes commented Nov 14, 2018

@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

This comment has been minimized.

Collaborator

zhouyx commented Nov 14, 2018

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

This comment has been minimized.

Member

jridgewell commented Nov 15, 2018

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

@torch2424 torch2424 self-assigned this Nov 16, 2018

@zhouyx

This comment has been minimized.

Collaborator

zhouyx commented Nov 16, 2018

Thank you @jridgewell.

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

@coreybyrnes

This comment has been minimized.

coreybyrnes commented Dec 5, 2018

@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

@lannka lannka added this to To do in Ads+Analytics Fixit Dec 2018 via automation Dec 5, 2018

@lannka lannka moved this from To do to Done in Ads+Analytics Fixit Dec 2018 Dec 5, 2018

@torch2424

This comment has been minimized.

Member

torch2424 commented Dec 5, 2018

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment