Handle missing mixins and applying mixin attributes to mappings of primitives (#5483)

Co-authored-by: Noeri Huisman <[email protected]>

Author: mrxz

src/core/a-mixin.js

@@ -8,11 +8,13 @@ var MULTIPLE_COMPONENT_DELIMITER = '__';
 /**
  * @member {object} componentCache - Cache of pre-parsed values. An object where the keys
  *         are component names and the values are already parsed by the component.
+ * @member {object} rawAttributeCache - Cache of the raw attribute values.
  */
 class AMixin extends ANode {
   constructor () {
     super();
     this.componentCache = {};
+    this.rawAttributeCache = {};
     this.isMixin = true;
   }
 
@@ -60,10 +62,11 @@ class AMixin extends ANode {
     // Get component data.
     componentName = utils.split(attr, MULTIPLE_COMPONENT_DELIMITER)[0];
     component = components[componentName];
-    if (!component) { return; }
     if (value === undefined) {
       value = window.HTMLElement.prototype.getAttribute.call(this, attr);
     }
+    this.rawAttributeCache[attr] = value;
+    if (!component) { return; }
     this.componentCache[attr] = component.parseAttrValueForCache(value);
   }
 

src/core/a-node.js

@@ -219,7 +219,7 @@ class ANode extends HTMLElement {
     this.computedMixinStr = '';
     this.mixinEls.length = 0;
     for (i = 0; i < newMixinIds.length; i++) {
-      this.registerMixin(document.getElementById(newMixinIds[i]));
+      this.registerMixin(newMixinIds[i]);
     }
 
     // Update DOM. Keep track of `computedMixinStr` to not recurse back here after
@@ -240,21 +240,25 @@ class ANode extends HTMLElement {
   /**
    * From mixin ID, add mixin element to `mixinEls`.
    *
-   * @param {Element} mixinEl
+   * @param {string} mixinId - ID of the mixin to register.
    */
-  registerMixin (mixinEl) {
+  registerMixin (mixinId) {
     var compositedMixinIds;
     var i;
     var mixin;
+    var mixinEl = document.getElementById(mixinId);
 
-    if (!mixinEl) { return; }
+    if (!mixinEl) {
+      warn('No mixin was found with id `%s`', mixinId);
+      return;
+    }
 
     // Register composited mixins (if mixin has mixins).
     mixin = mixinEl.getAttribute('mixin');
     if (mixin) {
       compositedMixinIds = utils.split(mixin.trim(), /\s+/);
       for (i = 0; i < compositedMixinIds.length; i++) {
-        this.registerMixin(document.getElementById(compositedMixinIds[i]));
+        this.registerMixin(compositedMixinIds[i]);
       }
     }
 
@@ -268,6 +272,11 @@ class ANode extends HTMLElement {
     window.HTMLElement.prototype.setAttribute.call(this, attr, newValue);
   }
 
+  /**
+   * Removes the mixin element from `mixinEls`.
+   *
+   * @param {string} mixinId - ID of the mixin to remove.
+   */
   unregisterMixin (mixinId) {
     var i;
     var mixinEls = this.mixinEls;

src/extras/primitives/primitives.js

@@ -70,7 +70,6 @@ module.exports.registerPrimitive = function registerPrimitive (name, definition)
       var i;
       var mapping;
       var mixins;
-      var path;
       var self = this;
 
       // Gather component data from default components.
@@ -79,12 +78,25 @@ module.exports.registerPrimitive = function registerPrimitive (name, definition)
       // Factor in mixins to overwrite default components.
       mixins = this.getAttribute('mixin');
       if (mixins) {
-        mixins = mixins.trim().split(' ');
+        mixins = utils.split(mixins.trim(), /\s+/);
         mixins.forEach(function applyMixin (mixinId) {
-          var mixinComponents = self.sceneEl.querySelector('#' + mixinId).componentCache;
-          Object.keys(mixinComponents).forEach(function setComponent (name) {
-            data[name] = extend(data[name], mixinComponents[name]);
-          });
+          var mixinEl = document.getElementById(mixinId);
+          if (!mixinEl) { return; }
+          var rawAttributeCache = mixinEl.rawAttributeCache;
+          var mixinComponents = mixinEl.componentCache;
+          for (var name in rawAttributeCache) {
+            // Check if the attribute matches a mapping.
+            mapping = self.mappings[name];
+            if (mapping) {
+              applyMapping(mapping, rawAttributeCache[name], data);
+              return;
+            }
+
+            // Check if the attribute belongs to a component.
+            if (name in mixinComponents) {
+              data[name] = extend(data[name], mixinComponents[name]);
+            }
+          }
         });
       }
 
@@ -93,14 +105,7 @@ module.exports.registerPrimitive = function registerPrimitive (name, definition)
         attr = this.attributes[i];
         mapping = this.mappings[attr.name];
         if (mapping) {
-          path = utils.entity.getComponentPropertyPath(mapping);
-          if (path.constructor === Array) {
-            data[path[0]] = data[path[0]] || {};
-            data[path[0]][path[1]] = attr.value.trim();
-          } else {
-            data[path] = attr.value.trim();
-          }
-          continue;
+          applyMapping(mapping, attr.value, data);
         }
       }
 
@@ -169,6 +174,23 @@ module.exports.registerPrimitive = function registerPrimitive (name, definition)
   return primitiveClass;
 };
 
+/**
+ * Sets the relevant property based on the mapping property path.
+ *
+ * @param {string} mapping - The mapped property path.
+ * @param {string} attrValue - The (raw) attribute value.
+ * @param {object} data - The data object to apply the mapping to.
+ */
+function applyMapping (mapping, attrValue, data) {
+  var path = utils.entity.getComponentPropertyPath(mapping);
+  if (path.constructor === Array) {
+    data[path[0]] = data[path[0]] || {};
+    data[path[0]][path[1]] = attrValue.trim();
+  } else {
+    data[path] = attrValue.trim();
+  }
+}
+
 /**
  * Add component mappings using schema.
  */

tests/extras/primitives/primitives.test.js

@@ -226,6 +226,23 @@ suite('registerPrimitive (using innerHTML)', function () {
     });
   });
 
+  test('applies mappings to mixin attributes', function (done) {
+    AFRAME.registerComponent('test', {
+      schema: {default: 'foo'}
+    });
+    primitiveFactory({
+      defaultComponents: {
+        material: {color: 'blue'}
+      },
+      mappings: {foo: 'material.color'}
+    }, 'mixin="bar"', function postCreation (el) {
+      assert.equal(el.getAttribute('material').color, 'purple');
+      done();
+    }, function preCreation (sceneEl) {
+      helpers.mixinFactory('bar', {foo: 'purple'}, sceneEl);
+    });
+  });
+
   test('handles mapping to a single-property component', function (done) {
     primitiveFactory({
       mappings: {