Bind service and state component #6449

dreamofabear · 2016-12-02T00:38:03Z

Partial for #6199.

First pass on Bind service and new <amp-bind-state> component
Support for changing src and a few other attributes for builtins (amp-img, amp-video)
Adds AMP.setBindState(key=value) action for now (can be changed later)

TODO:

Bind's runtime validation of evaluated attribute values, etc.
Allow Bind syntax in validator rules (ideally same solution as above)
Performance monitoring (chunking slow DOM scans or digests)
Support for extended components (amp-carousel, etc.)

jridgewell · 2016-12-02T20:02:12Z

src/service/resources-impl.js

-      request.newHeight = newHeight;
-      request.newWidth = newWidth;
+      request.newHeight = newHeight || request.newHeight;
+      request.newWidth = newWidth || request.newWidth;


This avoids two resources#changeSize methods from clobbering each other. Without this change:

changeSize(el, 100, undefined) changeSize(el, undefined, 100)

el won't have its height changed to 100 since the second call's newHeight value of undefined will override the first call.

Perhaps I should move this to a separate PR since it's a somewhat scary change?

Definitely. We also need to consider newHeight = 0, and that the combined might fail because it violates a heights rule but the width only should have succeeded.

Good point, I'll move this to a separate PR.

jridgewell · 2016-12-02T20:03:34Z

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

