✨ <amp-next-page> v2 default and templated separator elements (#26413)

* Added visual diff tests and better manual tests * Prototyping sticky element handling in amp-next-page * Deprecate amp-next-page-keep in favor of amp-next-page-hidden * Visibility bug fix * Remove animation added for testing * Exported host-page-specific parameters into HostPage * Preventing next-page form building again * Remove visual diff tests * Implement infinite loading for amp-next-page v2 * Implements un-loading and re-loading pages to reduce memory footprint * Added unit tests * Fixes types * Implemented a default separator pill and templating for the separator * Types fix * Fix tests for renaming * Added unit tests for separators * Fix types * Revert renaming and fixes (separate PR) * Suggested changes (1/2) * Suggested changes (2/2)
ampproject · Jan 31, 2020 · e7a8e7f · e7a8e7f
1 parent be3317a
commit e7a8e7f
Show file tree

Hide file tree

Showing 6 changed files with 657 additions and 121 deletions.
diff --git a/extensions/amp-next-page/1.0/amp-next-page.css b/extensions/amp-next-page/1.0/amp-next-page.css
@@ -27,6 +27,25 @@
   overflow: visible;
 }
 
+.amp-next-page-default-separator {
+  position: relative;
+  display: flex;
+  flex-direction: row;
+  justify-content: center;
+  align-items: center;
+  text-align: center;
+  font-family: 'Roboto', Helvetica, Arial, sans-serif;
+  font-weight: 400;
+  font-size: 14px;
+  width: 160px;
+  height: 38px;
+  margin: auto;
+  border-radius: 32px;
+  box-shadow: 0px 6px 17px 0px rgba(0, 0, 0, 0.09);
+  background: black;
+  color: white;
+}
+
 /** Fixes collapsing margins when switching between the visible and hidden states */
 .i-amphtml-next-page-document:before,
 .i-amphtml-next-page-document:after {

diff --git a/extensions/amp-next-page/1.0/page.js b/extensions/amp-next-page/1.0/page.js
@@ -53,7 +53,7 @@ export class Page {
     /** @private {?Element} */
     this.container_ = null;
     /** @private {?Document} */
-    this.cachedContent_ = null;
+    this.content_ = null;
     /** @private {!PageState} */
     this.state_ = PageState.QUEUED;
     /** @private {!VisibilityState} */
@@ -193,7 +193,7 @@ export class Page {
    * root has been removed
    */
   resume() {
-    this.attach_(devAssert(this.cachedContent_));
+    this.attach_();
   }
 
   /**
@@ -234,13 +234,14 @@ export class Page {
       .fetchPageDocument(this)
       .then(content => {
         this.state_ = PageState.LOADED;
-        this.container_ = this.manager_.createDocumentContainerForPage(
-          this /** page */
-        );
+        this.content_ = content;
+        return this.manager_.createDocumentContainerForPage(this /** page */);
+      })
+      .then(container => {
+        this.container_ = container;
         // TODO(wassgha): To further optimize, this should ideally
         // be parsed from the service worker instead of stored in memory
-        this.cachedContent_ = content;
-        this.attach_(content);
+        this.attach_();
       })
       .catch(() => {
         this.state_ = PageState.FAILED;
@@ -251,12 +252,11 @@ export class Page {
 
   /**
    * Inserts the fetched (or cached) HTML as the document's content
-   * @param {!Document} content
    */
-  attach_(content) {
+  attach_() {
     const shadowDoc = this.manager_.attachDocumentToPage(
       this /** page */,
-      content,
+      /** @type {!Document} */ (devAssert(this.content_)),
       this.isPaused() /** force */
     );
 

diff --git a/extensions/amp-next-page/1.0/service.js b/extensions/amp-next-page/1.0/service.js
@@ -23,11 +23,13 @@ import {
   childElementByAttr,
   childElementsByTag,
   isJsonScriptTag,
+  removeChildren,
   removeElement,
   scopedQuerySelector,
 } from '../../../src/dom';
 import {dev, devAssert, user, userAssert} from '../../../src/log';
 import {escapeCssSelectorIdent} from '../../../src/css';
+import {htmlFor} from '../../../src/static-template';
 import {installStylesForDoc} from '../../../src/style-installer';
 import {
   parseFavicon,
@@ -77,6 +79,9 @@ export class NextPageService {
      */
     this.mutator_ = Services.mutatorForDoc(ampdoc);
 
+    /** @private @const {!../../../src/service/template-impl.Templates} */
+    this.templates_ = Services.templatesFor(this.win_);
+
     /** @private {?Element} */
     this.separator_ = null;
 
@@ -380,22 +385,26 @@ export class NextPageService {
    * Create a container element for the document and insert it into
    * the amp-next-page element
    * @param {!Page} page
-   * @return {!Element}
+   * @return {!Promise<!Element>}
    */
   createDocumentContainerForPage(page) {
     const container = this.win_.document.createElement('div');
     container.classList.add(DOC_CONTAINER_CLASS);
+    this.element_.insertBefore(container, dev().assertElement(this.moreBox_));
+
+    // Insert the separator
+    const separatorInstance = this.separator_.cloneNode(true);
+    container.appendChild(separatorInstance);
+    const separatorRenderPromise = this.maybeRenderSeparatorTemplate_(
+      separatorInstance,
+      page
+    );
 
+    // Insert the document
     const shadowRoot = this.win_.document.createElement('div');
     shadowRoot.classList.add(SHADOW_ROOT_CLASS);
     container.appendChild(shadowRoot);
 
-    // Insert the separator
-    container.appendChild(this.separator_.cloneNode(true));
-
-    // Insert the container
-    this.element_.insertBefore(container, this.moreBox_);
-
     // Observe this page's visibility
     this.visibilityObserver_.observe(
       shadowRoot /** element */,
@@ -406,7 +415,7 @@ export class NextPageService {
       }
     );
 
-    return container;
+    return separatorRenderPromise.then(() => container);
   }
 
   /**
@@ -632,7 +641,10 @@ export class NextPageService {
         doc.close();
         return doc;
       })
-      .catch(e => user().error(TAG, 'failed to fetch %s', page.url, e));
+      .catch(e => {
+        user().error(TAG, 'failed to fetch %s', page.url, e);
+        throw e;
+      });
   }
 
   /**
@@ -715,11 +727,17 @@ export class NextPageService {
         'be inside a <script> tag with type="application/json"'
     );
 
-    const pages = tryParseJson(scriptElement.textContent, error => {
+    const parsed = tryParseJson(scriptElement.textContent, error => {
       user().error(TAG, 'failed to parse inline page list', error);
     });
 
-    return user().assertArray(pages, `${TAG} page list should be an array`);
+    const pages = user().assertArray(
+      parsed,
+      `${TAG} page list should be an array`
+    );
+
+    removeElement(scriptElement);
+    return pages;
   }
 
   /**
@@ -731,7 +749,6 @@ export class NextPageService {
    */
   getSeparatorElement_(element) {
     const providedSeparator = childElementByAttr(element, 'separator');
-    // TODO(wassgha): Use templates (amp-mustache) to render the separator
     if (providedSeparator) {
       removeElement(providedSeparator);
     }
@@ -743,9 +760,42 @@ export class NextPageService {
    * @private
    */
   buildDefaultSeparator_() {
-    const separator = this.win_.document.createElement('div');
-    separator.classList.add('amp-next-page-default-separator');
-    return separator;
+    const html = htmlFor(this.getHostNextPageElement_());
+    return html`
+      <div
+        class="amp-next-page-default-separator"
+        aria-label="Next article separator"
+      >
+        Next article
+      </div>
+    `;
+  }
+
+  /**
+   * Renders the template inside the separator element using
+   * data from the current article (if a template is present)
+   *
+   * @param {!Element} separator
+   * @param {!Page} page
+   * @return {!Promise}
+   */
+  maybeRenderSeparatorTemplate_(separator, page) {
+    if (!this.templates_.hasTemplate(separator)) {
+      return Promise.resolve();
+    }
+    const data = /** @type {!JsonObject} */ ({
+      title: page.title,
+      url: page.url,
+      image: page.image,
+    });
+    return this.templates_
+      .findAndRenderTemplate(separator, data)
+      .then(rendered => {
+        return this.mutator_.mutateElement(separator, () => {
+          removeChildren(dev().assertElement(separator));
+          separator.appendChild(rendered);
+        });
+      });
   }
 
   /**