From cd6d5d0185f72cac88b9561067abaf9a0f55174b Mon Sep 17 00:00:00 2001
From: Steven Orvell <sorvell@google.com>
Date: Fri, 26 Jul 2019 10:24:13 -0700
Subject: [PATCH] Fix issue with defaults overriding bound values when
 disable-upgrade is used.

---
 lib/mixins/disable-upgrade-mixin.js | 11 ++++++++++
 lib/mixins/element-mixin.js         | 16 ++++++++++----
 lib/mixins/properties-changed.js    |  8 +++++++
 test/unit/disable-upgrade.html      | 34 ++++++++++++++++++++++++-----
 4 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/lib/mixins/disable-upgrade-mixin.js b/lib/mixins/disable-upgrade-mixin.js
index 8f5fcdfc3c..222a2b6728 100644
--- a/lib/mixins/disable-upgrade-mixin.js
+++ b/lib/mixins/disable-upgrade-mixin.js
@@ -99,6 +99,17 @@ export const DisableUpgradeMixin = dedupingMixin((base) => {
       }
     }
 
+    // If the element starts upgrade-disabled and a property is set for
+    // which an accessor exists, the default should not be applied.
+    // This additional check is needed because defaults are applied via
+    // `_initializeProperties` which is called after initial properties
+    // have been set when the element starts upgrade-disabled.
+    /** @override */
+    _canApplyPropertyDefault(property) {
+      return super._canApplyPropertyDefault(property) &&
+        !(this.__isUpgradeDisabled && this._isPropertyPending(property));
+    }
+
     /**
      * @override
      * @param {string} name Attribute name.
diff --git a/lib/mixins/element-mixin.js b/lib/mixins/element-mixin.js
index 203f6174b0..3978a269d1 100644
--- a/lib/mixins/element-mixin.js
+++ b/lib/mixins/element-mixin.js
@@ -562,10 +562,7 @@ export const ElementMixin = dedupingMixin(base => {
       }
       for (let p in p$) {
         let info = p$[p];
-        // Don't set default value if there is already an own property, which
-        // happens when a `properties` property with default but no effects had
-        // a property set (e.g. bound) by its host before upgrade
-        if (!this.hasOwnProperty(p)) {
+        if (this._canApplyPropertyDefault(p)) {
           let value = typeof info.value == 'function' ?
             info.value.call(this) :
             info.value;
@@ -580,6 +577,17 @@ export const ElementMixin = dedupingMixin(base => {
       }
     }
 
+    /**
+     * @param {string} property
+     * @returns {boolean} Returns true if the property default can be applied.
+     */
+    // A property default can be overridden when a `properties` property with
+    // default but no effects had a property set (e.g. bound) by its host
+    // before upgrade.
+    _canApplyPropertyDefault(property) {
+      return !this.hasOwnProperty(property);
+    }
+
     /**
      * Gather style text for a style element in the template.
      *
diff --git a/lib/mixins/properties-changed.js b/lib/mixins/properties-changed.js
index ff49a5746b..ec9eff9be8 100644
--- a/lib/mixins/properties-changed.js
+++ b/lib/mixins/properties-changed.js
@@ -297,6 +297,14 @@ export const PropertiesChanged = dedupingMixin(
     }
     /* eslint-enable */
 
