🐛 Block initial amp-story render on amp-story layout #21382

newmuis · 2019-03-12T17:51:51Z

Followup to #21193. This fixes the previously-broken definition for amp-story, so that it may block on it's layoutCallback before rendering.

Fixes #21349

gmajoulet

Implementation LGTM

gmajoulet · 2019-03-12T18:11:06Z

extensions/amp-story/1.0/amp-story.js

@@ -2360,6 +2360,32 @@ export class AmpStory extends AMP.BaseElement {
  }
 }

+/** @implements {../../../src/render-delaying-services.RenderDelayingService} */
+class AmpStoryRender {


Should we have a separate amp-story-render.js file?

jridgewell · 2019-03-12T22:17:26Z

extensions/amp-story/1.0/amp-story.js

+      return Promise.resolve();
+    }
+
+    return this.storyEl_.signals().whenSignal(CommonSignals.LOAD_END);


You need to wait for storyEL_ to upgrade to an AMP element before calling signals()

This hasn't been addressed.

Should be addressed now 😃

extensions/amp-story/1.0/amp-story.js

torch2424

LGTM 😄 Merge once @jridgewell 's comments are addressed.

Also, thanks for taking this on! 💯

newmuis · 2019-03-27T01:30:17Z

PTAL @jridgewell for changes to amp-story-render-service.js.

gmajoulet · 2019-03-27T02:35:25Z

Could you please host a demo for this? I'd like to test it over different internet connections. If I understand correctly, it will show nothing (completely blank document), until the first page and its media are fully loaded?

