amp-bind: Handle attributes on input elements that sometimes only spe…

…cify initial values (#9584)
ampproject · May 30, 2017 · e42f305 · e42f305
1 parent 853399c
commit e42f305
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 2 deletions.
diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js
@@ -709,10 +709,22 @@ export class Bind {
         break;
 
       default:
+        // Some input elements treat some of their attributes as initial values.
+        // Once the user interacts with these elements, the JS properties
+        // underlying these attributes must be updated for the change to be
+        // visible to the user.
+        const updateElementProperty =
+            element.tagName == 'INPUT' && property in element;
         const oldValue = element.getAttribute(property);
 
         let attributeChanged = false;
         if (typeof newValue === 'boolean') {
+          if (updateElementProperty && element[property] !== newValue) {
+            // Property value *must* be read before the attribute is changed.
+            // Before user interaction, attribute updates affect the property.
+            element[property] = newValue;
+            attributeChanged = true;
+          }
           if (newValue && oldValue !== '') {
             element.setAttribute(property, '');
             attributeChanged = true;
@@ -736,6 +748,9 @@ export class Bind {
           // Rewriting can fail due to e.g. invalid URL.
           if (rewrittenNewValue !== undefined) {
             element.setAttribute(property, rewrittenNewValue);
+            if (updateElementProperty) {
+              element[property] = rewrittenNewValue;
+            }
             attributeChanged = true;
           }
         }

diff --git a/test/fixtures/bind-integrations.html b/test/fixtures/bind-integrations.html
@@ -48,6 +48,8 @@
   <p id="checkboxText" [text]="'Checked: ' + checkboxChange.checked" id="checkboxText">Unbound</p>
   <input type="radio" on="change:AMP.setState({radioChange: event})" id="radio">
   <p id="radioText" [text]="'Checked: ' + radioChange.checked" id="radioText">Unbound</p>
+  <input type="checkbox" [checked]="isChecked" id="checkedBound">
+  <button on="tap:AMP.setState({isChecked: isChecked != null ? !isChecked : false})" id="toggleCheckedButton"></button>
 
   <p>AMP-IMG</p>
   <button on="tap:AMP.setState({imageSrc: 'http://www.google.com/image2'})" id="changeImgSrcButton"></button>

diff --git a/test/integration/test-amp-bind.js b/test/integration/test-amp-bind.js
@@ -23,9 +23,13 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
   let fixture;
   let ampdoc;
   let sandbox;
+  let numSetStates;
+  let numMutated;
 
   beforeEach(() => {
     sandbox = sinon.sandbox.create();
+    numSetStates = 0;
+    numMutated = 0;
   });
 
   afterEach(() => {
@@ -51,13 +55,13 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
     // Bind should be available, but need to wait for actions to resolve
     // service promise for bind and call setState.
     return bindForDoc(ampdoc).then(unusedBind =>
-        fixture.awaitEvent('amp:bind:setState', 1));
+        fixture.awaitEvent('amp:bind:setState', ++numSetStates));
   }
 
   /** @return {!Promise} */
   function waitForAllMutations() {
     return bindForDoc(ampdoc).then(unusedBind =>
-        fixture.awaitEvent('amp:bind:mutated', 1));
+        fixture.awaitEvent('amp:bind:mutated', ++numMutated));
   }
 
   describe('[text] and [class] integration', () => {
@@ -161,6 +165,30 @@ describe.configure().retryOnSaucelabs().run('amp-bind', function() {
       });
     });
 
+    it('should update checkbox checked attr when its binding changes', () => {
+      // Does *NOT* have the `checked` attribute.
+      const checkbox = fixture.doc.getElementById('checkedBound');
+      const button = fixture.doc.getElementById('toggleCheckedButton');
+      // Some attributes on certain input elements, such as `checked` on
+      // checkbox, only specify an initial value. Clicking the checkbox
+      // ensures the element is no longer relying on `value` as
+      // an initial value.
+      checkbox.click();
+      expect(checkbox.hasAttribute('checked')).to.be.false;
+      expect(checkbox.checked).to.be.true;
+      button.click();
+      return waitForBindApplication().then(() => {
+        expect(checkbox.hasAttribute('checked')).to.be.false;
+        expect(checkbox.checked).to.be.false;
+        button.click();
+        return waitForBindApplication();
+      }).then(() => {
+        // When Bind checks the box back to true, it adds the checked attr.
+        expect(checkbox.hasAttribute('checked')).to.be.true;
+        expect(checkbox.checked).to.be.true;
+      });
+    });
+
     it('should update dependent bindings on radio input changes', () => {
       const radioText = fixture.doc.getElementById('radioText');
       const radio = fixture.doc.getElementById('radio');