Amp-bind: Detect bindings in amp-live-list #7990

kmh287 · 2017-03-06T15:29:08Z

This PR adds support for detecting new bind attributes in amp-live-list items. To do this, bind will observe changes to the children of the required <div items> element under any <amp-live-list> element

Tests for this and an example have also been added.

Fixes #7557

@choumx The example highlights an interesting problem with these dynamic elements. Right now, when bind detects mutations on dynamic elements, it will apply the current scope to the entire page after adding/removing appropriate bindings. If a bind attribute is bound to a variable that hasn't been initialized, it will be set to 'null' at this point. One thing we could do to prevent this is we could prevent applying the current scope when dynamic elements change if setState hasn't otherwise been called. What are your thoughts?

dreamofabear · 2017-03-09T19:08:22Z

it will apply the current scope to the entire page after adding/removing appropriate bindings

If we do evaluate, we should probably only evaluate the bindings within the dynamic element rather than the entire page. It's interesting since the user intent is slightly different between amp-list and amp-form, for example.

Also, is there a "flash of unbound content" as we evaluate the new bindings in the dynamic element?

kmh287 · 2017-03-09T19:33:56Z

I'm concerned about evaluating certain parts of the page since that opens us up to potential issues where parts of the page are out of sync. Even if we did do that, consider a scenario where we have a p tag outside the live list, and an identical tag (with the same bindings) inside an amp-live-list item that is added to the page 30 seconds after page load. If the user doesn't take any action to change the page state, the p tag in the live-list item will evaluate with null, whereas the p tag outside will be with whatever default was set.

If we want to maintain the invariant that the user must take a deliberate action to change the page's state (as in amp-state), then I think we should only evaluate new bindings if the user has already taken one such action.

EDIT: To answer your second question, yes there is.

dreamofabear · 2017-03-10T19:25:29Z

If the user doesn't take any action to change the page state, the p tag in the live-list item will evaluate with null

This shouldn't happen if the expression has a consistent default (we warn about this with #development=1).

If we want to maintain the invariant that the user must take a deliberate action to change the page's state

What I meant by "user intent" in my last post is that an amp-form submission may qualify as a deliberate action while amp-list and amp-live-list do not. So the best course of action may be component-dependent.

To unblock, let's avoid evaluation of new bindings completely for now. Then let's fix the FOUC and then figure out the best scheme for evaluation for each component.

dreamofabear · 2017-03-10T19:25:45Z

/cc @ericlindley-g

kmh287 · 2017-03-10T19:41:13Z

Okay, sounds like a good way forward.

re: templates, I don't think that the user submitting a form before any bindings have evaluated should cause an evaluation for all bindings on the page either.

kmh287 · 2017-03-13T14:02:46Z

@choumx Let me know if you have any further comments.

kmh287 · 2017-03-13T15:17:41Z

Continues #7369

dreamofabear · 2017-03-14T03:16:17Z

extensions/amp-bind/0.1/bind-impl.js

+      elementToObserve = element.parentElement;
+    } else if (tagName === 'AMP-LIVE-LIST') {
+      // All elements in AMP-LIVE-LIST are children of a <div> with an
+      // `items` attribute.


dom.js#childElementByAttr. Found that by looking at the implementation of amp-live-list.js.

I wonder if we should delegate this to the dynamic element itself, similar to mutatedAttributesCallback.

Done. Delegate deciding which element should be observed?

Yea, since that's an implementation detail of the element. Not necessary for this PR but something to think about.

dreamofabear · 2017-03-14T03:17:51Z

test/fixtures/amp-bind-integrations.html

@@ -17,6 +17,7 @@
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <script src="/dist/amp.js"></script>
  <script custom-element="amp-carousel" src="/dist/v0/amp-carousel-0.1.js"></script>
+  <script custom-element="amp-live-list" src="https://cdn.ampproject.org/v0/amp-live-list-0.1.js"></script>


Use the /dist/v0/ URL?

dreamofabear · 2017-03-14T03:18:38Z

test/fixtures/amp-bind-integrations.html

+    <button update id="liveListUpdateButton" on="tap:liveList.update"></button>
+    <div items id="liveListItems">
+      <div id="liveListItem1" data-sort-time="123456789">
+        <p [text]=liveListText>unbound</p>


Does liveListText need to be wrapped in quotes?

Apparently not, but it should be.

dreamofabear · 2017-03-14T03:20:07Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+      const liveListItem1 = fixture.doc.getElementById('liveListItem1');
+      expect(liveListItem1.firstElementChild.textContent).to.equal('unbound');
+
+      const impl = liveList.implementation_;


Move duplicated set up code into a beforeEach method.

There are only two lines of setup. I'm not sure extracting them is worth it.

dreamofabear · 2017-03-14T03:26:52Z

extensions/amp-bind/0.1/test/test-bind-integration.js

@@ -162,6 +162,73 @@ describe.configure().retryOnSaucelabs().run('integration amp-bind', function() {
    });
  });

