Collapse empty ads #1265

camelburrito · 2015-12-29T21:37:46Z

cramforce · 2015-12-30T01:16:03Z

builtins/amp-ad.js

@@ -283,6 +283,10 @@ export function installAd(win) {
     * @private
     */
    noContentHandler_() {
+      if (!this.placeholder_) {
+        this.element.style.height = 0;


display: none please.

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

Yes, I don't see why we shouldn't do display:none.

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.

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.

dvoytenko · 2016-01-04T16:49:22Z

src/resources.js

+    * }}
+    * @private
+    */
+   let ChangeHeightRequest;


ChangeHeightRequestDef - see "Def" suffix.

dvoytenko · 2016-01-04T16:52:32Z

All looks good. I left a couple of minor comments. The big thing, however, we need to confirm how we work with placeholder here. My sense is that fallback and placeholder are interoperable here and we need some basic priority mechanism for them.

cramforce · 2016-01-04T17:03:23Z

builtins/amp-ad.js

-      this.element.removeChild(this.iframe_);
-      this.toggleFallback(true);
+      if (!this.fallback_) {
+        this.changeHeight(0, () => {


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.

camelburrito · 2016-01-05T23:40:43Z

src/resources.js

+    * }}
+    * @private
+    */
+   let ChangeHeightRequestDef;


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

camelburrito · 2016-01-06T18:44:35Z

PTAL

dvoytenko · 2016-01-06T18:54:52Z

builtins/amp-ad.js

-      this.toggleFallback(true);
+      if (!this.fallback_) {
+        this./*REVIEW*/changeHeightIfAllowed(0, () => {
+          this.deferMutate(() => this.element.style.display = 'none');


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

dvoytenko · 2016-01-06T18:56:16Z

Couple more comments.

dvoytenko · 2016-01-06T19:36:47Z

LGTM after issue with naming is resolved.

cramforce · 2016-01-06T20:04:32Z

builtins/amp-ad.md

@@ -64,13 +64,23 @@ Optional attribute to pass configuration to the ad as an arbitrarily complex JSO



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.

cramforce · 2016-01-06T20:57:11Z

builtins/amp-ad.js

@@ -283,8 +283,17 @@ export function installAd(win) {
     * @private
     */
    noContentHandler_() {
-      this.element.removeChild(this.iframe_);
-      this.toggleFallback(true);
+      if (!this.fallback_) {


Add comment explaining what happens

cramforce · 2016-01-06T20:57:17Z

LGTM

Collapse empty ads (When possible)

camelburrito added the NEEDS REVIEW label Dec 29, 2015

camelburrito assigned cramforce Dec 29, 2015

camelburrito force-pushed the collapse_ad branch from f136819 to 134eb1a Compare December 29, 2015 22:17

camelburrito assigned dvoytenko and unassigned cramforce Dec 29, 2015

cramforce reviewed Dec 30, 2015
View reviewed changes

camelburrito force-pushed the collapse_ad branch 2 times, most recently from 99fdbd1 to 565f479 Compare December 31, 2015 00:39

dvoytenko reviewed Jan 4, 2016
View reviewed changes

src/resources.js

* }}

* @private

*/

let ChangeHeightRequest;

Copy link

Contributor

dvoytenko Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChangeHeightRequestDef - see "Def" suffix.

cramforce reviewed Jan 4, 2016
View reviewed changes

camelburrito force-pushed the collapse_ad branch 2 times, most recently from bdf9405 to fd8210a Compare January 5, 2016 23:39

camelburrito reviewed Jan 5, 2016
View reviewed changes

src/resources.js

* }}

* @private

*/

let ChangeHeightRequestDef;

Copy link

Contributor Author

camelburrito Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

camelburrito force-pushed the collapse_ad branch 4 times, most recently from ea23b34 to facc71b Compare January 6, 2016 01:16

dvoytenko reviewed Jan 6, 2016
View reviewed changes

camelburrito force-pushed the collapse_ad branch 2 times, most recently from ca0a6a7 to b98882d Compare January 6, 2016 19:34

dvoytenko added the LGTM label Jan 6, 2016

dvoytenko removed the NEEDS REVIEW label Jan 6, 2016

camelburrito force-pushed the collapse_ad branch from b98882d to d80720d Compare January 6, 2016 19:50

cramforce reviewed Jan 6, 2016
View reviewed changes

camelburrito force-pushed the collapse_ad branch from d80720d to 4368721 Compare January 6, 2016 20:20

cramforce reviewed Jan 6, 2016
View reviewed changes

Collapse empty ads (When possible)

b2fe74a

camelburrito force-pushed the collapse_ad branch from 4368721 to b2fe74a Compare January 6, 2016 21:09

camelburrito pushed a commit that referenced this pull request Jan 6, 2016

Merge pull request #1265 from sriramkrish85/collapse_ad

bdb2e5f

Collapse empty ads (When possible)

camelburrito merged commit bdb2e5f into ampproject:master Jan 6, 2016

camelburrito deleted the collapse_ad branch January 6, 2016 21:39

jasti mentioned this pull request Jan 13, 2016

Don't see empty ads being collapsed #1412

Closed

elijahsoria mentioned this pull request Sep 24, 2020

SwG Release 0.1.22.121 #30383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collapse empty ads #1265

Collapse empty ads #1265

camelburrito commented Dec 29, 2015

cramforce Dec 30, 2015

dvoytenko Dec 30, 2015

dvoytenko Dec 30, 2015

camelburrito Dec 30, 2015

dvoytenko Dec 30, 2015

camelburrito Dec 30, 2015

camelburrito Dec 31, 2015

dvoytenko Jan 4, 2016

dvoytenko commented Jan 4, 2016

cramforce Jan 4, 2016

camelburrito Jan 5, 2016

cramforce Jan 5, 2016

cramforce Jan 6, 2016

camelburrito Jan 5, 2016

camelburrito commented Jan 6, 2016

dvoytenko Jan 6, 2016

camelburrito Jan 6, 2016

dvoytenko commented Jan 6, 2016

dvoytenko commented Jan 6, 2016

cramforce Jan 6, 2016

camelburrito Jan 6, 2016

cramforce Jan 6, 2016

camelburrito Jan 6, 2016

cramforce commented Jan 6, 2016

		@@ -64,13 +64,23 @@ Optional attribute to pass configuration to the ad as an arbitrarily complex JSO

Collapse empty ads #1265

Collapse empty ads #1265

Conversation

camelburrito commented Dec 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jan 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelburrito commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jan 6, 2016

dvoytenko commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Jan 6, 2016