From 4177d9ce7ee575ed05bc75f69008a3ed89f81312 Mon Sep 17 00:00:00 2001
From: Steven Orvell <sorvell@google.com>
Date: Tue, 30 Jan 2018 15:14:25 -0800
Subject: [PATCH] [element-mixin] Do not create property accessors unless a
 property effect exists

Reverts an unintentional breaking change. Previously (2.3.x) property accessors were created only for properties in the properties object which had explicit property effects. In 2.4.0, property accessors were created for any property name returned in the properties object. This was a breaking change in rare cases (for example in https://github.com/PolymerElements/iron-a11y-keys-behavior).

This change reverts the 2.4.0 behavior. Accessors are now again only added for properties which have explicit effects. In addition, `_addPropetyToAttributeMap` was added to `PropertiesChanged` to record a mapping from the attribute to the property name (needed for deserialization).
---
 lib/mixins/element-mixin.html      |  7 ++-----
 lib/mixins/properties-changed.html | 19 ++++++++++++++++---
 test/unit/polymer.element.html     | 17 -----------------
 3 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/lib/mixins/element-mixin.html b/lib/mixins/element-mixin.html
index 06a886b893..2fed46d2b8 100644
--- a/lib/mixins/element-mixin.html
+++ b/lib/mixins/element-mixin.html
@@ -231,11 +231,8 @@
       if (info.observer) {
         proto._createPropertyObserver(name, info.observer, allProps[info.observer]);
       }
-      // always ensure an accessor is made for properties but don't stomp
-      // on existing values.
-      if (!info.readOnly && !(name in proto)) {
-        proto._createPropertyAccessor(name);
-      }
+      // always create the mapping from attribute back to property for deserialization.
+      proto._addPropertyToAttributeMap(name);
     }
 
     /**
diff --git a/lib/mixins/properties-changed.html b/lib/mixins/properties-changed.html
index baf7383d35..aca31674af 100644
--- a/lib/mixins/properties-changed.html
+++ b/lib/mixins/properties-changed.html
@@ -105,17 +105,30 @@
          * @protected
          */
         _createPropertyAccessor(property, readOnly) {
+          this._addPropertyToAttributeMap(property);
           if (!this.hasOwnProperty('__dataHasAccessor')) {
             this.__dataHasAccessor = Object.assign({}, this.__dataHasAccessor);
           }
+          if (!this.__dataHasAccessor[property]) {
+            this.__dataHasAccessor[property] = true;
+            this._definePropertyAccessor(property, readOnly);
+          }
+        }
+
+        /**
+         * Adds the given `property` to a map matching attribute names
+         * to property names, using `attributeNameForProperty`. This map is
+         * used when deserializing attribute values to properties.
+         *
+         * @param {string} property Name of the property
+         */
+        _addPropertyToAttributeMap(property) {
           if (!this.hasOwnProperty('__dataAttributes')) {
             this.__dataAttributes = Object.assign({}, this.__dataAttributes);
           }
-          if (!this.__dataHasAccessor[property]) {
-            this.__dataHasAccessor[property] = true;
+          if (!this.__dataAttributes[property]) {
             const attr = this.constructor.attributeNameForProperty(property);
             this.__dataAttributes[attr] = property;
-            this._definePropertyAccessor(property, readOnly);
           }
         }
 
diff --git a/test/unit/polymer.element.html b/test/unit/polymer.element.html
index d8f6ccf8c8..3f098da0d9 100644
--- a/test/unit/polymer.element.html
+++ b/test/unit/polymer.element.html
@@ -389,23 +389,6 @@ <h1>Sub template</h1>
       assert.equal(el.computedProp, true);
     });
 
-    test('properties have accessors', function() {
-      assert.isTrue(el.__dataHasAccessor.accessor);
-      el._propertiesChanged = sinon.spy();
-      el.accessor = 'hi';
-      assert.equal(el._propertiesChanged.args[0][0].accessor, 'hi');
-    });
-
-    test('properties with no effects do not stomp existing accessors', function() {
-      assert.notOk(el.__dataHasAccessor.noStomp);
-      el._propertiesChanged = sinon.spy();
-      el.accessor = 'hi';
-      el.noStomp = 'hi';
-      const props = el._propertiesChanged.args[0][0];
-      assert.equal(props.accessor, 'hi');
-      assert.notOk(props.noStomp);
-    });
-
     test('attributes', function() {
       const fixtureEl = fixture('my-element-attr');
       assert.equal(fixtureEl.prop, 'attr');