amp-list: Temporarily Disable: auto-resize attribute under an exper…

…iment (#18877) * better message * list * validator tests
ampproject · Oct 23, 2018 · ec8cf40 · ec8cf40
1 parent bd08ba5
commit ec8cf40
Show file tree

Hide file tree

Showing 8 changed files with 49 additions and 19 deletions.
diff --git a/css/amp.css b/css/amp.css
@@ -502,10 +502,10 @@ i-amphtml-scroll-container.amp-active, i-amp-scroll-container.amp-active {
 
 /**
  * The default amp-hidden changes visibility, which takes up space. In the case
- * where amp-list becomes a container via the auto-resize attribute, the loader
- * should be fully hidden and not take up space.
+ * where amp-list becomes a container via the resizable-children attribute, the
+ * loader should be fully hidden and not take up space.
  */
-amp-list[auto-resize] > .i-amphtml-loading-container.amp-hidden {
+amp-list[resizable-children] > .i-amphtml-loading-container.amp-hidden {
   display: none !important;
 }
 

diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js
@@ -124,6 +124,9 @@ export class AmpList extends AMP.BaseElement {
     /** @private {?../../../src/service/position-observer/position-observer-impl.PositionObserver} */
     this.positionObserver_ = null;
 
+    /** @private {boolean} */
+    this.hasResizableChildren_ = false;
+
     this.registerAction('refresh', () => {
       if (this.layoutCompleted_) {
         this.resetIfNecessary_();
@@ -133,7 +136,6 @@ export class AmpList extends AMP.BaseElement {
 
     /** @private {?../../../src/ssr-template-helper.SsrTemplateHelper} */
     this.ssrTemplateHelper_ = null;
-
   }
 
   /** @override */
@@ -158,6 +160,25 @@ export class AmpList extends AMP.BaseElement {
       this.element.setAttribute('aria-live', 'polite');
     }
 
+    // auto-resize is deprecated and will be removed per deprecation schedule
+    // It will relaunched under a new attribute (resizable-children) soon.
+    // please see https://github.com/ampproject/amphtml/issues/18849
+    if (this.element.hasAttribute('auto-resize')) {
+      user().warn(TAG, 'auto-resize attribute is deprecated and its behavior' +
+          ' is disabled. This feature will be relaunched under a new name' +
+          ' soon. Please see https://github.com/ampproject/amphtml/issues/18849'
+      );
+    }
+
+    // TODO(aghassemi): New name to be vetted, since under an experiment flag,
+    // going with `resizable-children` fo now but we can change it.
+    this.hasResizableChildren_ =
+      this.element.hasAttribute('resizable-children');
+    if (this.hasResizableChildren_) {
+      user().assert(isExperimentOn(this.win, 'amp-list-resizable-children'),
+          'Experiment amp-list-resizable-children is disabled');
+    }
+
     Services.bindForDocOrNull(this.element).then(bind => {
       this.bind_ = bind;
     });
@@ -548,9 +569,9 @@ export class AmpList extends AMP.BaseElement {
       // Attempt to resize to fit new rendered contents.
       this.attemptToFit_(this.container_);
 
-      if (this.element.hasAttribute('auto-resize')) {
+      if (this.hasResizableChildren_) {
         // If the element's size was changed, change to container layout
-        // if the auto-resize attribute is set.
+        // if the resizable-children attribute is set.
         this.element.signals().whenSignal(CommonSignals.CHANGE_SIZE_END)
             .then(() => this.changeToLayoutContainer_());
       }
@@ -573,7 +594,7 @@ export class AmpList extends AMP.BaseElement {
       if (scrollHeight > height) {
         this.attemptChangeHeight(scrollHeight).catch(() => {});
       } else if (scrollHeight == height
-          && this.element.hasAttribute('auto-resize')) {
+          && this.hasResizableChildren_) {
         this.changeToLayoutContainer_();
       }
     });

diff --git a/extensions/amp-list/0.1/test/validator-amp-list.html b/extensions/amp-list/0.1/test/validator-amp-list.html
@@ -81,7 +81,7 @@
             binding="no">
     <div></div>
   </amp-list>
-  <!-- Valid amp-list with auto-resize attribute -->
+  <!-- Valid amp-list with deprecated auto-resize attribute -->
   <amp-list width=10 height=10
     src="https://data.com/articles.json?ref=CANONICAL_URL"
     auto-resize>

diff --git a/extensions/amp-list/0.1/test/validator-amp-list.out b/extensions/amp-list/0.1/test/validator-amp-list.out
@@ -84,8 +84,10 @@ amp-list/0.1/test/validator-amp-list.html:43:2 The attribute '[state]' in tag 'a
 |              binding="no">
 |      <div></div>
 |    </amp-list>
-|    <!-- Valid amp-list with auto-resize attribute -->
+|    <!-- Valid amp-list with deprecated auto-resize attribute -->
 |    <amp-list width=10 height=10
+>>   ^~~~~~~~~
+amp-list/0.1/test/validator-amp-list.html:85:2 The attribute 'auto-resize' in tag 'amp-list' is deprecated - use 'replacement-to-be-determined-at-a-later-date' instead. (see https://github.com/ampproject/amphtml/issues/18849) [DEPRECATION]
 |      src="https://data.com/articles.json?ref=CANONICAL_URL"
 |      auto-resize>
 |    <div></div>
@@ -119,4 +121,4 @@ amp-list/0.1/test/validator-amp-list.html:107:2 Incomplete layout attributes spe
 |      <div></div>
 |    </amp-list>
 |  </body>
-|  </html>
+|  </html>
diff --git a/extensions/amp-list/validator-amp-list.protoascii b/extensions/amp-list/validator-amp-list.protoascii
@@ -45,6 +45,8 @@ tags: {  # <amp-list> with mandatory src and/or [src] attr
   attrs: {
     name: "auto-resize"
     value: ""
+    deprecation: "replacement-to-be-determined-at-a-later-date"
+    deprecation_url: "https://github.com/ampproject/amphtml/issues/18849"
   }
   attrs: {
     name: "binding"

diff --git a/test/manual/amp-list-resize-flex.amp.html b/test/manual/amp-list-resize-flex.amp.html
@@ -75,7 +75,7 @@ <h2>AMP List Basic</h2>
 
   <div class="flex">
     <amp-img src="/examples/img/sample.jpg" width=80 height=60></amp-img>
-    <amp-list width="396" height="80" layout="flex-item" auto-resize
+    <amp-list width="396" height="80" layout="flex-item" resizable-children
         src="/test/manual/amp-list-data.json?RANDOM">
       <template type="amp-mustache">
         <div class="story-entry">
@@ -94,7 +94,7 @@ <h2>AMP List Basic</h2>
   <h2>AMP List Resize with Accordion</h2>
   <div class="flex">
     <amp-img src="/examples/img/sample.jpg" width=80 height=60></amp-img>
-    <amp-list width="396" height="80" layout="flex-item" auto-resize reset-on-refresh
+    <amp-list width="396" height="80" layout="flex-item" resizable-children reset-on-refresh
         src="/test/manual/amp-list-data.json">
       <template type="amp-mustache">
         <div class="story-entry">
@@ -129,7 +129,7 @@ <h2>AMP List Resize After Toggling Class</h2>
       height="80"
       layout="flex-item"
       reset-on-refresh
-      auto-resize
+      resizable-children
       src="/test/manual/amp-list-data.json?RANDOM">
       <template type="amp-mustache">
         <div class="story-entry" [class]="'story-entry ' + listStyle">
@@ -169,7 +169,7 @@ <h2>AMP List Resize After Changing Src</h2>
       layout="flex-item"
       src="/search/countries"
       reset-on-refresh
-      auto-resize
+      resizable-children
       [src]="'/search/countries?q=' + query">
       <template type="amp-mustache">
         {{#results}}

diff --git a/test/manual/amp-list-resize.amp.html b/test/manual/amp-list-resize.amp.html
@@ -68,7 +68,7 @@ <h1>AMP List Resize Manual Test</h1>
 
   <h2>AMP List Basic</h2>
   <p>This contains an incorrectly sized container where the contents exceed the list height. It should show an overflow button. </p>
-  <amp-list width="396" height="80" layout="responsive" auto-resize reset-on-refresh
+  <amp-list width="396" height="80" layout="responsive" resizable-children reset-on-refresh
       src="/test/manual/amp-list-data.json?RANDOM">
     <template type="amp-mustache">
       <div class="story-entry">
@@ -85,7 +85,7 @@ <h2>AMP List Basic</h2>
 
   <h2>AMP List Basic Below fold</h2>
   <p>This contains an incorrectly sized container where the contents exceed the list height. It should be changed to layout container. </p>
-  <amp-list width="396" height="80" layout="responsive" auto-resize reset-on-refresh
+  <amp-list width="396" height="80" layout="responsive" resizable-children reset-on-refresh
       src="/test/manual/amp-list-data.json?RANDOM">
     <template type="amp-mustache">
       <div class="story-entry">
@@ -99,7 +99,7 @@ <h2>AMP List Basic Below fold</h2>
   </amp-list>
 
   <h2>AMP List Resize with Accordion</h2>
-  <amp-list width="396" height="80" layout=responsive reset-on-refresh auto-resize
+  <amp-list width="396" height="80" layout=responsive reset-on-refresh resizable-children
       src="/test/manual/amp-list-data.json">
     <template type="amp-mustache">
       <div class="story-entry">
@@ -126,7 +126,7 @@ <h1>{{title}} 2</h1>
   <h2>AMP List Resize After Toggling Class</h2>
   <button on="tap:AMP.setState({listStyle: 'grid'})">Show Grid</button>
   <button on="tap:AMP.setState({listStyle: 'list'})">Show List</button>
-  <amp-list width="396" height="80" layout="responsive" auto-resize
+  <amp-list width="396" height="80" layout="responsive" resizable-children
       src="/test/manual/amp-list-data.json?RANDOM">
     <template type="amp-mustache">
       <div class="story-entry" [class]="'story-entry ' + listStyle">
@@ -164,7 +164,7 @@ <h2>AMP List Resize After Changing Src</h2>
     layout=responsive
     src="/search/countries"
     reset-on-refresh
-    auto-resize
+    resizable-children
     [src]="'/search/countries?q=' + query">
     <template type="amp-mustache">
       {{#results}}

diff --git a/tools/experiments/experiments.js b/tools/experiments/experiments.js
@@ -384,6 +384,11 @@ const EXPERIMENTS = [
     name: 'Enables <amp-script>.',
     cleanupIssue: 'https://github.com/ampproject/amphtml/issues/18845',
   },
+  {
+    id: 'amp-list-resizable-children',
+    name: 'Experiment for allowing amp-list to resize when its children resize',
+    cleanupIssue: 'https://github.com/ampproject/amphtml/issues/18875',
+  },
 ];
 
 if (getMode().localDev) {