amp-list: Support diffable with single-item

extensions/amp-list/0.1/amp-list.js

@@ -195,9 +195,7 @@ export class AmpList extends AMP.BaseElement {
       const doc = this.element.ownerDocument;
       const isEmail = doc && isAmp4Email(doc);
       const hasPlaceholder =
-        this.getPlaceholder() ||
-        (this.element.hasAttribute('diffable') &&
-          this.queryDiffablePlaceholder_());
+        this.getPlaceholder() || this.element.hasAttribute('diffable');
       if (isEmail) {
         if (!hasPlaceholder) {
           user().warn(
@@ -264,11 +262,7 @@ export class AmpList extends AMP.BaseElement {
       if (this.diffablePlaceholder_) {
         this.container_ = this.diffablePlaceholder_;
       } else {
-        user().warn(
-          TAG,
-          'Could not find child div[role=list] for diffing.',
-          this.element
-        );
+        user().warn(TAG, 'Could not find child div for diffing.', this.element);
       }
     }
     if (!this.container_) {
@@ -326,20 +320,28 @@ export class AmpList extends AMP.BaseElement {
   }
 
   /**
-   * A "diffable placeholder" is just a div[role=list] child without the
-   * [placeholder] attribute.
+   * A "diffable placeholder" is the child container <div> (which is usually created by the amp-list
+   * to hold the rendered children). It serves the same purpose as a placeholder, except it can be diffed.
    *
-   * It's used to display pre-fetch (stale) list content that can be
-   * DOM diffed with the fetched (fresh) content later.
+   * For example:
+   * <amp-list>
+   *   <div placeholder>I'm displayed before render.</div>
+   * </amp-list>
+   *
+   * <amp-list diffable>
+   *   <div role=list>I'm displayed before render.</div>
+   * </amp-list>
    *
    * @return {?Element}
    * @private
    */
   queryDiffablePlaceholder_() {
-    return scopedQuerySelector(
-      this.element,
-      '> div[role=list]:not([placeholder]):not([fallback]):not([fetch-error])'
-    );
+    let selector = this.element.hasAttribute('single-item')
+      ? '> div'
+      : '> div[role=list]';
+    // Don't select other special <div> children used for placeholders/fallback/etc.
+    selector += ':not([placeholder]):not([fallback]):not([fetch-error])';
+    return scopedQuerySelector(this.element, selector);
   }
 
   /**
@@ -1066,21 +1068,19 @@ export class AmpList extends AMP.BaseElement {
 
     // binding=refresh: Only do render-blocking update after initial render.
     if (binding && binding.startsWith('refresh')) {
-      // Bind service must be available after first mutation, so don't
-      // wait on the async service getter.
-      if (this.bind_ && this.bind_.signals().get('FIRST_MUTATE')) {
+      // Don't bother using bindForDocOrNull() since the Bind service must be available after first mutate.
+      const afterFirstMutate =
+        this.bind_ && this.bind_.signals().get('FIRST_MUTATE');
+      if (afterFirstMutate) {
         return updateWith(this.bind_);
       } else {
-        // On initial render, do a non-blocking scan and don't update.
-        Services.bindForDocOrNull(this.element).then((bind) => {
-          if (bind) {
-            const evaluate = binding == 'refresh-evaluate';
-            bind.rescan(elements, [], {
-              'fast': true,
-              'update': evaluate ? 'evaluate' : false,
-            });
-          }
-        });
+        // This must be initial render, so do a non-blocking scan for bindings only.
+        // [diffable] is a special case that is handled later in render_(), see comment there.
+        if (!this.element.hasAttribute('diffable')) {
+          this.scanForBindings_(elements, []);
+        }
+
+        // Don't block render and return synchronously.
         return Promise.resolve(elements);
       }
     }
@@ -1095,6 +1095,32 @@ export class AmpList extends AMP.BaseElement {
     });
   }
 
+  /**
+   * Scans for bindings in `addedElements` and removes bindings in `removedElements`.
+   * Unlike updateBindings(), does NOT apply bindings or update DOM.
+   * Should only be used for binding="refresh" or binding="refresh-evaluate".
+   * @param {!Array<!Element>} addedElements
+   * @param {!Array<!Element>} removedElements
+   * @private
+   */
+  scanForBindings_(addedElements, removedElements) {
+    const binding = this.element.getAttribute('binding');
+    if (!binding || !binding.startsWith('refresh')) {
+      return;
+    }
+    Services.bindForDocOrNull(this.element).then((bind) => {
+      if (bind) {
+        // For binding="refresh-evaluate", we scan for bindings, evaluate+cache expressions, but skip DOM update.
+        // For binding="refresh", we only scan for bindings.
+        const update = binding == 'refresh-evaluate' ? 'evaluate' : false;
+        bind.rescan(addedElements, removedElements, {
+          'fast': true,
+          'update': update,
+        });
+      }
+    });
+  }
+
   /**
    * @param {!Array<!Element>} elements
    * @param {boolean=} opt_append
@@ -1107,10 +1133,18 @@ export class AmpList extends AMP.BaseElement {
     const renderAndResize = () => {
       this.hideFallbackAndPlaceholder_();
 
-      if (this.element.hasAttribute('diffable') && container.hasChildNodes()) {
+      if (this.element.hasAttribute('diffable')) {
         // TODO:(wg-performance)(#28781) Ensure owners_.scheduleUnlayout() is
         // called for diff elements that are removed
         this.diff_(container, elements);
+
+        // For diffable amp-list, we have to wait until DOM diffing is done here to scan for new bindings
+        // (vs. asynchronously in updateBindings()), and scan the entire container (vs. just `elements`).
+        //
+        // This is because instead of replacing the entire DOM subtree, the diffing process removes
+        // select children from `elements` and inserts them into the container. This results in a race
+        // between diff_() and Bind.rescan(), which we avoid by delaying the latter until now.
+        this.scanForBindings_([container], [container]);
       } else {
         if (!opt_append) {
           this.owners_./*OK*/ scheduleUnlayout(this.element, container);

extensions/amp-list/0.1/test/test-amp-list.js

@@ -770,7 +770,7 @@ describes.repeated(
             it('should attemptChangeHeight initial content', async () => {
               const initialContent = doc.createElement('div');
               initialContent.setAttribute('role', 'list');
-              initialContent.style.height = '1337px';
+              initialContent.setAttribute('style', 'height: 123px');
 
               // Initial content must be set before buildCallback(), so use
               // a new test AmpList instance.
@@ -785,11 +785,12 @@ describes.repeated(
               // content, once to resize to rendered contents.
               listMock
                 .expects('attemptChangeHeight')
-                .withExactArgs(1337)
+                .withExactArgs(123)
                 .returns(Promise.resolve())
                 .twice();
 
               const itemElement = doc.createElement('div');
+              itemElement.setAttribute('style', 'height: 123px');
               expectFetchAndRender(DEFAULT_FETCHED_DATA, [itemElement]);
               await list.layoutCallback();
             });
@@ -1416,6 +1417,50 @@ describes.repeated(
                 }
               );
             });
+
+            describe('rescan vs. diff race', () => {
+              async function rescanVsDiffTest() {
+                env.sandbox.spy(list, 'diff_');
+                env.sandbox.spy(list, 'render_');
+
+                // Diffing is skipped if there's no existing children to diff against.
+                const oldChild = doc.createElement('p');
+                oldChild.textContent = 'foo';
+                list.container_.appendChild(oldChild);
+
+                const newChild = doc.createElement('p');
+                newChild.textContent = 'bar';
+                // New children must have at least one binding to trigger rescan.
+                newChild.setAttribute('i-amphtml-binding', '');
+                const rendered = expectFetchAndRender(DEFAULT_FETCHED_DATA, [
+                  newChild,
+                ]);
+                await list.layoutCallback().then(() => rendered);
+              }
+
+              it('without diffing, should rescan _before_ render', async () => {
+                await rescanVsDiffTest();
+
+                expect(list.diff_).to.not.have.been.called;
+                expect(bind.rescan).to.have.been.calledOnce;
+
+                // Without diffable, rescan should happen before rendering the new children.
+                expect(bind.rescan).calledBefore(list.render_);
+              });
+
+              it('with diffing, should rescan _after_ render/diff', async () => {
+                element.setAttribute('diffable', '');
+
+                await rescanVsDiffTest();
+
+                expect(list.diff_).to.have.been.calledOnce;
+                expect(bind.rescan).to.have.been.calledOnce;
+
+                // With diffable, rescanning must happen after rendering (diffing) the new children.
+                expect(bind.rescan).calledAfter(list.render_);
+                expect(bind.rescan).calledAfter(list.diff_);
+              });
+            });
           });
 
           describe('binding="no"', () => {