Use amp-elements for creating dynamic placeholders during buildCallback. #2846

mkhatib · 2016-04-07T23:21:26Z

Follow up to #2738 this is another approach we're thinking about how to get prerendering to work. This is a discussion PR to discuss the following approach,

This basically does what we kind of do today but instead of using native elements for placeholders the suggestion from @dvoytenko was to use amp-elements (i.e. amp-img in the examples below). This will allow the resources manager to manage loading these placeholders as well.

Few problems with this approach that I ran into:

amp-youtube hack for falling back to the hqdefault will no longer work unless we expose load promise for the amp-img or fire load event for it.
Initial tests seems to be troublesome with styling the amp-img directly to match what we want - Probably fixable by wrapping the amp-img with a container.

dvoytenko · 2016-04-08T18:26:12Z

extensions/amp-instagram/0.1/amp-instagram.js

+      'left': '8px',
+      'right': '8px',
+    });
+    wrapper.appendChild(image);


Attribute placeholder should be set somewhere here :)

dvoytenko · 2016-04-08T18:29:30Z

So, I definitely think this looks better, especially if we specifically focus on a separate createPlaceholderCallback. I'd even have it return the placeholder element as we discussed before and finish its styling from a common code. This gives us an option to add prerenderCallback in the future.

RE: YouTube
It's ok if we have an exception with them for now - I don't see a problem here. Loading two images is bad in either case. Ideally, let's reach out to them and see if we can improve that.

mkhatib · 2016-04-11T17:56:22Z

Sounds good. I'll do some more refinement here and try to get this to work for most cases. For YouTube should we just use the sddefault (regardless if it'll fail or not) or stick to the sure lower quality hqdefault?

As for createPlaceholderCallback, I just want to make sure this makes sense and it's easy to understand when an element needs to override this, I am worried this implies that people need to override it just to have a placeholder.

But either case we can make this clear in our life-cycle docs that this is optionally gets called depending on whether we are able to detect a placeholder exists already at that time or not.

Let me do some more work on this to get a good visual result out of it and see if we can take it from there.

dvoytenko · 2016-04-11T18:00:55Z

I'll do some more refinement here and try to get this to work for most cases. For YouTube should we just use the sddefault (regardless if it'll fail or not) or stick to the sure lower quality hqdefault?

I'm totally ok if we continue "the old way" with YouTube and much more interested in us reaching out to them to discuss and adjust. I think what we want is very straightforward and reasonable. (and possibly already exists).

As for createPlaceholderCallback, I just want to make sure this makes sense and it's easy to understand when an element needs to override this, I am worried this implies that people need to override it just to have a placeholder.

Let's think of a good name. But I think eventually it'll be the only way for us to reconcile this with "incomplete build" problem.

cramforce · 2016-04-12T00:00:34Z

This is a really elegant solution. I think youtube is enough of a special case that we can keep it untouched for now. Getting this done without a new concept is great.

mkhatib · 2016-04-14T01:10:47Z

Alright, I've updated the code with buildPlaceholderCallback as described in #2830 this also addresses the double placeholder problem mentioned in that bug. Let me know what do you folks think.

mkhatib · 2016-04-14T01:14:41Z

One thing to notice is that I removed the YouTube thumbnails hack and now using hqdefault. The only ideas that I can think of about keeping the hack is as I mentioned before, expose amp-img internals namely img_ and updateImageSrc_, or allow amp-img to propagate the img load event maybe with an API like amp-img#onImageLoad.

cramforce · 2016-04-14T01:17:59Z

Can we just leave youtube unchanged?

mkhatib · 2016-04-14T19:31:13Z

Yeah we can leave the old hack in place and not implement the buildPlaceholderCallback.

mkhatib · 2016-04-14T19:58:21Z

Reverted amp-youtube change to keep the hack.

mkhatib · 2016-04-14T20:01:03Z

@cramforce let me know what do you think about this and if it looks good I can proceed of adding some tests.

cramforce · 2016-04-14T21:58:58Z

css/amp.css