+  describe('amp-live-list integration', () => {
+    function createFromServer(childAttrs = []) {


I see this is copied from test-amp-live-list.js. Generally not a good idea to dupe code, and besides many parts of it is unused here (updateTime, tombstone).

You're only calling this method once so you can probably remove it and replace it with a single innerHTML = ... line.

dreamofabear · 2017-03-14T03:35:06Z

examples/bind/live-list.amp.html

@@ -0,0 +1,257 @@
+<!doctype html>


Please remove unnecessary pieces of this example page so that just the essential bind/live-list parts remain.

dreamofabear · 2017-03-14T03:35:19Z

build-system/server.js

@@ -413,6 +468,17 @@ app.use('/examples/live-blog(-non-floating-button)?.amp.(min.|max.)?html',
    next();
 });

+app.use('/examples/bind/live-list.amp.(min.|max.)?html',
+  function(req, res, next) {
+    console.log('test');


dreamofabear · 2017-03-14T03:36:08Z

build-system/server.js

+  return values[Math.round(Math.random() * (max - min))]
+}
+
+function flip() {


dreamofabear · 2017-03-14T03:36:27Z

build-system/server.js

@@ -346,16 +346,19 @@ function liveListTombstone(liveList) {
  }
 }

+
+// Value is inclusive of both min and max values.
+function range(min, max) {


Please briefly document this method. Not your fault but the implementation is pretty inscrutable.

Done. There has to be a simpler way to do this, right?

Yea, I'm not sure what the use is creating the values array in the first place.

dreamofabear · 2017-03-14T03:37:05Z

build-system/server.js

+              data-param-text="Hello world"
+              data-param-href="https://example.com/?ref=URL"
+              data-param-app_id="145634995501895"></amp-social-share>
+          <amp-social-share type="twitter"></amp-social-share>


Is amp-social-share necessary here? Where did this markup originally come from?

No, I was trying to make this as close to the non-bind live-list example as possible.

ampprojectbot · 2017-03-14T03:37:12Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to @cramforce @dvoytenko

extensions/amp-bind/0.1/bind-impl.js
extensions/amp-bind/0.1/test/test-bind-integration.js

/to @erwinmombay @cramforce

build-system/server.js

/to @camelburrito @lannka

examples/bind/live-list.amp.html

/to @alanorozco @aghassemi @cvializ @chenshay @honeybadgerdontcare @dvoytenko @ericlindley-g @erwinmombay @Gregable @lannka @mrjoro @powdercloud @newmuis @jridgewell @kmh287 @cramforce @mkhatib @camelburrito @choumx @muxin @zhouyx

test/fixtures/amp-bind-integrations.html

…_live_list_binding

ampprojectbot · 2017-03-15T15:07:58Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce erwinmombay

build-system/server.js

/to camelburrito lannka

examples/bind/live-list.amp.html

/to choumx kmh287

extensions/amp-bind/0.1/bind-impl.js
extensions/amp-bind/0.1/test/test-bind-integration.js

/to aghassemi alanorozco camelburrito chenshay choumx cramforce cvializ dvoytenko ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

test/fixtures/amp-bind-integrations.html

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

kmh287 · 2017-03-16T15:16:42Z

@choumx good to go?

dreamofabear · 2017-03-16T15:20:30Z

build-system/server.js

@@ -346,16 +346,19 @@ function liveListTombstone(liveList) {
  }
 }

+
+// Value is inclusive of both min and max values.
+function range(min, max) {


Yea, I'm not sure what the use is creating the values array in the first place.

dreamofabear · 2017-03-16T15:25:11Z

extensions/amp-bind/0.1/bind-impl.js

@@ -40,6 +41,16 @@ const TAG = 'amp-bind';
 const AMP_CSS_RE = /^(i?-)?amp(html)?-/;

 /**
+ * Tags under which bind should observe mutaitons to detect added/removed
+ * bindings.
+ * @type {!Object<string, boolean>}


dreamofabear · 2017-03-16T15:25:40Z

extensions/amp-bind/0.1/bind-impl.js

+ * bindings.
+ * @type {!Object<string, boolean>}
+ */
+const DYNAMIC_TAGS = {


Wrap in map() to prevent matching on things like __proto__.

dreamofabear · 2017-03-16T15:28:47Z

test/fixtures/amp-bind-integrations.html

@@ -83,6 +84,17 @@
    [controls]="videoControls">
  </amp-video>

+  <p> AMP-LIVE-LIST</p>


Nit: Leading whitespace.

dreamofabear · 2017-03-16T15:30:02Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+      impl.update(update);
+      fixture.doc.getElementById('liveListUpdateButton').click();
+
+      let liveListItem2;


#7990 (comment)

They're not before and after. There should be two live list items in the list by the end of the test.

Sorry I meant "after" the live list update. Maybe you can ID one live list item "pizza" and another "bacon" to go with the theme of the example... up to you though.

dreamofabear · 2017-03-16T15:30:24Z

build-system/server.js

+      <div id="live-blog-item-${now}" data-sort-time="${now}">
+        <div class="article-body">
+          ${body}
+          <p> As you can see, bacon is far superior to <b><span [text]='favoriteFood'>everything!</span></b>!</p>


dreamofabear · 2017-03-16T15:32:30Z

build-system/server.js

+  var numOfParagraphs = range(1, 2);
+  var body = Array.apply(null, Array(numOfParagraphs)).map(x => {
+    return `<p>${bacon(range(50, 90))}</p>`;
+  }).join('\n');


This content generation doesn't seem necessary for your test.

It's necessary for the example to work.

Right-o. 👍

ampprojectbot · 2017-03-16T18:49:24Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce erwinmombay

build-system/server.js

/to camelburrito lannka

examples/bind/live-list.amp.html

/to choumx kmh287

extensions/amp-bind/0.1/bind-impl.js
extensions/amp-bind/0.1/test/test-bind-integration.js

/to alanorozco camelburrito chenshay choumx cramforce cvializ dvoytenko ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

test/fixtures/amp-bind-integrations.html

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

dreamofabear

Good work! Sorry for being a bit strict on the last round of code review. I might've been hangry.

dreamofabear · 2017-03-17T04:13:25Z

build-system/server.js

+  var numOfParagraphs = range(1, 2);
+  var body = Array.apply(null, Array(numOfParagraphs)).map(x => {
+    return `<p>${bacon(range(50, 90))}</p>`;
+  }).join('\n');


Right-o. 👍

dreamofabear · 2017-03-17T04:15:45Z

extensions/amp-bind/0.1/test/test-bind-integration.js

+      impl.update(update);
+      fixture.doc.getElementById('liveListUpdateButton').click();
+
+      let liveListItem2;


Sorry I meant "after" the live list update. Maybe you can ID one live list item "pizza" and another "bacon" to go with the theme of the example... up to you though.

ampprojectbot · 2017-03-17T04:17:09Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce erwinmombay

build-system/server.js

/to camelburrito lannka

examples/bind/live-list.amp.html

/to choumx kmh287

extensions/amp-bind/0.1/bind-impl.js
extensions/amp-bind/0.1/test/test-bind-integration.js

/to alanorozco camelburrito chenshay choumx cvializ ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

test/fixtures/amp-bind-integrations.html

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

kmh287 · 2017-03-17T18:56:28Z

Waiting for testing fixes in #8010

…e_list_binding

cramforce · 2017-03-27T16:20:30Z

extensions/amp-bind/0.1/bind-impl.js

+ */
+const DYNAMIC_TAGS = map({
+  'TEMPLATE': true,
+  'AMP-LIVE-LIST': true,


Why don't we instead have these components emit something like

this.dispatchCustomEvent('dom:changed-subtree', {root: element})

And listen to that from here?

That would not require changing AMP bind as more such components come.

@cramforce

👍, I'm in the middle of implementing a similar alternative strategy here

kmh287 added 2 commits March 3, 2017 15:06

changes to bind-impl

fd6f944

server changes and some cleanup

d138c18

kmh287 changed the title ~~#7557 amp live list binding~~ Amp-bind: Support amp-live-list Mar 6, 2017

kmh287 added the PR: Work in Progress label Mar 6, 2017

kmh287 added 7 commits March 6, 2017 10:40

cleanup types

8f73b08

revert some server changes

ea3a64f

example

f1e08d9

cleanup

576d626

live list bind integration test

c3d5ced

cleanup

46c7d43

merge

a353ed7

kmh287 changed the title ~~Amp-bind: Support amp-live-list~~ Amp-bind: Detect bindings in amp-live-list Mar 7, 2017

kmh287 requested a review from dreamofabear March 7, 2017 22:05

kmh287 removed the PR: Work in Progress label Mar 7, 2017

kmh287 added 2 commits March 8, 2017 15:53

merge

acddaaa

merge

2977977

dreamofabear mentioned this pull request Mar 9, 2017

amp-live-list: support amp-bind #7557

Closed

2 tasks

pr comment

9347979

dreamofabear requested changes Mar 14, 2017

View reviewed changes

kmh287 added 3 commits March 14, 2017 10:38

pr comments

4ea071d

Merge branch 'master' of github.com:ampproject/amphtml into #7557_amp…

ce73789

…_live_list_binding

pr comments

cabde97

kmh287 and others added 4 commits March 14, 2017 11:25

pr comments

aa81d9e

pr comments

64727e2

lint issues

1075ab9

pr comments

d9af9ee

aghassemi requested a review from dreamofabear March 15, 2017 17:09

merge

71ef50e

dreamofabear requested changes Mar 16, 2017

View reviewed changes

pr comments

917ea74

dreamofabear approved these changes Mar 17, 2017

View reviewed changes

kmh287 added 2 commits March 17, 2017 09:39

pr comment

3f6dcfc

merge with master

a4b6a5b

kmh287 added 2 commits March 20, 2017 13:11

Merge branch 'master' of github.com:kmh287/amphtml into #7557_amp_liv…

121b8d9

…e_list_binding

merge master

7b64d0b

kmh287 mentioned this pull request Mar 20, 2017

Weekly Status 2017-03-20 #8253

Closed

kmh287 merged commit d040812 into ampproject:master Mar 20, 2017

jpettitt pushed a commit to jpettitt/amphtml that referenced this pull request Mar 21, 2017

Amp-bind: Detect bindings in amp-live-list (ampproject#7990)

e7c557d

cramforce reviewed Mar 27, 2017

View reviewed changes

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Amp-bind: Detect bindings in amp-live-list (ampproject#7990)

3377b50

Amp-bind: Detect bindings in amp-live-list #7990

Amp-bind: Detect bindings in amp-live-list #7990

Conversation

kmh287 commented Mar 6, 2017 • edited

dreamofabear commented Mar 9, 2017

kmh287 commented Mar 9, 2017 • edited

dreamofabear commented Mar 10, 2017

dreamofabear commented Mar 10, 2017

kmh287 commented Mar 10, 2017

kmh287 commented Mar 13, 2017

kmh287 commented Mar 13, 2017

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

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

ampprojectbot commented Mar 14, 2017

ampprojectbot commented Mar 15, 2017

kmh287 commented Mar 16, 2017

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

ampprojectbot commented Mar 16, 2017

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

ampprojectbot commented Mar 17, 2017

kmh287 commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Mar 6, 2017 •

edited

kmh287 commented Mar 9, 2017 •

edited