From 658c885c7c017a0910ecbd35eb9259cce740a4fc Mon Sep 17 00:00:00 2001
From: Steven Orvell <sorvell@google.com>
Date: Tue, 16 Jul 2019 18:35:32 -0700
Subject: [PATCH] Add tests for connected/disconnected while disabled

---
 lib/legacy/class.js                 |  10 ++-
 lib/mixins/disable-upgrade-mixin.js |  16 +++--
 test/unit/disable-upgrade.html      | 103 ++++++++++++++++++++++++++--
 3 files changed, 116 insertions(+), 13 deletions(-)

diff --git a/lib/legacy/class.js b/lib/legacy/class.js
index 66288c4765..3c537c9b80 100644
--- a/lib/legacy/class.js
+++ b/lib/legacy/class.js
@@ -10,6 +10,7 @@ subject to an additional IP rights grant found at http://polymer.github.io/PATEN
 
 import { LegacyElementMixin } from './legacy-element-mixin.js';
 import { legacyOptimizations } from '../utils/settings.js';
+import { wrap } from '../utils/wrap.js';
 
 const lifecycleProps = {
   attached: true,
@@ -250,6 +251,8 @@ function GenerateClassFromInfo(info, Base, behaviors) {
     }
 
     // Prevent element from enabling properties when it's upgrade disabled.
+    // Normally overriding connectedCallback would be enough, but dom-* elements
+    // enable elements early.
     /** @override */
     _enableProperties() {
       if (!this.__isUpgradeDisabled) {
@@ -269,10 +272,10 @@ function GenerateClassFromInfo(info, Base, behaviors) {
       if (name == DISABLED_ATTR) {
         // When disable-upgrade is removed, intialize properties and
         // provoke connectedCallback if the element is already connected.
-        if (!this.__dataEnabled && value == null) {
+        if (this.__isUpgradeDisabled && value == null) {
           super._initializeProperties();
           this.__isUpgradeDisabled = false;
-          if (this.isConnected) {
+          if (wrap(this).isConnected) {
             super.connectedCallback();
           }
         }
@@ -283,6 +286,7 @@ function GenerateClassFromInfo(info, Base, behaviors) {
     }
 
     // Prevent element from connecting when it's upgrade disabled.
+    // This prevents user code in `attached` from being called.
     /** @override */
     connectedCallback() {
       if (!this.__isUpgradeDisabled) {
@@ -291,6 +295,8 @@ function GenerateClassFromInfo(info, Base, behaviors) {
     }
 
     // Prevent element from disconnecting when it's upgrade disabled.
+    // This avoids allowing user code `detached` from being called without a
+    // paired call to `attached`.
     /** @override */
     disconnectedCallback() {
       if (!this.__isUpgradeDisabled) {
diff --git a/lib/mixins/disable-upgrade-mixin.js b/lib/mixins/disable-upgrade-mixin.js
index d77d10507b..8f5fcdfc3c 100644
--- a/lib/mixins/disable-upgrade-mixin.js
+++ b/lib/mixins/disable-upgrade-mixin.js
@@ -10,8 +10,8 @@
  * rights grant found at http://polymer.github.io/PATENTS.txt
  */
 import { ElementMixin } from './element-mixin.js';
-
 import { dedupingMixin } from '../utils/mixin.js';
+import { wrap } from '../utils/wrap.js';
 
 const DISABLED_ATTR = 'disable-upgrade';
 
@@ -83,14 +83,15 @@ export const DisableUpgradeMixin = dedupingMixin((base) => {
     // Prevent element from initializing properties when it's upgrade disabled.
     /** @override */
     _initializeProperties() {
-      if (!this.hasAttribute(DISABLED_ATTR)) {
-        super._initializeProperties();
-      } else {
+      if (this.hasAttribute(DISABLED_ATTR)) {
         this.__isUpgradeDisabled = true;
+      } else {
+        super._initializeProperties();
       }
     }
 
     // Prevent element from enabling properties when it's upgrade disabled.
+    // Normally overriding connectedCallback would be enough, but dom-* elements
     /** @override */
     _enableProperties() {
       if (!this.__isUpgradeDisabled) {
@@ -110,10 +111,10 @@ export const DisableUpgradeMixin = dedupingMixin((base) => {
       if (name == DISABLED_ATTR) {
         // When disable-upgrade is removed, intialize properties and
         // provoke connectedCallback if the element is already connected.
-        if (!this.__dataEnabled && value == null) {
+        if (this.__isUpgradeDisabled && value == null) {
           super._initializeProperties();
           this.__isUpgradeDisabled = false;
-          if (this.isConnected) {
+          if (wrap(this).isConnected) {
             super.connectedCallback();
           }
         }
@@ -124,6 +125,7 @@ export const DisableUpgradeMixin = dedupingMixin((base) => {
     }
 
     // Prevent element from connecting when it's upgrade disabled.
+    // This prevents user code in `attached` from being called.
     /** @override */
     connectedCallback() {
       if (!this.__isUpgradeDisabled) {
@@ -132,6 +134,8 @@ export const DisableUpgradeMixin = dedupingMixin((base) => {
     }
 
     // Prevent element from disconnecting when it's upgrade disabled.
+    // This avoids allowing user code `detached` from being called without a
+    // paired call to `attached`.
     /** @override */
     disconnectedCallback() {
       if (!this.__isUpgradeDisabled) {
diff --git a/test/unit/disable-upgrade.html b/test/unit/disable-upgrade.html
index 47bca562ad..6a941344ae 100644
--- a/test/unit/disable-upgrade.html
+++ b/test/unit/disable-upgrade.html
@@ -53,6 +53,16 @@ <h2 id="element">[[prop]]</h2>
     super.ready();
     this.enabled = true;
   }
+
+  connectedCallback() {
+    this.wasConnected = true;
+    super.connectedCallback();
+  }
+
+  disconnectedCallback() {
+    this.wasDisconnected = true;
+    super.disconnectedCallback();
+  }
 });
 customElements.define(Disabled.is, Disabled);
 </script>
@@ -74,8 +84,6 @@ <h2 id="element">[[prop]]</h2>
       </template>
 
       <script type="module">
-import { DisableUpgradeMixin } from '../../lib/mixins/disable-upgrade-mixin.js';
-import { Class as PolymerClass } from '../../lib/legacy/class.js';
 import { Polymer } from '../../lib/legacy/polymer-fn.js';
 import {setLegacyOptimizations} from '../../lib/utils/settings.js';
 
@@ -96,6 +104,12 @@ <h2 id="element">[[prop]]</h2>
     this.hasCreated = true;
     this.prop = 'enabled!';
   },
+  attached() {
+    this.wasConnected = true;
+  },
+  detached() {
+    this.wasDisconnected = true;
+  },
   ready() {
     this.enabled = true;
   },
@@ -246,6 +260,7 @@ <h2 id="element">[[prop]]</h2>
   <script type="module">
 import '../../lib/mixins/disable-upgrade-mixin.js';
 import {flush} from '../../lib/utils/flush.js';
+import {wrap} from '../../lib/utils/wrap.js';
 suite('disable-upgrade', function() {
   let el;
 
@@ -254,33 +269,61 @@ <h2 id="element">[[prop]]</h2>
     document.body.appendChild(el);
   });
   teardown(function() {
-    document.body.removeChild(el);
+    if (el.parentNode) {
+      document.body.removeChild(el);
+    }
   });
 
   test('elements upgrade as expected with and without `disable-upgrade`', function() {
     assert.ok(el.$.enabledEl.enabled);
     assert.ok(el.$.enabledEl.$.element);
+    assert.ok(el.$.enabledEl.wasConnected);
     assert.equal(el.$.enabledEl.$.element.textContent, 'enabled!');
+    assert.notOk(el.$.disabledEl.wasConnected);
     assert.notOk(el.$.disabledEl.enabled);
     assert.notOk(el.$.disabledEl.$);
     assert.notOk(el.$.disabledBoundEl.enabled);
+    assert.notOk(el.$.disabledBoundEl.wasConnected);
     assert.notOk(el.$.disabledBoundEl.$);
   });
 
   test('elements upgrade when `disable-upgrade` removed', function() {
     assert.notOk(el.$.disabledEl.enabled);
     assert.notOk(el.$.disabledEl.$);
+    assert.notOk(el.$.disabledEl.wasConnected);
     assert.notOk(el.$.disabledBoundEl.enabled);
     assert.notOk(el.$.disabledBoundEl.$);
+    assert.notOk(el.$.disabledBoundEl.wasConnected);
     el.enable();
     assert.ok(el.$.disabledEl.enabled);
     assert.ok(el.$.disabledEl.$.element);
     assert.equal(el.$.disabledEl.$.element.textContent, 'enabled!');
+    assert.ok(el.$.disabledEl.wasConnected);
     assert.ok(el.$.disabledBoundEl.enabled);
     assert.ok(el.$.disabledBoundEl.$.element);
+    assert.ok(el.$.disabledBoundEl.wasConnected);
     assert.equal(el.$.disabledBoundEl.$.element.textContent, 'enabled!');
   });
 
+  test('elements do not connect/disconnect when disabled', function() {
+    el.parentNode.removeChild(el);
+    assert.ok(el.$.enabledEl.wasConnected);
+    assert.ok(el.$.enabledEl.wasDisconnected);
+    assert.notOk(el.$.disabledEl.wasConnected);
+    assert.notOk(el.$.disabledEl.wasDisconnected);
+    assert.notOk(el.$.disabledBoundEl.wasConnected);
+    assert.notOk(el.$.disabledBoundEl.wasDisconnected);
+    document.body.appendChild(el);
+    el.enable();
+    assert.ok(el.$.disabledEl.wasConnected);
+    assert.notOk(el.$.disabledEl.wasDisconnected);
+    assert.ok(el.$.disabledBoundEl.wasConnected);
+    assert.notOk(el.$.disabledBoundEl.wasDisconnected);
+    document.body.appendChild(el);
+    assert.ok(el.$.disabledEl.wasDisconnected);
+    assert.ok(el.$.disabledBoundEl.wasDisconnected);
+  });
+
 
 });
 
@@ -308,6 +351,7 @@ <h2 id="element">[[prop]]</h2>
     assert.ok(el.$.enabledEl.hasCreated);
     assert.ok(el.$.enabledEl.enabled);
     assert.ok(el.$.enabledEl.$.element);
+    assert.ok(el.$.enabledEl.wasConnected);
     assert.equal(el.$.enabledEl.$.element.textContent, 'enabled!');
     el.$.enabledEl.fooHandler = sinon.spy();
     el.$.enabledEl.fire('foo');
@@ -315,12 +359,14 @@ <h2 id="element">[[prop]]</h2>
     assert.notOk(el.$.disabledEl.hasCreated);
     assert.notOk(el.$.disabledEl.enabled);
     assert.notOk(el.$.disabledEl.$);
+    assert.notOk(el.$.disabledEl.wasConnected);
     el.$.disabledEl.fooHandler = sinon.spy();
     el.$.disabledEl.fire('foo');
     assert.equal(el.$.disabledEl.fooHandler.callCount, 0);
     assert.notOk(el.$.disabledBoundEl.hasCreated);
     assert.notOk(el.$.disabledBoundEl.enabled);
     assert.notOk(el.$.disabledBoundEl.$);
+    assert.notOk(el.$.disabledBoundEl.wasConnected);
     el.$.disabledBoundEl.fooHandler = sinon.spy();
     el.$.disabledBoundEl.fire('foo');
     assert.equal(el.$.disabledBoundEl.fooHandler.callCount, 0);
@@ -331,6 +377,7 @@ <h2 id="element">[[prop]]</h2>
     assert.ok(el.$.disabledEl.hasCreated);
     assert.ok(el.$.disabledEl.enabled);
     assert.ok(el.$.disabledEl.$.element);
+    assert.ok(el.$.disabledEl.wasConnected);
     assert.equal(el.$.disabledEl.$.element.textContent, 'enabled!');
     el.$.disabledEl.fooHandler = sinon.spy();
     el.$.disabledEl.fire('foo');
@@ -338,12 +385,32 @@ <h2 id="element">[[prop]]</h2>
     assert.ok(el.$.disabledBoundEl.hasCreated);
     assert.ok(el.$.disabledBoundEl.enabled);
     assert.ok(el.$.disabledBoundEl.$.element);
+    assert.ok(el.$.disabledBoundEl.wasConnected);
     assert.equal(el.$.disabledBoundEl.$.element.textContent, 'enabled!');
     el.$.disabledBoundEl.fooHandler = sinon.spy();
     el.$.disabledBoundEl.fire('foo');
     assert.equal(el.$.disabledBoundEl.fooHandler.callCount, 1);
   });
 
+  test('elements do not connect/disconnect when disabled', function() {
+    el.parentNode.removeChild(el);
+    assert.ok(el.$.enabledEl.wasConnected);
+    assert.ok(el.$.enabledEl.wasDisconnected);
+    assert.notOk(el.$.disabledEl.wasConnected);
+    assert.notOk(el.$.disabledEl.wasDisconnected);
+    assert.notOk(el.$.disabledBoundEl.wasConnected);
+    assert.notOk(el.$.disabledBoundEl.wasDisconnected);
+    document.body.appendChild(el);
+    el.enable();
+    assert.ok(el.$.disabledEl.wasConnected);
+    assert.notOk(el.$.disabledEl.wasDisconnected);
+    assert.ok(el.$.disabledBoundEl.wasConnected);
+    assert.notOk(el.$.disabledBoundEl.wasDisconnected);
+    document.body.appendChild(el);
+    assert.ok(el.$.disabledEl.wasDisconnected);
+    assert.ok(el.$.disabledBoundEl.wasDisconnected);
+  });
+
 
 });
 
@@ -361,66 +428,87 @@ <h2 id="element">[[prop]]</h2>
   test('elements upgrade as expected with and without `disable-upgrade`', function() {
     assert.ok(el.$.enabledEl1.enabled);
     assert.ok(el.$.enabledEl1.$.element);
+    assert.ok(el.$.enabledEl1.wasConnected);
     assert.equal(el.$.enabledEl1.$.element.textContent, 'enabled!');
     assert.notOk(el.$.disabledEl1.enabled);
     assert.notOk(el.$.disabledEl1.$);
+    assert.notOk(el.$.disabledEl1.wasConnected);
     assert.notOk(el.$.disabledBoundEl1.enabled);
     assert.notOk(el.$.disabledBoundEl1.$);
+    assert.notOk(el.$.disabledBoundEl1.wasConnected);
 
     assert.ok(el.$.enabledEl2.enabled);
     assert.ok(el.$.enabledEl2.$.element);
+    assert.ok(el.$.enabledEl2.wasConnected);
     assert.equal(el.$.enabledEl2.$.element.textContent, 'enabled!');
     assert.notOk(el.$.disabledEl2.enabled);
     assert.notOk(el.$.disabledEl2.$);
+    assert.notOk(el.$.disabledEl2.wasConnected);
     assert.notOk(el.$.disabledBoundEl2.enabled);
     assert.notOk(el.$.disabledBoundEl2.$);
+    assert.notOk(el.$.disabledBoundEl2.wasConnected);
 
     assert.ok(el.$.enabledEl3.enabled);
     assert.ok(el.$.enabledEl3.$.element);
+    assert.ok(el.$.enabledEl3.wasConnected);
     assert.equal(el.$.enabledEl3.$.element.textContent, 'enabled!');
     assert.notOk(el.$.disabledEl3.enabled);
     assert.notOk(el.$.disabledEl3.$);
+    assert.notOk(el.$.disabledEl3.wasConnected);
     assert.notOk(el.$.disabledBoundEl3.enabled);
     assert.notOk(el.$.disabledBoundEl3.$);
+    assert.notOk(el.$.disabledBoundEl3.wasConnected);
   });
 
   test('elements upgrade when `disable-upgrade` removed', function() {
     assert.notOk(el.$.disabledEl1.enabled);
     assert.notOk(el.$.disabledEl1.$);
+    assert.notOk(el.$.disabledEl1.wasConnected);
     assert.notOk(el.$.disabledBoundEl1.enabled);
     assert.notOk(el.$.disabledBoundEl1.$);
+    assert.notOk(el.$.disabledBoundEl1.wasConnected);
 
     assert.notOk(el.$.disabledEl2.enabled);
     assert.notOk(el.$.disabledEl2.$);
+    assert.notOk(el.$.disabledEl2.wasConnected);
     assert.notOk(el.$.disabledBoundEl2.enabled);
     assert.notOk(el.$.disabledBoundEl2.$);
+    assert.notOk(el.$.disabledBoundEl2.wasConnected);
 
     assert.notOk(el.$.disabledEl3.enabled);
     assert.notOk(el.$.disabledEl3.$);
+    assert.notOk(el.$.disabledEl3.wasConnected);
     assert.notOk(el.$.disabledBoundEl3.enabled);
     assert.notOk(el.$.disabledBoundEl3.$);
+    assert.notOk(el.$.disabledBoundEl3.wasConnected);
 
     el.enable();
 
     assert.ok(el.$.disabledEl1.enabled);
     assert.ok(el.$.disabledEl1.$.element);
+    assert.ok(el.$.disabledEl1.wasConnected);
     assert.equal(el.$.disabledEl1.$.element.textContent, 'enabled!');
     assert.ok(el.$.disabledBoundEl1.enabled);
     assert.ok(el.$.disabledBoundEl1.$.element);
+    assert.ok(el.$.disabledBoundEl1.wasConnected);
     assert.equal(el.$.disabledBoundEl1.$.element.textContent, 'enabled!');
 
     assert.ok(el.$.disabledEl2.enabled);
     assert.ok(el.$.disabledEl2.$.element);
+    assert.ok(el.$.disabledEl2.wasConnected);
     assert.equal(el.$.disabledEl2.$.element.textContent, 'enabled!');
     assert.ok(el.$.disabledBoundEl2.enabled);
     assert.ok(el.$.disabledBoundEl2.$.element);
+    assert.ok(el.$.disabledBoundEl2.wasConnected);
     assert.equal(el.$.disabledBoundEl2.$.element.textContent, 'enabled!');
 
     assert.ok(el.$.disabledEl3.enabled);
     assert.ok(el.$.disabledEl3.$.element);
+    assert.ok(el.$.disabledEl3.wasConnected);
     assert.equal(el.$.disabledEl3.$.element.textContent, 'enabled!');
     assert.ok(el.$.disabledBoundEl3.enabled);
     assert.ok(el.$.disabledBoundEl3.$.element);
+    assert.ok(el.$.disabledBoundEl3.wasConnected);
     assert.equal(el.$.disabledBoundEl3.$.element.textContent, 'enabled!');
   });
 });
@@ -437,18 +525,21 @@ <h2 id="element">[[prop]]</h2>
     document.body.removeChild(el);
   });
 
-  test('elements upgrade as expected with and without `disable-upgrade` and upgrad when disagled-upgrade is removed', function() {
-    const els = Array.from(el.shadowRoot.querySelectorAll('my-element-legacy'));
+  test('elements upgrade as expected with and without `disable-upgrade` and upgrad when disable-upgrade is removed', function() {
+    const els = Array.from(wrap(el).shadowRoot.querySelectorAll('my-element-legacy'));
     assert.equal(els.length, 4);
 
     const checkDisabledEl = (e) => {
       assert.ok(e.$.enabledEl.enabled);
       assert.ok(e.$.enabledEl.$.element);
+      assert.ok(e.$.enabledEl.wasConnected);
       assert.equal(e.$.enabledEl.$.element.textContent, 'enabled!');
       assert.notOk(e.$.disabledEl.enabled);
       assert.notOk(e.$.disabledEl.$);
+      assert.notOk(e.$.disabledEl.wasConnected);
       assert.notOk(e.$.disabledBoundEl.enabled);
       assert.notOk(e.$.disabledBoundEl.$);
+      assert.notOk(e.$.disabledBoundEl.wasConnected);
     };
     // check initial state
     els.forEach(checkDisabledEl);
@@ -459,8 +550,10 @@ <h2 id="element">[[prop]]</h2>
     const checkEnabledEl = (e) => {
       assert.notOk(e.$.disabledEl.enabled);
       assert.notOk(e.$.disabledEl.$);
+      assert.notOk(e.$.disabledEl.wasConnected);
       assert.ok(e.$.disabledBoundEl.enabled);
       assert.ok(e.$.disabledBoundEl.$.element);
+      assert.ok(e.$.disabledBoundEl.wasConnected);
       assert.equal(e.$.disabledBoundEl.$.element.textContent, 'enabled!');
     };
     checkEnabledEl(els[1]);