Add amp-story-page element #11510

newmuis · 2017-09-29T22:24:16Z

Implements the page level of the amp-story component.

Depends on #11330, #11485, #11509, and #11525

dreamofabear · 2017-09-29T23:50:56Z

extensions/amp-story/0.1/page-element.js

+
+
+
+export class PageElement {


This is an interesting file. Was adding functionality/events to existing classes like amp-img.js not tenable?

I'm not sure whether the information we're seeking is general-purpose enough to port to those classes. We very likely need some kind of definition of "loaded" that is specific to amp-story, as the tradeoff between seeing loading indicators vs. seeing stuttering video is likely different in the story use-case than the article use-case.

dreamofabear · 2017-09-30T00:02:35Z

extensions/amp-story/0.1/amp-story-page.js

+/**
+ * The delay (in milliseconds) to wait between polling for loaded resources.
+ */
+const LOAD_TIMER_POLL_DELAY_MS = 250;


This seems coarse and will probably add ~125ms latency due to imprecision. Is avoiding polling altogether possible?

Yeah, should be possible. The first pass at this implementation used browser events (e.g. canplaythrough, loadeddata, and progress) and that didn't work well (seems like in practice, browsers differ pretty greatly in how they implement this, and it results in pretty poor results, especially on Android Chrome).

An intermediate implementation used events from each PageElement to the AmpStoryPage, but each element polled internally, since there were no events that got us the desired result. The code written here was a refactor of that to allow only creating a single timer per page, rather than a single timer per element (n timers per page).

Fair enough. Consider including this under an area for future optimization.

Filed #11532

dreamofabear · 2017-09-30T00:04:44Z

extensions/amp-story/0.1/amp-story-page.js

+    this.element.appendChild(loadingScreen);
+
+    // Build a list of page elements and poll until they are all loaded.
+    this.pageElements_ = PageElement.getElementsFromPage(this);


Do we know how performant this is in practice?

There's a timeout (8s), but in practice it seems to be working reasonably, depending on the elements on the page.

We need to do something like this, though; having all the descendants loaded asynchronously whenever they are ready doesn't work well for this format. A potential future optimization is to use the hero attribute (or similar) to allow publishers to mark up which element(s) to wait for, but I think we'll need to default to a conservative approach, as partially loaded pages are very problematic for the format.

8ms I hope? 😅 Is that on a workstation with CPU throttling or on a low-end mobile device?

Sorry, I guess it depends which part you're asking about the performance of. I don't think we've evaluated the performance of the PageElement.getElementsFromPage(this) call, but if you're talking about the comment // Build a list of page elements and poll until they are all loaded., then that's what has the 8s timeout. We'll show a loading indicator/screen for up to 8s after the user has started looking at the screen. It takes so long because we're actually waiting for the assets (i.e. mostly videos) to be fetched over the network.

I see. I was actually curious about the DOM queries. Generally we try to avoid DOM scans in favor of custom element API. For instance, forms would be more robust to dynamic markup changes (e.g. new form elements created from amp-list) if it used an <amp-form> instead of <form> as it does now.

We could devise a kind of dynamic add-on system to avoid the need for scanning here. Only justifiable if the scan is slow or we foresee dynamic markup in STAMP.

dreamofabear

Some small things that can be fixed. As discussed, feel free to address these in follow-up PRs.

dreamofabear · 2017-10-02T15:58:14Z

extensions/amp-story/0.1/amp-story-page.js

+   * @return {!Promise}
+   * @private
+   */
+  maybeApplyFirstAnimationFrame() {


Suffix private functions with _.

This was meant to be public

dreamofabear · 2017-10-02T15:59:15Z

extensions/amp-story/0.1/amp-story-page.js

+    }
+  }
+
+  /** */


Please document this function.

dreamofabear · 2017-10-02T15:59:26Z

extensions/amp-story/0.1/amp-story-page.js

