display: none please.

It should only do this when resizing is allowed. @dvoytenko can point you at the relevant code.

1. Yes, I don't see why we shouldn't do display:none.
2. Malte is correct, all DOM mutations here should go via this.deferMutate(callback). It ensures that (a) the changes are done in the mutate phase and (b) they don't interfere with scrolling.
3. toggleFallback(true) combined with display = none or height = 0 makes no sense.

Let's clear up this issues. Another problem is that just setting display to none (or height to 0) directly won't be able to adjust scroll position. If that's desirable, we'd need to do a bit more work here.

Made it display:none

also the noContentHandler_ method is called with defer mutate so i dont think ill have to do it here again.

this.deferMutate(this.noContentHandler_.bind(this));

@dvoytenko any tips on how to adjust scroll position?

Thinking this through again. The only way to adjust scroll position is to go back to, initially, setting height to 0. Not directly, but rather via this.changeHeight API. What we need to do is schedule display:none via deferMutate right after changeHeight has completed. We currently don't have that option - so we need to add it. E.g. we need to add an optional callback to changeHeight API that will be called as soon as it has completed. So, it will look like this:

this.changeHeight(0, () => {
  this.deferMutate(() => display = 'none');
})

However, one more thing,

if (!this.placeholder_) {
    this.element.style.display = 'none';
} else {
    this.toggleFallback(true);
}

I still don't think that this is correct. Placeholder and fallback are not necessarily related. There's some confusion going on here. For instance, if there's no placeholder, but there's fallback, should we still hide the whole element?

Nice co-incidence - i was just pinging you about this

we have a fallback and a placeholder implemented
the md file does not say anything about the fallback
but the fallback is the only thing that is displayed when there is no ad content

I am going to update the MD file to reflect usage for fallback

@dvoytenko updated the code to have optional callbacks on change height - let me know if this is correct - ill update the tests and get this in.

ChangeHeightRequestDef - see "Def" suffix.

Does the height and the display:none happen in the same instant?

Does this do nothing if resize is not allowed?

Does the height and the display:none happen in the same instant?
No it does not -

Does this do nothing if resize is not allowed?
changed it to use requestChangeHeight , which only changes height if allowed.

It would be important to do it in the same instant to reduce the number of style recalcs and layouts to 1. Setting display to none is a much cheaper op than setting height to 0, because the former doesn't require layouting the children.

Ping on this.

Done - Where do i see "Def" suffix info?

As Malte requested - he wanted to do it in the same mutate context. If so - let's do.

Done

Add a sentence:

The amp-ad tag will be collapsed (set to display: none) if the ad sends a message that the ad slot cannot be filled and AMP determines that this operation can be performed without affecting the user's scroll position.

Done

Add comment explaining what happens

Done