Experimentally enable layout=container on <amp-list> #27911

caroqliu · 2020-04-21T19:25:50Z

Allows an amp-list to be initialized with layout=container by default for AMP4Email or with the experimental flag amp-list-layout-container for other formats.

PR Deploy Bot Demos:

examples/amp-list.amp.html
examples/amp-list-layout-container.amp.html
- Document should be broken without amp-list-layout-container experiment enabled.

TODO:

Add validator rules/tests
~~Documentation changes regarding the experimental feature~~

Related to #26873 b/152928375 cc @zhangsu

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

amp-owners-bot · 2020-04-24T15:00:04Z

Hey @ampproject/wg-caching, these files were changed:

validator/testdata/amp4email_feature_tests/amp_list.html
validator/testdata/amp4email_feature_tests/amp_list.out

caroqliu · 2020-04-24T15:01:33Z

extensions/amp-list/amp-list.md

@@ -220,6 +220,13 @@ In several cases, we may need the `<amp-list>` to resize on user interaction. Fo
 </amp-list>
 ```

+[filter formats="websites, stories"]
+The following feature is available with the `amp-list-layout-container` [experiment](https://amp.dev/documentation/guides-and-tutorials/learn/experimental/) enabled.


@CrystalOnScript Is there a more appropriate syntactic format to designate an experiment in the documentation? Also as a note, the bit about the feature being experimental only applies to websites and stories.

There's no specific syntax, but it may be helpful to give it an alternative heading for websites and stories - maybe just Experimental feature: dynamic resizing. wdyt?

CrystalOnScript

Just have a quick question and a heading suggestion - thanks for looping me in!

CrystalOnScript · 2020-04-24T20:52:16Z

extensions/amp-list/amp-list.md

@@ -220,6 +220,13 @@ In several cases, we may need the `<amp-list>` to resize on user interaction. Fo
 </amp-list>
 ```