+
+
+  /**
+   * @return {boolean} True if animations were stopped.


@private?

This is actually unused; deleted

dreamofabear · 2017-10-02T15:59:51Z

extensions/amp-story/0.1/amp-story-page.js

+
+  /**
+   * Initializes the loading screen for this amp-story-page, and the listeners
+   * to remove it once loaded.


dreamofabear · 2017-10-02T15:59:56Z

extensions/amp-story/0.1/amp-story-page.js

+
+
+  /**
+   * Marks any AMP elements that represent media elements with preload="auto".


dreamofabear · 2017-10-02T16:07:33Z

extensions/amp-story/0.1/amp-story-page.js

+  isVideoInterfaceVideo_(el) {
+    // TODO(newmuis): Check whether the element has the
+    // i-amphtml-video-interface class, after #11015 from amphtml is merged
+    // into amphtml-story.


This can be fixed now.

dreamofabear · 2017-10-02T16:08:11Z

extensions/amp-story/0.1/amp-story-page.js

+   * advance.
+   * @param {?Element} el
+   * @param {!function()} callback
+   * @return {!Element}


This function doesn't seem to return anything.

dreamofabear · 2017-10-02T16:17:02Z

extensions/amp-story/0.1/amp-story-page.js

+    this.element.appendChild(loadingScreen);
+
+    // Build a list of page elements and poll until they are all loaded.
+    this.pageElements_ = PageElement.getElementsFromPage(this);


8ms I hope? 😅 Is that on a workstation with CPU throttling or on a low-end mobile device?

dreamofabear · 2017-10-02T16:18:30Z

extensions/amp-story/0.1/page-element.js

+class VideoInterfaceElement extends PageElement {
+  constructor(element, page) {
+    super(element, page);
+  }


Minor: Can be removed.

dreamofabear · 2017-10-02T16:20:43Z

extensions/amp-story/0.1/amp-story-page.js

+/**
+ * The delay (in milliseconds) to wait between polling for loaded resources.
+ */
+const LOAD_TIMER_POLL_DELAY_MS = 250;


Fair enough. Consider including this under an area for future optimization.

newmuis · 2017-10-03T16:27:40Z

I think this is ready to be merged once the dependency PR #11509 is merged.

Add amp-story-page element

019fba2

newmuis requested review from dvoytenko and dreamofabear September 29, 2017 22:24

googlebot added the cla: yes label Sep 29, 2017

This was referenced Sep 29, 2017

Send PRs to merge amphtml-story into the amphtml repo newmuis/amphtml-story#180

Closed

Add the core amp-story component #11511

Merged

dreamofabear reviewed Sep 30, 2017

View reviewed changes

dreamofabear approved these changes Oct 2, 2017

View reviewed changes

newmuis added this to Merge in Stories - By Type Oct 2, 2017

newmuis moved this from Pending to Ready to Merge in Stories - By Type Oct 2, 2017

newmuis moved this from Approved to Needs Work in Stories - By Type Oct 2, 2017

newmuis mentioned this pull request Oct 2, 2017

Avoid polling for page element load status #11532

Closed

newmuis added 2 commits October 2, 2017 16:17

Update amp-story-page.js

d246495

Update amp-story-page.js

3aebe86

newmuis mentioned this pull request Oct 2, 2017

Refactor loaded states for PageElement #11534

Closed

newmuis added 4 commits October 2, 2017 16:31

Update page-element.js

afdd81e

Update amp-story-page.js

4435463

Update amp-story-page.js

8627898

Update amp-story-page.js

be4eedf

ampprojectbot added this to the Pending Triage milestone Oct 2, 2017

newmuis added 3 commits October 3, 2017 07:46

Fix max line lengths in amp-story-page.js

7ed19c7

Use scopedQuerySelector(All) in page-element.js

c62ce21

Fix missing variable name in type annotation

2278156

newmuis moved this from Needs Work to Awaiting Review in Stories - By Type Oct 3, 2017

newmuis moved this from Awaiting Review to Ready to Merge (approved + comments addressed) in Stories - By Type Oct 3, 2017

Fix return type for getAllMedia_()

58be6ba

alanorozco merged commit d9b081d into ampproject:master Oct 3, 2017

newmuis moved this from Ready to Merge (approved + comments addressed) to Merged in Stories - By Type Oct 3, 2017

newmuis moved this from Migration to amphtml repo to Done in Stories - By Type Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add amp-story-page element #11510

Add amp-story-page element #11510

newmuis commented Sep 29, 2017 •

edited

dreamofabear Sep 29, 2017

newmuis Oct 2, 2017

dreamofabear Sep 30, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Sep 30, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

dreamofabear left a comment

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis Oct 2, 2017

dreamofabear Oct 2, 2017

dreamofabear Oct 2, 2017

dreamofabear Oct 2, 2017

newmuis commented Oct 3, 2017

+                  }
+                }
+                /** */



		/**
		* Marks any AMP elements that represent media elements with preload="auto".

Add amp-story-page element #11510

Add amp-story-page element #11510

Conversation

newmuis commented Sep 29, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis commented Oct 3, 2017

newmuis commented Sep 29, 2017 •

edited