Merge pull request #340 from webcomponents/311-kschaaf

Ensure attached & detached occur for moves in the same turn. Fixes #311

src/CustomElements/observe.js

@@ -27,38 +27,31 @@ var forDocumentTree = scope.forDocumentTree;
 // manage lifecycle on added node and it's subtree; upgrade the node and
 // entire subtree if necessary and process attached for the node and entire
 // subtree
-function addedNode(node) {
-  return added(node) || addedSubtree(node);
+function addedNode(node, isAttached) {
+  return added(node, isAttached) || addedSubtree(node, isAttached);
 }
 
 // manage lifecycle on added node; upgrade if necessary and process attached
-function added(node) {
-  if (scope.upgrade(node)) {
+function added(node, isAttached) {
+  if (scope.upgrade(node, isAttached)) {
+    // Return true to indicate
     return true;
   }
-  attached(node);
+  if (isAttached) {
+    attached(node);
+  }
 }
 
 // manage lifecycle on added node's subtree only; allows the entire subtree
 // to upgrade if necessary and process attached
-function addedSubtree(node) {
+function addedSubtree(node, isAttached) {
   forSubtree(node, function(e) {
-    if (added(e)) {
+    if (added(e, isAttached)) {
       return true;
     }
   });
 }
 
-function attachedNode(node) {
-  attached(node);
-  // only check subtree if node is actually in document
-  if (inDocument(node)) {
-    forSubtree(node, function(e) {
-      attached(e);
-    });
-  }
-}
-
 // On platforms without MutationObserver, mutations may not be
 // reliable and therefore attached/detached are not reliable.
 // To make these callbacks less likely to fail, we defer all inserts and removes
@@ -102,15 +95,11 @@ function attached(element) {
 // multiple times so we protect against extra processing here.
 function _attached(element) {
   // track element for insertion if it's upgraded and cares about insertion
-  if (element.__upgraded__ &&
-    (element.attachedCallback || element.detachedCallback)) {
-    // bail if the element is already marked as attached and proceed only
-    // if it's actually in the document at this moment.
-    if (!element.__attached && inDocument(element)) {
-      element.__attached = true;
-      if (element.attachedCallback) {
-        element.attachedCallback();
-      }
+  // bail if the element is already marked as attached
+  if (element.__upgraded__ && !element.__attached) {
+    element.__attached = true;
+    if (element.attachedCallback) {
+      element.attachedCallback();
     }
   }
 }
@@ -142,15 +131,11 @@ function detached(element) {
 // multiple times so we protect against extra processing here.
 function _detached(element) {
   // track element for removal if it's upgraded and cares about removal
-  if (element.__upgraded__ &&
-    (element.attachedCallback || element.detachedCallback)) {
-    // bail if the element is already marked as not attached and proceed only
-    // if it's actually *not* in the document at this moment.
-    if (element.__attached && !inDocument(element)) {
-      element.__attached = false;
-      if (element.detachedCallback) {
-        element.detachedCallback();
-      }
+  // bail if the element is already marked as not attached
+  if (element.__upgraded__ && element.__attached) {
+    element.__attached = false;
+    if (element.detachedCallback) {
+      element.detachedCallback();
     }
   }
 }
@@ -195,8 +180,12 @@ function watchShadow(node) {
   (2) In this case, child will get its own mutation record:
 
       node.appendChild(div).appendChild(child);
+
+  We cannot know ahead of time if we need to walk into the node in (1) so we
+  do and see child; however, if it was added via case (2) then it will have its
+  own record and therefore be seen 2x.
 */
-function handler(mutations) {
+function handler(root, mutations) {
   // for logging only
   if (flags.dom) {
     var mx = mutations[0];
@@ -213,13 +202,18 @@ function handler(mutations) {
     console.group('mutations (%d) [%s]', mutations.length, u || '');
   }
   // handle mutations
+  // NOTE: do an `inDocument` check dynamically here. It's possible that `root`
+  // is a document in which case the answer here can never change; however 
+  // `root` may be an element like a shadowRoot that can be added/removed
+  // from the main document.
+  var isAttached = inDocument(root);
   mutations.forEach(function(mx) {
     if (mx.type === 'childList') {
       forEach(mx.addedNodes, function(n) {
         if (!n.localName) {
           return;
         }
-        addedNode(n);
+        addedNode(n, isAttached);
       });
       forEach(mx.removedNodes, function(n) {
         if (!n.localName) {
@@ -250,7 +244,7 @@ function takeRecords(node) {
   }
   var observer = node.__observer;
   if (observer) {
-    handler(observer.takeRecords());
+    handler(node, observer.takeRecords());
     takeMutations();
   }
 }
@@ -265,7 +259,9 @@ function observe(inRoot) {
   }
   // For each ShadowRoot, we create a new MutationObserver, so the root can be
   // garbage collected once all references to the `inRoot` node are gone.
-  var observer = new MutationObserver(handler);
+  // Give the handler access to the root so that an 'in document' check can
+  // be done.
+  var observer = new MutationObserver(handler.bind(this, inRoot));
   observer.observe(inRoot, {childList: true, subtree: true});
   inRoot.__observer = observer;
 }
@@ -274,7 +270,8 @@ function observe(inRoot) {
 function upgradeDocument(doc) {
   doc = window.wrap(doc);
   flags.dom && console.group('upgradeDocument: ', (doc.baseURI).split('/').pop());
-  addedNode(doc);
+  var isMainDocument = (doc === window.wrap(document));
+  addedNode(doc, isMainDocument);
   observe(doc);
   flags.dom && console.groupEnd();
 }
@@ -305,7 +302,7 @@ scope.watchShadow = watchShadow;
 scope.upgradeDocumentTree = upgradeDocumentTree;
 scope.upgradeSubtree = addedSubtree;
 scope.upgradeAll = addedNode;
-scope.attachedNode = attachedNode;
+scope.attached = attached;
 scope.takeRecords = takeRecords;
 
 });

src/CustomElements/upgrade.js

@@ -32,21 +32,21 @@ var flags = scope.flags;
  * @return {Element} The upgraded element.
  */
 // Upgrade a node if it can be upgraded and is not already.
-function upgrade(node) {
+function upgrade(node, isAttached) {
   if (!node.__upgraded__ && (node.nodeType === Node.ELEMENT_NODE)) {
     var is = node.getAttribute('is');
     var definition = scope.getRegisteredDefinition(is || node.localName);
     if (definition) {
       if (is && definition.tag == node.localName) {
-        return upgradeWithDefinition(node, definition);
+        return upgradeWithDefinition(node, definition, isAttached);
       } else if (!is && !definition.extends) {
-        return upgradeWithDefinition(node, definition);
+        return upgradeWithDefinition(node, definition, isAttached);
       }
     }
   }
 }
 
-function upgradeWithDefinition(element, definition) {
+function upgradeWithDefinition(element, definition, isAttached) {
   flags.upgrade && console.group('upgrade:', element.localName);
   // some definitions specify an 'is' attribute
   if (definition.is) {
@@ -59,9 +59,11 @@ function upgradeWithDefinition(element, definition) {
   // lifecycle management
   created(element);
   // attachedCallback fires in tree order, call before recursing
-  scope.attachedNode(element);
+  if (isAttached) {
+    scope.attached(element);
+  }
   // there should never be a shadow root on element at this point
-  scope.upgradeSubtree(element);
+  scope.upgradeSubtree(element, isAttached);
   flags.upgrade && console.groupEnd();
   // OUTPUT
   return element;

tests/CustomElements/js/customElements.js

@@ -46,9 +46,12 @@ suite('customElements', function() {
   });
 
   test('document.registerElement requires name argument to not conflict with a reserved name', function() {
-    assert.throws(function() {
-      document.registerElement('font-face', {prototype: Object.create(HTMLElement.prototype)});
-    }, '', 'Failed to execute \'registerElement\' on \'Document\': Registration failed for type \'font-face\'. The type name is invalid.');
+    // Native Custom Elements no longer throws here so skip this test.
+    if (!CustomElements.useNative) {
+      assert.throws(function() {
+        document.registerElement('font-face', {prototype: Object.create(HTMLElement.prototype)});
+      }, '', 'Failed to execute \'registerElement\' on \'Document\': Registration failed for type \'font-face\'. The type name is invalid.');
+    }
   });
 
   test('document.registerElement requires name argument to be unique', function() {
@@ -457,6 +460,47 @@ suite('customElements', function() {
     assert.deepEqual(['a', 'b', 'c', 'd', 'e'], log);
   });
 
+  test('attached and detached in same turn', function(done) {
+    var log = [];
+    var p = Object.create(HTMLElement.prototype);
+    p.attachedCallback = function() {
+      log.push('attached');
+    };
+    p.detachedCallback = function() {
+      log.push('detached');
+    };
+    document.registerElement('x-ad', {prototype: p});
+    var el = document.createElement('x-ad');
+    work.appendChild(el);
+    work.removeChild(el);
+    setTimeout(function() {
+      assert.deepEqual(['attached', 'detached'], log);
+      done();
+    });
+  });
+
+  test('detached and re-attached in same turn', function(done) {
+    var log = [];
+    var p = Object.create(HTMLElement.prototype);
+    p.attachedCallback = function() {
+      log.push('attached');
+    };
+    p.detachedCallback = function() {
+      log.push('detached');
+    };
+    document.registerElement('x-da', {prototype: p});
+    var el = document.createElement('x-da');
+    work.appendChild(el);
+    CustomElements.takeRecords();
+    log = [];
+    work.removeChild(el);
+    work.appendChild(el);
+    setTimeout(function() {
+      assert.deepEqual(['detached', 'attached'], log);
+      done();
+    });
+  });
+
   test('detachedCallback ordering', function() {
     var log = [];
     var p = Object.create(HTMLElement.prototype);

tests/ShadowCSS/html/style-import-base-tag.html

@@ -11,7 +11,13 @@
 <html>
   <head>
     <title>Imports style loading with base tag</title>
-    <base href="/">
+    <base id="base">
+    <script>
+      // URL's in this page are relative to webcomponentsjs repo root
+      // Set base to root of webcomponentsjs repo, or else '/'
+      var wcjs = location.href.match(/^.*webcomponentsjs\//);
+      base.href = wcjs ? wcjs[0] : '/';
+    </script>
     <script src="tests/tools/chai/chai.js"></script>
     <script src="tests/tools/htmltest.js"></script>
 
@@ -21,11 +27,11 @@
   <body>
     <div id="test1" class="red">red</div>
     <script>
+      var link = document.querySelector('link[rel=stylesheet]');
       document.addEventListener('WebComponentsReady', function() {
         function getComputed(selector) {
           return getComputedStyle(document.querySelector(selector));
         }
-        var link = document.querySelector('link[rel=stylesheet]');
         //Tricky interval, because WebComponentsReady is fired before all link[rel=stylesheet] are processed
         var interval = setInterval(function () {
           if (link.__importParsed) {

tests/ShadowDOM/js/Document.js

@@ -575,7 +575,7 @@ htmlSuite('Document', function() {
   });
 
   test('document.registerElement attachedCallback, detachedCallback',
-      function() {
+      function(done) {
     if (!document.registerElement)
       return;
 
@@ -601,9 +601,16 @@ htmlSuite('Document', function() {
 
     var a = new A;
     document.body.appendChild(a);
-    assert.equal(attachedCalls, 1);
-    document.body.removeChild(a);
-    assert.equal(detachedCalls, 1);
+
+    // CE polyfill attachment is async, need to wait... (or force using takeRecords)
+    setTimeout(function() {
+      assert.equal(attachedCalls, 1);
+      document.body.removeChild(a);
+      setTimeout(function() {
+        assert.equal(detachedCalls, 1);
+        done();
+      });
+    });
   });
 
   test('document.registerElement attributeChangedCallback', function() {
@@ -648,7 +655,7 @@ htmlSuite('Document', function() {
     assert.equal(attributeChangedCalls, 3);
   });
 
-  test('document.registerElement get reference, upgrade, rewrap', function() {
+  test('document.registerElement get reference, upgrade', function() {
     if (!document.registerElement)
       return;
 
@@ -664,8 +671,7 @@ htmlSuite('Document', function() {
     };
 
     A = document.registerElement('x-a6', A);
-    // re-wrap after registration to update wrapper
-    ShadowDOMPolyfill.rewrap(ShadowDOMPolyfill.unwrap(div.firstChild));
+
     assert.isTrue(div.firstChild.isCustom);
   });