+
+    const id = this.element.id;
+    if (!id) {
+      user().error(TAG, 'Element must have an id.');


jridgewell · 2016-12-02T20:05:08Z

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

+    }
+
+    if (id && json) {
+      const state = Object.create(null);


map helper.

In this case, map just wraps Object.create(null). Why is adding an import and function call for 1 LOC preferable here?

jridgewell · 2016-12-02T20:09:13Z

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

+  scanForBindings_(body) {
+    const bindings = [];
+    const elements = body.getElementsByTagName('*');
+    for (let i = 0; i < elements.length; i++) {


This is going to be very expensive. Can we chunk it?

Certainly possible. One nice thing of Bind is that we can chunk the DOM scan as well as digests. I'd like to leave performance tuning (including for binding expressions as discussed) to a later stage. Added a TODO here.

jridgewell · 2016-12-02T20:09:37Z

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

+      return null;
+    }
+    const name = attribute.name;
+    if (name.charAt(0) === '[' && name.charAt(name.length - 1) === ']') {


Just use name[0] and name[name.length - 1]

Good suggestion. Done.

jridgewell · 2016-12-02T20:19:08Z

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

+    } else {
+      const attribute = element.getAttribute(property);
+      if (typeof expectedValue === 'boolean') {
+        initialValue = !!attribute;


If they use attr="false", this will never verify. Intended?

Why not?

[attr]="false" will expect attr to be a toggled-off boolean attribute

[attr]="'false'" will expect attr to have the string value "false"

jridgewell · 2016-12-02T20:20:37Z

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

+    if (property === 'text') {
+      element.textContent = newValue;
+    } else if (property === 'class') {
+      element.classList = newValue;


This will have problems setting the class on SVG elements.

Good point; added a TODO. The validator should disallow [class] binding on SVG elements, which should be aligned with runtime binding sanitation.

jridgewell · 2016-12-02T20:33:12Z

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

+   * @private
+   */
+  sanitizeAttribute_(value) {
+    if (typeof value === 'string') {


Once it gets to #setAttribute, the value will be cast to a string. So, {toString() { return 'javascript:alert("")'; } would pass this check but still inject the XSS.

Shouldn't be possible since Bind does not allow arbitrary function declarations (and I'm not sure that code fragment specifically works as an exploit). Angular did have similar exploits though, so good thinking. This is definitely something the security review will cover.

jridgewell · 2016-12-02T20:34:10Z

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

+    this.vsync_ = vsyncFor(ampdoc.win);
+
+    /** @const {!Array<string>} */
+    this.protocolWhitelist_ = ['http', 'https'];


We can make this a prototype-less map.

Removed as I rethink how best to validate attribute values.

jridgewell · 2016-12-02T20:35:10Z

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

+   */
+  sanitizeAttribute_(value) {
+    if (typeof value === 'string') {
+      const split = value.split(':');


This'll have to pass security review, but might be able specialize our protocol search a bit more:

const protocol = `/^([A-Za-z]+([A-Za-z+.-])*):/`.match(value); if (protocol && !this.protocolWhitelist_[protocol[0]]) { // unsafe } // safe

Removed for now as I rethink how runtime validation will work.

dreamofabear · 2016-12-05T21:15:14Z

/to @cramforce @dvoytenko /cc @aghassemi

…comments

dreamofabear

Thanks for the review! Working on tests for bind-impl.js now.

dreamofabear · 2016-12-06T15:39:42Z

builtins/amp-video.js

@@ -41,6 +41,10 @@ export function installVideo(win) {

      /** @private {?Element} */
      this.video_ = null;
+
+      /** @private {!Array<string>} */
+      this.attributesToPropagateOnChange_ = ['poster', 'controls',


dreamofabear · 2016-12-06T15:42:25Z

builtins/amp-img.js

@@ -38,6 +38,20 @@ export class AmpImg extends BaseElement {

    /** @private {?../src/srcset.Srcset} */
    this.srcset_ = null;
+
+    /** @private {!Array<string>} */
+    this.attributesToPropagate_ = ['alt', 'referrerpolicy', 'aria-label',


dreamofabear · 2016-12-06T15:50:41Z

examples/bind.amp.html

+    }
+  </style>
+
+  <amp-bind-state id="myState">


I actually had it as amp-state and AMP.setState originally, but felt it may be too broad. But since you brought it up, I'm happy to remove the "bind" verbiage. Done.

dreamofabear · 2016-12-06T15:59:05Z

examples/everything.amp.html

@@ -109,6 +109,7 @@
  <script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js"></script>
  <script async custom-element="amp-anim" src="https://cdn.ampproject.org/v0/amp-anim-0.1.js"></script>
  <script async custom-element="amp-audio" src="https://cdn.ampproject.org/v0/amp-audio-0.1.js"></script>
+  <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>


Oops, removed.

dreamofabear · 2016-12-06T15:59:22Z

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

+   * @private
+   */
+  getName_() {
+    return 'AmpBindState ' +


dreamofabear · 2016-12-06T15:59:34Z

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

+/** @typedef {(null|boolean|string|number|Array|Object)} */
+let BindExpressionResultDef;
+
+export class Bind {


dreamofabear · 2016-12-06T16:17:16Z

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

+   * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
+   */
+  constructor(ampdoc) {
+    /** @const {!../../../src/service/ampdoc-impl.AmpDoc} */


dreamofabear · 2016-12-06T16:17:23Z

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

+  measure_(state) {
+    for (let i = 0; i < this.bindings_.length; i++) {
+      const binding = this.bindings_[i];
+      state.results[i] = evaluateBindExpr(binding.expression, this.scope_);


Good idea, done.

dreamofabear · 2016-12-06T17:12:35Z

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

+    // TODO(choumx): Support objects for attributes.
+
+    if (property === 'text') {
+      element.textContent = newValue;


Think we ought to find and mutate the child text node instead? That may be more user friendly but I'm a bit concerned about performance implications.

Mutating textContent also doesn't trigger reflow which users will probably want. Any suggestions on how to handle that?

dreamofabear · 2016-12-06T17:22:26Z

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

+    let match = true;
+
+    if (property === 'text') {
+      initialValue = element.textContent;


jridgewell · 2016-12-06T19:18:54Z

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

+    const task = {measure: this.boundMeasure_, mutate: this.boundMutate_};
+    this.vsync_.run(task, {
+      results: [],
+      verifyOnly: opt_verifyOnly,


Nit: Let's !! this.

Done. This is only a problem if called by a class that has type-checking disabled, right?

jridgewell · 2016-12-06T19:19:51Z

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

+     * element is the last result of `this.bindings_[i].expression`.
+     * @type {!Array<BindExpressionResultDef>}
+     */
+    this.previousResults_ = [];


Could this be on the BindingDef?

That is a bit cleaner. Done.

jridgewell · 2016-12-06T19:22:27Z

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

+   * @return {(string|boolean|number|null)}
+   */
+  attributeValueOf_(value) {
+    if (typeof value === 'string' || typeof value === 'boolean'


We can store the value of typeof into a variable.

I actually had it that way originally but couldn't get it to work with Closure's type checker. I'll give it another try.

Also changed to use a hash as a minor optimization.

jridgewell · 2016-12-06T19:31:23Z

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

+    } else {
+      const attribute = element.getAttribute(property);
+      if (typeof expectedValue === 'boolean') {
+        initialValue = !!attribute;


Continuing:

You're casting the value from #getAttribute, which in this case would be:

"false", we expect boolean. But !!"false" === true.

"'false'". This one works fine, because we expect a string.

Ah, good catch. Added a unit test for this. Thanks!

aghassemi · 2016-12-06T17:38:48Z

builtins/amp-img.js

+      this.srcset_ = srcsetFromElement(this.element);
+      this.updateImageSrc_();
+    } else if (this.img_ && this.attributesToPropagate_.indexOf(name) >= 0) {
+      this.propagateAttributes(name, this.img_);


Looking at propagateAttributes's implementation, it will not propagate the attribute if element originally did not have it. This will become a problem when bind supports conditionally adding/removing attributes and not just changing them. (unless we plan to have a different callback for added/removed attributes)

At this point, Bind will have already called element.setAttribute(name, newValue) so this should still work for adding new attributes.

Good point about removing attributes though, added a new optional param to support that.

aghassemi · 2016-12-06T17:41:53Z

builtins/amp-video.js

@@ -41,6 +41,10 @@ export function installVideo(win) {

      /** @private {?Element} */
      this.video_ = null;
+
+      /** @private {!Array<string>} */
+      this.attributesToPropagateOnChange_ = ['poster', 'controls',


skip controls for now, there is more to just propagating it. video-manager also needs to be able to handle videos dynamically losing controls which it does not handle right now.

aghassemi · 2016-12-06T18:12:11Z

examples/bind.amp.html

+  <p [text]="foo">After clicking the button below, this will read 'foo'<p>
+  <p id="foo" [text]="foo + 'bar'">And this will read 'foobar'<p>
+  <p [text]="myState.myStateKey1">This will read 'myStateValue1'<p>
+  <button [disabled]="isButtonDisabled">This button will be disabled</button>


I am a bit worried "boolean" attributes. boolAttr=false is technically invalid in HTML5. ("The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.")

although disable=false works for buttons, it won't work for many of AMP specific attributes since we use hasAttribute to check for them (e.g. will actually loop).

Few options here:
1- We allow boolAttr=<true/false> in bind syntax but bind handles adding/removing it based on the boolean value (goes back to my first comment of how to let the component know about new added/removed attrs)

2- Have special bind syntax for bool attributes (e.g. [disabled]?="boolExpression")

3- Do both.

I personally think (1) is ok despite being against HTML5 recommendation, we just need to have bind handle adding/removing boolean attributes and let the component know ( I suggest a new callback separate from attributeChangedCallback, maybe attributeAddedRemovedCallback(bool added). @dvoytenko @cramforce

Acknowledged per comment below.

aghassemi · 2016-12-06T22:18:06Z

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

+  buildCallback() {
+    const TAG = this.getName_();
+
+    this.element.setAttribute('aria-hidden', 'true');


apply nodisplay layout to it instead? (otherwise it will create a line break as it is a block element)

Can you modify layout_ within the element subclass? I don't see any examples of this. It shouldn't cause a newline since we toggle it off before layout completes.

aghassemi · 2016-12-06T22:18:50Z

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

+
+    this.element.setAttribute('aria-hidden', 'true');
+
+    const id = this.element.id;


nit: const id = user().assert(this.element.id, '%s element must have an id.', TAG);

aghassemi · 2016-12-06T22:24:34Z

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

+  layoutCallback() {
+    // Now that we are rendered, stop rendering the element to reduce
+    // resource consumption.
+    toggle(this.element, false);


Do we need layout callback all all? why not do toggle(this.element, false); in buildCallback remove layoutCallback and set renderOutsideViewport to return true;

I suppose we want amp-state to be available wherever it's located in the document. Done.

aghassemi · 2016-12-06T22:38:25Z

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

+    this.boundMutate_ = this.mutate_.bind(this);
+
+    this.ampdoc.whenBodyAvailable().then(body => {
+      this.bindings_ = this.scanForBindings_(body);


This alone is not enough and can't handle dynamic content (e.g. rendered via amp-mustache templates). Ideally we could use MutationObserver to run this on newly added DOM but aside from not being supported everywhere, I am not sure of its performance. Maybe a good compromise it to have the template service either run this on render of new templates or it triggers an event we can listen to here to scan again (I like the latter for looser coupling)

Agreed. Current approach is to add support for other AMP extensions one-by-one and make adjustments as necessary.

it triggers an event we can listen to here to scan again

SGTM.

aghassemi · 2016-12-06T22:50:28Z

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

+      if (newValue === true) {
+        element.setAttribute(property, '');
+      } else if (newValue === false) {
+        element.removeAttribute(property);


Per #6449 (comment), components need to be notified of this as well. So maybe element.attributeAddedRemoveCallback(bool added)? (you can ignore parts of that comment as I was advocating for what you already have here before I saw this)

I think the current method can handle that with null representing nonexistence and '' representing an existing boolean attribute.

Current method can handle it too as you mentioned but it is a bit of mental work to figure out if attribute was added/removed based on values of oldValue attributeValue. Either way, we are not calling attributeChangedCallback for boolean attributes currently.

Oops, good catch! Fixed and added unit test.

aghassemi · 2016-12-06T22:52:39Z

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

+        if (element.classList.contains('-amp-element')) {
+          const resources = element.getResources();
+          if (property === 'width') {
+            user().assert(typeof attributeValue === 'number',


nit: use isFiniteNumber from types.js

Good tip. Done.

jridgewell · 2016-12-07T19:48:08Z

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

+   * @param {BindExpressionResultDef|undefined} b
+   * @return {boolean}
+   */
+  shallowEquals_(a, b) {


@dvoytenko: Didn't we create a jsonEquals that's just like this?

Couldn't find any methods named jsonEquals. Also for this method we only check the first-level children for performance reasons (Bind also doesn't care about children past first level).

I actually not aware of jsonEquals.

jridgewell · 2016-12-07T19:50:16Z

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

@@ -300,7 +301,8 @@ export class Bind {

    if (property === 'text') {
      initialValue = element.textContent;
-      match = (initialValue.trim() === expectedValue.trim());
+      match = (initialValue.trim() ===
+          Object.prototype.toString.call(expectedValue).trim());


Nit:

expectedValue = String(expectedValue); match = (trim() === trim())

jridgewell · 2016-12-09T19:53:27Z

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

      if (typeof expectedValue === 'boolean') {
-        initialValue = !!attribute;
+        // Boolean attributes return values of either '' or null.
+        match = (expectedValue && initialValue === '')


I think we should allow "true" and "false" as well, else we'd disallow expressions like [attr]="true".

IMO this is probably not a common use case and muddles the API a bit. Also, the expression would have to be [attr]="'true'", which evaluates to a string.

We don't allow toplevel boolean attributes? Never mind.

jridgewell · 2016-12-09T19:53:58Z

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

-    if (typeof value === 'string' || typeof value === 'boolean'
-       || typeof value === 'number') {
+    const type = typeof(value);
+    if (this.attributeValueTypes_[type] !== undefined) {


No need for the !== undefined?

dvoytenko · 2016-12-09T22:43:50Z

src/base-element.js

@@ -802,6 +806,16 @@ export class BaseElement {
  }

  /**
+   * Called when an attribute's value changes.
+   * @param {!string} unusedName


Just {string} - they are non-nullable by default.

dvoytenko · 2016-12-09T22:44:35Z

src/base-element.js

@@ -802,6 +806,16 @@ export class BaseElement {
  }

  /**
+   * Called when an attribute's value changes.
+   * @param {!string} unusedName
+   * @param {?string} unusedOldValue


Please explain in docs the meaning of null values. newValue == null means the attribute was removed?

Good idea, done.

dvoytenko · 2016-12-09T22:49:02Z

src/base-element.js

+   * @param {?string} unusedOldValue
+   * @param {?string} unusedNewValue
+   */
+  attributeChangedCallback(unusedName, unusedOldValue, unusedNewValue) {


So the notion of bulk change is lost in this API. E.g. the system might accumulate N changes for this element and we'll call them uncorrelated one at a time. How do you see an implementation would be able to reduce amount of work? Would we use vsync? E.g.

attributeChangedCallback(...) { switch (name) { case 'src': this.src_ = newValue; break; case 'abc': this.stateAbc_ = newValue; break; } this.getVsync().mutate(() => this.updateUi_); }

Is this how you see it?

Hmm, good point. Yea, internal vsync is probably the most robust (e.g. if services other than Bind mutate element attributes). I can also revisit this later and change it to pass a map of attributes to old/new values if that looks more attractive as Bind support rolls out.

dvoytenko · 2016-12-09T22:49:39Z

src/service/action-impl.js

@@ -36,6 +37,7 @@ const DEFAULT_METHOD_ = 'activate';
 /** @const {!Object<string,!Array<string>>} */
 const ELEMENTS_ACTIONS_MAP_ = {
  'form': ['submit'],
+  'amp': ['setState'],


Let's do all capitals AMP.

dvoytenko · 2016-12-09T22:51:11Z

src/service/action-impl.js

@@ -224,12 +226,22 @@ export class ActionService {
      return;
    }

+    if (action.actionInfo.target === 'AMP') {
+      if (action.actionInfo.method === 'setState') {


We should instead create an AMP object in src and register it with action service as a target. We have ActionService.installActionHandler for this purpose.

The issue is that all action targets are assumed to be Element type, so I would need to substantially refactor action-impl.js to do this. Should I proceed?

Fine to defer. But we do need to do it very soon.

dvoytenko · 2016-12-09T22:52:17Z

builtins/amp-img.js

@@ -41,6 +47,17 @@ export class AmpImg extends BaseElement {
  }

  /** @override */
+  attributeChangedCallback(name, unusedOldValue, unusedNewValue) {


So, does attributeChangedCallback already assumes mutate context? If so, please document this at the API level.

I don't think it must be run in a vsync mutate. Do you see a problem if it isn't?

As long as it's documented well at the API level, it's fine.

dvoytenko · 2016-12-09T22:52:39Z

builtins/amp-img.js

+      this.srcset_ = srcsetFromElement(this.element);
+      this.updateImageSrc_();
+    } else if (this.img_ && ATTRIBUTES_TO_PROPAGATE.indexOf(name) >= 0) {
+      this.propagateAttributes(name, this.img_,


Do we ever NOT need to remove extra attributes?

I wondered the same. I'm not sure that all AMP extensions would not misbehave with this new functionality, so I'm being conservative with this new arg.

dvoytenko · 2016-12-09T22:54:16Z

src/custom-element.js

+     * @param {?string} oldValue
+     * @param {?string} newValue
+     */
+    attributeChangedCallback(name, oldValue, newValue) {


Where is this method called? Or not yet?

It's actually called by the browser (part of Custom Elements V1 API). In this PR, bind-impl.js also calls it.

I think it is a better design to not align ourselves with the custom elements spec and instead call APIs in various lifecycle of the state changes.

We might need attributeChangedCallback, but I think it should only be used to translate those changes into state changes

@cramforce Could you elaborate on what you mean by that? Add specific APIs for each AMP extension like srcChangedCallback to be called by Bind? Or an API that supports bulk changes like Dima suggested?

Also note that the browser only calls attributeChangedCallback for attributes specified in observedAttributes, which is currently not used anywhere in AMP.

By the way: I don't necessarily want to block this PR on the discussion.

I think we need a doc (or a section in your existing design doc) explaining the lifecycle that happens on an attribute change.

I think the data flow is a bit off when we have these callbacks for attribute changes. I was thinking more of bundling all the attributes as state for a component and calling it when it changes with the request to redraw itself.

I see. I'll add a section to the design doc about this.

dvoytenko · 2016-12-09T22:55:14Z

@choumx Looks good. But let's checkin on Monday re:idea of media queries. I'd like to simply make sure we won't run into ourselves here.

cramforce · 2016-12-09T22:52:50Z

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

+    }
+
+    if (typeof a === 'object') {
+      const keysA = a.keys();


Object.keys()

cramforce · 2016-12-09T22:53:35Z

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

+      return true;
+    }
+
+    if (typeof a !== typeof b) {


Early exit for "is actually the same object?"

Good idea, moved the last clause to the top.

cramforce · 2016-12-09T22:58:32Z

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

+   * @param {BindVsyncStateDef} state
+   * @private
+   */
+  measure_(state) {


Could you change the design to

move all processing of bindings into its own file

in a way that that file itself never touches the DOM (and has its own cache for expression results)

and produces a set of instructions with the sparse set of things to apply to the DOM

has an async interface

The goal would be that we can move all of it into a worker without major refactoring of this file.

Moved expression evaluation to new file bind-evaluator.js.

cramforce · 2016-12-09T22:59:28Z

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

+  measure_(state) {
+    for (let i = 0; i < this.bindings_.length; i++) {
+      const binding = this.bindings_[i];
+      state.results[i] = evaluateBindExpr(binding.expression, this.scope_);


One easy speedup would be to cache the result of expressions per run. I think realistic docs will often have the same expression many times.

Done and added unit test.

jridgewell · 2016-12-13T18:09:03Z

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

+      state[id] = json;
+
+      bindForDoc(this.getAmpDoc()).then(bind => {
+        bind.setState(state, true);


Does this mean there can only be one AmpState on the page?

No, setState(state) merges state into existing data via Object#assign rather than replacing it.

dvoytenko · 2016-12-14T04:58:21Z

LGTM from me. Deferring to @cramforce and @jridgewell for the final approval.

cramforce

Lets get this in.

cramforce · 2016-12-14T05:00:19Z

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

+  evaluate(scope) {
+    return new Promise(resolve => {
+      /** @type {!Object<string,*>} */
+      const output = {};


Rename to cache

* initial commit for bind component/service * make default value error more descriptive, only fire in dev mode * rename some methods/vars * don't strict equality check for default verification * hack support for on='tap:setState' * remove null result check for expr eval * use vsync for digest mutation, add class and attr toggle * first pass on amp-bind-state element * add class change example * add amp-img ex, fix dumb sanitize attr bug * move state to separate element per design doc * document methods in bind-impl.js * properly handle boolean attributes * add framework for reacting to attr changes, support size change, fix comments * propagate attrs in amp-img * move bind service to /extensions, skip digest for amp-bind-state * clean up action-impl * fix lint errors * handle amp-bind not installed * fix closure errors in compiled bind expr * fix type annotations * add amp-video support * add amp-bind validator proto * PR comments, more types, TODOs * fix long line * remove local assets in bind example * fix typedef * more type fixes * fix lint error * revert amp-video changes * rename bind-state to state * PR comments * add unit tests * check shallow equality during mutate, more unit tests * PR comments * nit fix * call attr changed callback for bool attrs * more PR comments * lint and style fixes * PR comments, class-ify bind-expr * move expr eval to separate file * fix lint and type errors * PR comments

dreamofabear added the WIP label Dec 2, 2016

jridgewell reviewed Dec 2, 2016

View reviewed changes

dreamofabear force-pushed the bind-component branch from ee5d727 to e655b07 Compare December 5, 2016 17:25

dreamofabear changed the title ~~[WIP] Bind service and state component~~ Bind service and state component Dec 5, 2016

dreamofabear added NEEDS REVIEW and removed WIP labels Dec 5, 2016

dreamofabear assigned dvoytenko, aghassemi and cramforce and unassigned aghassemi Dec 5, 2016

dreamofabear force-pushed the bind-component branch from bfa8f69 to a4c8d61 Compare December 5, 2016 21:17

William Chou added 18 commits December 5, 2016 17:09

initial commit for bind component/service

aa35ab3

make default value error more descriptive, only fire in dev mode

7bc1806

rename some methods/vars

e201862

don't strict equality check for default verification

521790a

hack support for on='tap:setState'

6e3366f

remove null result check for expr eval

350836c

use vsync for digest mutation, add class and attr toggle

41fd24f

first pass on amp-bind-state element

1c3ea61

add class change example

0bb8352

add amp-img ex, fix dumb sanitize attr bug

09e6276

move state to separate element per design doc

530da99

document methods in bind-impl.js

d2efb01

properly handle boolean attributes

7cca3a9

add framework for reacting to attr changes, support size change, fix …

16746d9

…comments

propagate attrs in amp-img

49ade02

move bind service to /extensions, skip digest for amp-bind-state

505615b

clean up action-impl

4397029

fix lint errors

40074ae

PR comments

1b04da1

dreamofabear commented Dec 6, 2016

View reviewed changes

jridgewell reviewed Dec 6, 2016

View reviewed changes

aghassemi reviewed Dec 6, 2016

View reviewed changes

William Chou added 2 commits December 6, 2016 18:42

add unit tests

b247b8e

check shallow equality during mutate, more unit tests

651c241

jridgewell reviewed Dec 7, 2016

View reviewed changes

erwinmombay removed the NEEDS REVIEW label Dec 7, 2016

William Chou added 2 commits December 9, 2016 14:29

PR comments

5c637aa

nit fix

7fe4c19

jridgewell reviewed Dec 9, 2016

View reviewed changes

William Chou added 3 commits December 9, 2016 16:28

call attr changed callback for bool attrs

0cf2c8a

more PR comments

5b3d71d

lint and style fixes

6c98f3a

dvoytenko suggested changes Dec 9, 2016

View reviewed changes

cramforce requested changes Dec 9, 2016

View reviewed changes

William Chou added 3 commits December 12, 2016 13:12

PR comments, class-ify bind-expr

7875beb

move expr eval to separate file

344dc19

fix lint and type errors

6be79b1

jridgewell reviewed Dec 13, 2016

View reviewed changes

dvoytenko approved these changes Dec 14, 2016

View reviewed changes

cramforce approved these changes Dec 14, 2016

View reviewed changes

PR comments

14b1d3d

dreamofabear merged commit d25c72e into ampproject:master Dec 14, 2016

dreamofabear deleted the bind-component branch December 14, 2016 16:02


		this.element.setAttribute('aria-hidden', 'true');

		const id = this.element.id;

Bind service and state component #6449

Bind service and state component #6449

Conversation

dreamofabear commented Dec 2, 2016 • edited Loading

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

Choose a reason for hiding this comment

dreamofabear commented Dec 5, 2016

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

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 Dec 9, 2016 • edited Loading

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 Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear Dec 9, 2016 • edited Loading

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 Dec 9, 2016 • edited Loading

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 commented Dec 2, 2016 •

edited

Loading

dreamofabear Dec 9, 2016 •

edited

Loading

dreamofabear Dec 9, 2016 •

edited

Loading

dreamofabear Dec 9, 2016 •

edited

Loading

dreamofabear Dec 9, 2016 •

edited

Loading