Some people actually like the fact that we try to show the text content + CSS before the media is loaded, cf: https://twitter.com/TappableHQ/status/1107950229335277568, they even made a video.
In the case of this video, if I understand correctly, this PR would delay the FMP by 5 or 6 seconds. :(

newmuis · 2019-03-27T12:13:12Z

Sure, I've hosted this here.

I don't think we actually block on the media of the first page. We create a separate promise for what we return in layoutCallback vs. our own internal representation of the story being "loaded".

amphtml/extensions/amp-story/1.0/amp-story.js

Lines 851 to 856 in 1f73910

    
           // Do not block the layout callback on the completion of these promises, as 
        
           // that prevents descendents from being laid out (and therefore loaded). 
        
           storyLayoutPromise.then(() => this.whenPagesLoaded_(PAGE_LOAD_TIMEOUT_MS)) 
        
               .then(() => this.markStoryAsLoaded_()); 
        
           return storyLayoutPromise;

extensions/amp-story/1.0/amp-story-render-service.js

Co-Authored-By: newmuis <newmuis@users.noreply.github.com>

googlebot · 2019-03-29T19:59:58Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-29T20:28:12Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-29T20:28:47Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

jridgewell · 2019-03-29T21:05:35Z

extensions/amp-story/1.0/amp-story-render-service.js

-      return whenUpgradedToCustomElement(storyEl).signals()
+      return whenUpgradedToCustomElement(storyEl).then(() => {
+        return storyEl.signals().whenSignal(CommonSignals.LOAD_END);
+      })
          .whenSignal(CommonSignals.LOAD_END);


Auto suggest bug. 😬

jridgewell · 2019-03-29T21:07:15Z

src/render-delaying-services.js

@@ -39,6 +39,7 @@ import {getServicePromise} from './service';
 const SERVICES = {
  'amp-dynamic-css-classes': '[custom-element=amp-dynamic-css-classes]',
  'variant': 'amp-experiment',
+  'amp-story-render': 'amp-story[standalone]',


The validator team will need to update our list of render-blocking extensions so that this one is injected with a link-preload when necessary.

/cc @Gregable is that something you can do? I'm not sure if it's already there; amp-story previously had a definition that was just broken.

Internally we have b/77581738 filed which is about being able to remove the boilerplate for amp-story and not including a preload link. @newmuis Would the preload link be for all of amp-story (I don't see an amp-story-render-0.1.js being served from the cdn)?

Right now, yes: all of amp-story. We could take it as a separate project to divide amp-story into what's render-blocking vs. what's not, but we're a while away from that.

Filed b/129863998

Annnnd, this is already being done. Apologies for the noise. If you're not seeing a preload please let me know.

Thanks @honeybadgerdontcare!

googlebot · 2019-03-30T14:31:58Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Block initial amp-story render on amp-story layout

a8558e8

newmuis requested review from jridgewell, torch2424, gmajoulet and lannka March 12, 2019 17:51

googlebot added the cla: yes label Mar 12, 2019

gmajoulet approved these changes Mar 12, 2019

View reviewed changes

jridgewell requested changes Mar 12, 2019

View reviewed changes

torch2424 approved these changes Mar 19, 2019

View reviewed changes

torch2424 mentioned this pull request Mar 19, 2019

Fix amp-story render-delaying usage #21349

Closed

Review comments

d4c4a53

newmuis added 2 commits March 27, 2019 22:49

Wait for story to be upgraded

4178d7a

Fix ampdoc type

4e2c109

newmuis requested review from jridgewell and Enriqe March 28, 2019 21:17

jridgewell reviewed Mar 29, 2019

View reviewed changes

extensions/amp-story/1.0/amp-story-render-service.js Outdated Show resolved Hide resolved

Update extensions/amp-story/1.0/amp-story-render-service.js

1b93c16

Co-Authored-By: newmuis <newmuis@users.noreply.github.com>

googlebot added cla: no and removed cla: yes labels Mar 29, 2019

newmuis added cla: yes and removed cla: no labels Mar 29, 2019

Update amp-story-render-service.js

2d43d78

googlebot added cla: no and removed cla: yes labels Mar 29, 2019

Add missing semicolon

c440213

jridgewell reviewed Mar 29, 2019

View reviewed changes

jridgewell approved these changes Mar 29, 2019

View reviewed changes

newmuis added cla: yes and removed cla: no labels Mar 30, 2019

newmuis merged commit a6fdcad into ampproject:master Mar 30, 2019

gmajoulet mentioned this pull request May 29, 2019

Story render service for amp-story 0.1. #22575

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Block initial amp-story render on amp-story layout #21382

🐛 Block initial amp-story render on amp-story layout #21382

newmuis commented Mar 12, 2019 •

edited

gmajoulet left a comment

gmajoulet Mar 12, 2019

jridgewell Mar 12, 2019

jridgewell Mar 27, 2019

newmuis Mar 29, 2019

torch2424 left a comment

newmuis commented Mar 27, 2019

gmajoulet commented Mar 27, 2019

newmuis commented Mar 27, 2019 •

edited

googlebot commented Mar 29, 2019

googlebot commented Mar 29, 2019

googlebot commented Mar 29, 2019

jridgewell Mar 29, 2019

jridgewell Mar 29, 2019

newmuis Mar 30, 2019

honeybadgerdontcare Apr 2, 2019

newmuis Apr 3, 2019 •

edited

honeybadgerdontcare Apr 3, 2019

honeybadgerdontcare Apr 3, 2019

newmuis Apr 3, 2019

googlebot commented Mar 30, 2019

🐛 Block initial amp-story render on amp-story layout #21382

🐛 Block initial amp-story render on amp-story layout #21382

Conversation

newmuis commented Mar 12, 2019 • edited

gmajoulet 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

torch2424 left a comment

Choose a reason for hiding this comment

newmuis commented Mar 27, 2019

gmajoulet commented Mar 27, 2019

newmuis commented Mar 27, 2019 • edited

googlebot commented Mar 29, 2019

googlebot commented Mar 29, 2019

googlebot commented Mar 29, 2019

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 Apr 3, 2019 • 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

googlebot commented Mar 30, 2019

newmuis commented Mar 12, 2019 •

edited

newmuis commented Mar 27, 2019 •

edited

newmuis Apr 3, 2019 •

edited