+[filter formats="websites, stories"]
+The following feature is available with the `amp-list-layout-container` [experiment](https://amp.dev/documentation/guides-and-tutorials/learn/experimental/) enabled.


There's no specific syntax, but it may be helpful to give it an alternative heading for websites and stories - maybe just Experimental feature: dynamic resizing. wdyt?

CrystalOnScript · 2020-04-24T20:52:43Z

extensions/amp-list/amp-list.md

+[filter formats="websites, stories"]
+The following feature is available with the `amp-list-layout-container` [experiment](https://amp.dev/documentation/guides-and-tutorials/learn/experimental/) enabled.
+[/filter]<!-- formats="websites, stories" -->
+`<amp-list>` can also be initialized with `layout="CONTAINER"` with two caveats: (1) A list with changing contents must have a determinable initial height from a fixed-height placeholder, and (2) once content changes inside the list, resize will occur _only_ if it does not cause content jumping. This is enforced by locking the height of the list prior to rendering contents and conditionally unlocking it accordingly.


Is there a reason CONTAINER is all caps? Does it have to be? Does it matter if this value if caps or not?

CrystalOnScript · 2020-04-24T20:57:31Z

extensions/amp-list/amp-list.md

+`<amp-list>` can also be initialized with `layout="CONTAINER"` with two caveats: (1) A list with changing contents must have a determinable initial height from a fixed-height placeholder, and (2) once content changes inside the list, resize will occur _only_ if it does not cause content jumping. This is enforced by locking the height of the list prior to rendering contents and conditionally unlocking it accordingly.
+
+When an `<amp-list>` is initialized with `layout="CONTAINER"`, the publisher is opting into the managed resizing behavior described above. This means that other dynamic resizing mechanisms such as a mutation to `[is-layout-container]` and the `changeToLayoutContainer` action will be ineffective.
+


Could you provide a brief explanation on why someone would want amp-list to be initialized with layout="container"?

dreamofabear

Looks good overall. Let's run through the manual test/examples just before merge. :)

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

samouri · 2020-04-27T19:14:16Z

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

@@ -191,6 +222,12 @@ export class AmpList extends AMP.BaseElement {
    );

    this.loadMoreEnabled_ = this.element.hasAttribute('load-more');
+    userAssert(


For my own learning: whats the benefit of having the userAssert that load-more and layout=container are incompatible within buildCallback vs. layoutCallback?

nit: On a similar note, can we assume that this invariant is true in later callbacks/methods? For example, within resetIfNecessary_ we double check that !this.loadMoreEnabled_ && this.enableManagedResizing_, but if we gated enabling that flag based on load-more's absence, then we could remove that check from later points.

In regards to why place the assertion in buildCallback as opposed to layoutCallback, in this case the intention was to place it as close as possible to the load-more attribute being read. If we know it's going to cause issues, we might as well error at the earlier possible opportunity so as not to use more resources.

And to go deeper, the reason the load-more attribute is read in the buildCallback is because this is generally where internal state is set in a component instance, including reading attributes.

One might put it in layoutCallback if a critical piece of the assertion is costly, dealing with rendering or loading resources.

Building an AMP Extension is a really helpful resource we have that describes the usage of these callbacks.

re: your other note, yes that's a reasonable assumption for the current state of the code, but I don't think it hurts to be explicit that these are treated as exclusive features, especially if say, in the future, someone else decides to set the internal loadMoreEnabled_ variable without knowing the implicit dependencies here.

Thank you for the thorough explanation and links! Its making me realize that I never got around to reading all of our dev-facing docs after skimming them on my first week 😱. I'll read up on it! Something tells me it will make more sense to me now than it did the last time

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

samouri · 2020-04-27T19:32:58Z

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

+                toggleExperiment(win, 'amp-list-layout-container', false);
+              });
+
+              it('should require placeholder', () => {


nit: feels like there has to be a better way than duplicating all the layout=container tests for amp4email and feature flag cases :/

Good point, thanks for catching this! Used describes.repeated() here.

whoa, awesome diff! TIL about describes.repeated, super useful

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

samouri

nice work, its a great feature. had some nits/questions

dreamofabear · 2020-04-28T01:52:44Z

Oh, we should probably remove the documentation changes until it's widely available. I'd consider only dark launching this for email until V2.

caroqliu · 2020-04-28T12:48:42Z

Thanks for the thorough review @CrystalOnScript! Looks like we are not adding documentation changes in this PR, but I'll be sure to come back for your comments when we do.

Changes in discussion no longer present in this PR

samouri · 2020-04-28T17:50:12Z

Just went through the examples on deploy bot and it LGTM

alanorozco · 2020-04-28T19:49:10Z

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

+     * (1) requiring a placeholder to detect initial sizing and
+     * (2) fixing height on content change to prevent content jumping
+     *
+     * TODO: This configuration should eventually allow resizing to


How would the mechanism change here? It would be useful to add an issue describing it.

samouri · 2020-04-28T20:52:39Z

I used Charles Proxy to test B&H on both #26873 (should be broken), and this branch.

	#26873	#27911 (this branch)
description	Could not resize the `<amp-list>` and displays no text after clicking "Reviews"	Works! I'm seeing `<amp-bind>` warnings as well as an analytics error, but I'm also getting those in production.
screenshot		)

caroqliu · 2020-04-28T21:17:18Z

Thanks @samouri for lending a second set of eyes for manual testing on this, and doing it so thoroughly! Also for walking me through how to use the proxy 😄 🎉

* Allow layout=container for email or experiment * Define and use lockHeight/unlockHeight * Don't apply fill content for layout=container * Isolate feature from existing APIs * Return attemptChangeHeight promise * Move assertions to buildCallback (blocking) * Add todos, comments, assertions * Add unit tests * Add checks for existing unit tests * Add example markup * Move TAG in param order * Will comments * Make promise thenable * Will comments * Modify unit tests to include isLayoutSupported * Add validator allowance in email and upate tests * Update documentation on amp-list * Add assertion in `unlockHeightInsideMutate_` * Return correct type * Add layout=container test case * Assert expect(...).not.called; * Use describes.repeated() * Remove documentation changes * Remove documentation in infinite scroll section * Link PR in TODO comment

caroqliu added 10 commits April 20, 2020 16:20

Allow layout=container for email or experiment

7532445

Define and use lockHeight/unlockHeight

a07caf4

Don't apply fill content for layout=container

2f79bf3

Isolate feature from existing APIs

f3b06f0

Return attemptChangeHeight promise

1173c83

Move assertions to buildCallback (blocking)

68638ec

Add todos, comments, assertions

1056d02

Add unit tests

e13702f

Add checks for existing unit tests

181e0d9

Add example markup

7614a6a

caroqliu requested review from alanorozco, samouri and dreamofabear April 21, 2020 19:25

googlebot added the cla: yes label Apr 21, 2020

Move TAG in param order

7454d66

dreamofabear reviewed Apr 22, 2020

View reviewed changes

caroqliu added 6 commits April 22, 2020 15:58

Will comments

0457eb7

Make promise thenable

ad316d2

Will comments

89e8634

Modify unit tests to include isLayoutSupported

87e2514

Add validator allowance in email and upate tests

51d628f

Update documentation on amp-list

ef6dff6

caroqliu commented Apr 24, 2020

View reviewed changes

caroqliu requested a review from CrystalOnScript April 24, 2020 15:01

CrystalOnScript previously requested changes Apr 24, 2020

View reviewed changes

dreamofabear approved these changes Apr 26, 2020

View reviewed changes

extensions/amp-list/0.1/amp-list.js Show resolved Hide resolved

Gregable approved these changes Apr 27, 2020

View reviewed changes

Gregable added WG: caching WG: Caching: Triaged Initial triage from wg-caching complete. Remove label if new input required. labels Apr 27, 2020

samouri reviewed Apr 27, 2020

View reviewed changes

extensions/amp-list/0.1/test/test-amp-list-container.js Outdated Show resolved Hide resolved

samouri reviewed Apr 27, 2020

View reviewed changes

extensions/amp-list/0.1/test/test-amp-list-load-more.js Show resolved Hide resolved

samouri approved these changes Apr 27, 2020

View reviewed changes

caroqliu added 6 commits April 27, 2020 17:35

Merge branch 'master' into amp-list-layout-container

8b90db3

Add assertion in unlockHeightInsideMutate_

0c8c3e8

Return correct type

9e39dc5

Add layout=container test case

aeb1470

Assert expect(...).not.called;

d7f9174

Use describes.repeated()

e02c315

Remove documentation changes

64c6c81

Remove documentation in infinite scroll section

b4fdee9

alanorozco approved these changes Apr 28, 2020

View reviewed changes

Link PR in TODO comment

6575c42

caroqliu merged commit 72fbcb0 into ampproject:master Apr 29, 2020

ampprojectbot added the PR use: In Beta / Experimental label May 5, 2020

ampprojectbot added the PR use: In Stable label May 14, 2020

samouri mentioned this pull request Jun 5, 2020

FR: More flexible height-change requests #11945

Closed

caroqliu mentioned this pull request Jun 19, 2020

Allow amp-list[layout=container] to opt into default loaders #28958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimentally enable layout=container on <amp-list> #27911

Experimentally enable layout=container on <amp-list> #27911

caroqliu commented Apr 21, 2020 •

edited

amp-owners-bot bot commented Apr 24, 2020 •

edited by ampprojectbot

caroqliu Apr 24, 2020

CrystalOnScript Apr 24, 2020

CrystalOnScript left a comment

CrystalOnScript Apr 24, 2020

CrystalOnScript Apr 24, 2020

CrystalOnScript Apr 24, 2020

dreamofabear left a comment

samouri Apr 27, 2020 •

edited

caroqliu Apr 27, 2020

samouri Apr 27, 2020

samouri Apr 27, 2020

caroqliu Apr 27, 2020

samouri Apr 27, 2020

samouri left a comment •

edited

dreamofabear commented Apr 28, 2020

caroqliu commented Apr 28, 2020

samouri commented Apr 28, 2020 •

edited

alanorozco Apr 28, 2020

samouri commented Apr 28, 2020 •

edited

caroqliu commented Apr 28, 2020

		`<amp-list>` can also be initialized with `layout="CONTAINER"` with two caveats: (1) A list with changing contents must have a determinable initial height from a fixed-height placeholder, and (2) once content changes inside the list, resize will occur _only_ if it does not cause content jumping. This is enforced by locking the height of the list prior to rendering contents and conditionally unlocking it accordingly.

		When an `<amp-list>` is initialized with `layout="CONTAINER"`, the publisher is opting into the managed resizing behavior described above. This means that other dynamic resizing mechanisms such as a mutation to `[is-layout-container]` and the `changeToLayoutContainer` action will be ineffective.

Experimentally enable layout=container on <amp-list> #27911

Experimentally enable layout=container on <amp-list> #27911

Conversation

caroqliu commented Apr 21, 2020 • edited

amp-owners-bot bot commented Apr 24, 2020 • edited by ampprojectbot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrystalOnScript 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

dreamofabear left a comment

Choose a reason for hiding this comment

samouri Apr 27, 2020 • 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

samouri left a comment • edited

Choose a reason for hiding this comment

dreamofabear commented Apr 28, 2020

caroqliu commented Apr 28, 2020

samouri commented Apr 28, 2020 • edited

Choose a reason for hiding this comment

samouri commented Apr 28, 2020 • edited

caroqliu commented Apr 28, 2020

caroqliu commented Apr 21, 2020 •

edited

amp-owners-bot bot commented Apr 24, 2020 •

edited by ampprojectbot

samouri Apr 27, 2020 •

edited

samouri left a comment •

edited

samouri commented Apr 28, 2020 •

edited

samouri commented Apr 28, 2020 •

edited