+/**
+ * Allow images inside amp-img to be fit-cover: object.
+ */
+amp-img.-amp-object-fit-cover > img {


Where is this used?

Ah. Inside YouTube - I guess no longer since we reverted back to the hack instead of using amp-img I can drop this.

cramforce · 2016-04-14T22:01:22Z

Looks great

mkhatib · 2016-04-14T23:33:17Z

css/amp.css

 .-amp-element > [placeholder] {
+  display: none;
+}
+.-amp-element > [placeholder]:last-of-type {


Unfortunately this doesn't really mean what I thought it meant. This only checks the tag so this wouldn't work as expected unless the placeholders are of the same tag type.

This breaks things, multiple placeholders will still show if they're of different types because the selector will match them all. I am trying to think of a way around this but so far coming empty. Let me know if you have ideas on how to deal with this.

cc/ @cramforce @erwinmombay @dvoytenko - see for more details #2830

I found a way to do this but for the first match using ~ selector
See: http://stackoverflow.com/a/8539107/646979

However, I doubt this can work somehow for last match. So one way to work this out is for us to adapt the first placeholder instead of the last. The problem with this is that in case the double placeholder problem happens, our CSS would choose the dynamic placeholder instead of the user provided one which doesn't really solve the whole problem.

Ok after tons of experiments it seems it is impossible to do last-match in CSS as we want here. I came up with a different idea of how to approach this. Still in CSS.

Instead of hiding placeholders we just show all placeholders with trbl=0 as it's currently is. The only difference is that we set a background for the placeholders (e.g. #efefef), this will force the last placeholder to show at the top and virtually hide all the other placeholders.

The other change that we'd need to do is with togglePlaceholder which now needs to toggle ALL placeholders instead of just the one. I don't see any problem with this.

I'll update the code to reflect this but if there are any concerns with this please let me know.

cramforce · 2016-04-19T15:14:42Z

src/dom.js

+ * Finds all child elements that satisfies the callback.
+ * @param {!Element} parent
+ * @param {function(!Element):boolean} callback
+ * @return {Array.<Element>}


!Array<!Element>

cramforce · 2016-04-19T15:15:22Z

I think we should wait for a final review by @dvoytenko

dvoytenko · 2016-05-02T20:30:26Z

css/amp.css

@@ -148,6 +148,8 @@ i-amp-sizer {

 .-amp-element > [placeholder] {
  display: block;
+  /** Background is important to hide all other placeholders below it. */
+  background-color: #efefef;


Is this not too big of a change? All existing placeholders will now get the differing background?

We can stick with a white background and maybe make this somewhat less specific so it would allow other selectors to override this correctly.

Dropped BG.

dvoytenko · 2016-05-04T19:39:41Z

extensions/amp-instagram/0.1/amp-instagram.js

-      setStyles(this.placeholderWrapper_, {
-        'display': '',
-      });
+      this.togglePlaceholder(true);


This has to go into the runtime level. Whoever calls unlayoutCallback should also toggle placeholder back on.

Done. Moved inside Resources.unlayout

dvoytenko · 2016-05-04T19:42:02Z

@mkhatib Couple more comments, otherwise looks very good.

mkhatib · 2016-05-04T22:06:32Z

Thanks @dvoytenko moved placeholder toggling to resources. PTAL

dvoytenko · 2016-05-05T01:42:16Z

LGTM! This is a great improvement. But let's merge tomorrow if you don't mind. I'd like to squeeze in the isReadyToBuild improvement into the same release with this fix.

jridgewell · 2016-05-05T18:38:29Z

src/custom-element.js

-    return this.implementation_.unlayoutCallback();
+    const isReLayoutNeeded = this.implementation_.unlayoutCallback();
+    if (isReLayoutNeeded) {
+      this.layoutCount_ = 0;


~~I added this before...~~ In resources-impl.js, never mind.

jridgewell · 2016-05-05T18:51:21Z

How do we get into a situation where there are multiple placeholders?

mkhatib · 2016-05-05T20:55:59Z

@jridgewell this might happen for elements that create dynamic placeholders (youtube and instagram as examples), the reason is that the element would start being built by AMP before all of its tree is ready, so AMP would think there's no placeholder for this element because the placeholder has not actually been parsed by the browser yet.

<amp-youtube>
  <div placeholder></div> <-- if build starts before this was parsed amp-youtube will dynamically create another placeholder and insert it - leaving us with two placeholders. 
</amp-youtube>

jridgewell · 2016-05-05T21:11:11Z

Thanks! This may be impossible to test, but which placeholder is last in that case? We use #appendChild on a childless element, so are new parsed children appended after our created placeholder? (I'm asking because I think we definitely want to prioritize the user declared, not-yet-parsed placeholder over our own).

dvoytenko · 2016-05-05T22:19:50Z

@mkhatib @jridgewell - this would be a very rare problem. We should avoid the whole problem altogether. Let's try to fix #3081 by next canary.

dvoytenko · 2016-05-05T22:20:22Z

@mkhatib @jridgewell also important to note - this is the problem we already have today, so we are not making it worse - just trying to make it better.

mkhatib · 2016-05-05T22:28:36Z

@jridgewell yes I think the user provided one would be appended later that's why we take the last placeholder to be the element placeholder. I tested this with something like this, though I am not entirely sure if this really emulates the same situation.

<element>
  <script>element.appendChild(dynamic-placeholder)</script>
  <user-placeholder></user-placeholder>
</element>

This would create the following DOM:

<element>
  <dynamic-placeholder>
  <user-placeholder>
</element>

mkhatib assigned dvoytenko Apr 7, 2016

mkhatib added the Type: Discussion/Question label Apr 7, 2016

dvoytenko reviewed Apr 8, 2016
View reviewed changes

mkhatib assigned cramforce and unassigned dvoytenko Apr 14, 2016

cramforce reviewed Apr 14, 2016
View reviewed changes

mkhatib reviewed Apr 14, 2016
View reviewed changes

mkhatib changed the title ~~WIP: Use amp-elements for creating dynamic placeholders during buildCallback.~~ Use amp-elements for creating dynamic placeholders during buildCallback. Apr 18, 2016

mkhatib added NEEDS REVIEW and removed Type: Discussion/Question labels Apr 18, 2016

cramforce reviewed Apr 19, 2016
View reviewed changes

cramforce assigned dvoytenko and unassigned cramforce Apr 19, 2016

dvoytenko reviewed May 2, 2016
View reviewed changes

dvoytenko reviewed May 4, 2016
View reviewed changes

dvoytenko added LGTM and removed NEEDS REVIEW labels May 5, 2016

jridgewell reviewed May 5, 2016
View reviewed changes

mkhatib mentioned this pull request May 5, 2016

Reuse dom.childElements in ElementProto.getRealChildNodes #3115

Closed

mkhatib added 9 commits May 5, 2016 15:39

Use amp-elements for creating dynamic placeholders during buildCallback.

fb5982d

Implement buildPlaceholderCallback for dynamic placeholders.

b15a974

Reverting to YouTube hack

62c0819

Hide all placeholders.

ec5bc0c

Add tests cases.

2f36b0b

Increase amp-element count in integration tests.

054c45f

Address comments on PR.

2c643f5

use layout=fill

1e55e3e

move placeholder toggling to resources.

1611d76

mkhatib force-pushed the prerender-during-build branch from 13d2d91 to 1611d76 Compare May 5, 2016 23:32

fix lint and presubmit

672106c

mkhatib merged commit 074343b into ampproject:master May 6, 2016

mkhatib deleted the prerender-during-build branch May 6, 2016 00:19

This was referenced May 13, 2016

Refactor placeholder creation by using createPlaceholderCallback #3212

Closed

Refactor amp-youtube placeholder fallback mechanism #3216

Closed

Use amp-elements for creating dynamic placeholders during buildCallback. #2846

Use amp-elements for creating dynamic placeholders during buildCallback. #2846

Conversation

mkhatib commented Apr 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Apr 8, 2016

mkhatib commented Apr 11, 2016

dvoytenko commented Apr 11, 2016

cramforce commented Apr 12, 2016

mkhatib commented Apr 14, 2016

mkhatib commented Apr 14, 2016

cramforce commented Apr 14, 2016

mkhatib commented Apr 14, 2016

mkhatib commented Apr 14, 2016

mkhatib commented Apr 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Apr 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce Apr 19, 2016 • edited

Choose a reason for hiding this comment

cramforce commented Apr 19, 2016

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

dvoytenko commented May 4, 2016

mkhatib commented May 4, 2016

dvoytenko commented May 5, 2016

jridgewell May 5, 2016 • edited

Choose a reason for hiding this comment

jridgewell commented May 5, 2016

mkhatib commented May 5, 2016

jridgewell commented May 5, 2016

dvoytenko commented May 5, 2016

dvoytenko commented May 5, 2016

mkhatib commented May 5, 2016

cramforce Apr 19, 2016 •

edited

jridgewell May 5, 2016 •

edited