+    /**
+     * @param {string} property
+     * @return {boolean} Returns true if the property is pending.
+     */
+    _isPropertyPending(property) {
+      return !!(this.__dataPending && this.__dataPending.hasOwnProperty(property));
+    }
+
     /**
      * Marks the properties as invalid, and enqueues an async
      * `_propertiesChanged` callback.
diff --git a/test/unit/disable-upgrade.html b/test/unit/disable-upgrade.html
index 6a941344ae..ff705f7e9f 100644
--- a/test/unit/disable-upgrade.html
+++ b/test/unit/disable-upgrade.html
@@ -30,6 +30,8 @@
         }
       </style>
       <h2 id="element">[[prop]]</h2>
+      <!-- force an accessor for the default property -->
+      default: [[default]]
     </template>
 
     <script type="module">
@@ -41,6 +43,10 @@ <h2 id="element">[[prop]]</h2>
     return {
       prop: {
         type: String
+      },
+      default: {
+        type: String,
+        value: 'default'
       }
     };
   }
@@ -81,6 +87,8 @@ <h2 id="element">[[prop]]</h2>
           }
         </style>
         <h2 id="element">[[prop]]</h2>
+        <!-- force an accessor for the default property -->
+        default: [[default]]
       </template>
 
       <script type="module">
@@ -95,6 +103,10 @@ <h2 id="element">[[prop]]</h2>
   properties: {
     prop: {
       type: String
+    },
+    default: {
+      type: String,
+      value: 'default'
     }
   },
   listeners: {
@@ -137,8 +149,8 @@ <h2 id="element">[[prop]]</h2>
 
     <template>
       <x-disabled id="enabledEl">Disabled</x-disabled>
-      <x-disabled id="disabledEl" disable-upgrade>Disabled</x-disabled>
-      <x-disabled id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]">Disabled</x-disabled>
+      <x-disabled id="disabledEl" disable-upgrade default="custom">Disabled</x-disabled>
+      <x-disabled id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]" default="[[default]]">Disabled</x-disabled>
     </template>
 
     <script type="module">
@@ -148,7 +160,8 @@ <h2 id="element">[[prop]]</h2>
   static get is() { return 'my-element'; }
   static get properties() {
     return {
-      upgradeDisabled: {value: true}
+      upgradeDisabled: {value: true},
+      default: {type: String, value: 'custom'}
     };
   }
   enable() {
@@ -165,8 +178,8 @@ <h2 id="element">[[prop]]</h2>
 
     <template>
       <x-disabled-legacy id="enabledEl">Disabled</x-disabled-legacy>
-      <x-disabled-legacy id="disabledEl" disable-upgrade>Disabled</x-disabled-legacy>
-      <x-disabled-legacy id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]">Disabled</x-disabled-legacy>
+      <x-disabled-legacy id="disabledEl" disable-upgrade default="custom">Disabled</x-disabled-legacy>
+      <x-disabled-legacy id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]" default="[[default]]">Disabled</x-disabled-legacy>
       <x-disabled-legacy-reg1 id="disabledRegEl" disable-upgrade>Disabled</x-disabled-legacy-reg1>
       <x-disabled-legacy-reg2 id="disabledRegBoundEl" disable-upgrade$="[[upgradeDisabled]]">Disabled</x-disabled-legacy-reg2>
 
@@ -177,7 +190,8 @@ <h2 id="element">[[prop]]</h2>
 Polymer({
   is: 'my-element-legacy',
   properties: {
-    upgradeDisabled: { type: Boolean, value: true }
+    upgradeDisabled: { type: Boolean, value: true },
+    default: {type: String, value: 'custom'}
   },
   enable() {
     this.$.disabledEl.removeAttribute('disable-upgrade');
@@ -281,8 +295,10 @@ <h2 id="element">[[prop]]</h2>
     assert.equal(el.$.enabledEl.$.element.textContent, 'enabled!');
     assert.notOk(el.$.disabledEl.wasConnected);
     assert.notOk(el.$.disabledEl.enabled);
+    assert.equal(el.$.disabledEl.default, 'custom');
     assert.notOk(el.$.disabledEl.$);
     assert.notOk(el.$.disabledBoundEl.enabled);
+    assert.equal(el.$.disabledBoundEl.default, 'custom');
     assert.notOk(el.$.disabledBoundEl.wasConnected);
     assert.notOk(el.$.disabledBoundEl.$);
   });
@@ -298,8 +314,10 @@ <h2 id="element">[[prop]]</h2>
     assert.ok(el.$.disabledEl.enabled);
     assert.ok(el.$.disabledEl.$.element);
     assert.equal(el.$.disabledEl.$.element.textContent, 'enabled!');
+    assert.equal(el.$.disabledEl.default, 'custom');
     assert.ok(el.$.disabledEl.wasConnected);
     assert.ok(el.$.disabledBoundEl.enabled);
+    assert.equal(el.$.disabledBoundEl.default, 'custom');
     assert.ok(el.$.disabledBoundEl.$.element);
     assert.ok(el.$.disabledBoundEl.wasConnected);
     assert.equal(el.$.disabledBoundEl.$.element.textContent, 'enabled!');
@@ -358,6 +376,7 @@ <h2 id="element">[[prop]]</h2>
     assert.equal(el.$.enabledEl.fooHandler.callCount, 1);
     assert.notOk(el.$.disabledEl.hasCreated);
     assert.notOk(el.$.disabledEl.enabled);
+    assert.equal(el.$.disabledEl.default, 'custom');
     assert.notOk(el.$.disabledEl.$);
     assert.notOk(el.$.disabledEl.wasConnected);
     el.$.disabledEl.fooHandler = sinon.spy();
@@ -365,6 +384,7 @@ <h2 id="element">[[prop]]</h2>
     assert.equal(el.$.disabledEl.fooHandler.callCount, 0);
     assert.notOk(el.$.disabledBoundEl.hasCreated);
     assert.notOk(el.$.disabledBoundEl.enabled);
+    assert.equal(el.$.disabledBoundEl.default, 'custom');
     assert.notOk(el.$.disabledBoundEl.$);
     assert.notOk(el.$.disabledBoundEl.wasConnected);
     el.$.disabledBoundEl.fooHandler = sinon.spy();
@@ -376,6 +396,7 @@ <h2 id="element">[[prop]]</h2>
     el.enable();
     assert.ok(el.$.disabledEl.hasCreated);
     assert.ok(el.$.disabledEl.enabled);
+    assert.equal(el.$.disabledEl.default, 'custom');
     assert.ok(el.$.disabledEl.$.element);
     assert.ok(el.$.disabledEl.wasConnected);
     assert.equal(el.$.disabledEl.$.element.textContent, 'enabled!');
@@ -384,6 +405,7 @@ <h2 id="element">[[prop]]</h2>
     assert.equal(el.$.disabledEl.fooHandler.callCount, 1);
     assert.ok(el.$.disabledBoundEl.hasCreated);
     assert.ok(el.$.disabledBoundEl.enabled);
+    assert.equal(el.$.disabledBoundEl.default, 'custom');
     assert.ok(el.$.disabledBoundEl.$.element);
     assert.ok(el.$.disabledBoundEl.wasConnected);
     assert.equal(el.$.disabledBoundEl.$.element.textContent, 'enabled!');