Single property components have to return the default value when the …

…DOM attribute is empty string (#1631)

src/core/a-entity.js

@@ -57,6 +57,8 @@ var proto = Object.create(ANode.prototype, {
         delete component.justInitialized;
         return;
       }
+      // When a component is removed after calling el.removeAttribute('material')
+      if (!component && newVal === null) { return; }
       this.setEntityAttribute(attr, oldVal, newVal);
     }
   },
@@ -87,15 +89,11 @@ var proto = Object.create(ANode.prototype, {
 
   applyMixin: {
     value: function (attr) {
-      var attrValue;
       if (!attr) {
         this.updateComponents();
         return;
       }
-      attrValue = this.getAttribute(attr);
-      // Make absence of attribute for getAttribute return undefined rather than null
-      attrValue = attrValue === null ? undefined : attrValue;
-      this.updateComponent(attr, attrValue);
+      this.updateComponent(attr, this.getAttribute(attr));
     }
   },
 
@@ -284,7 +282,6 @@ var proto = Object.create(ANode.prototype, {
       var componentId = componentInfo[1];
       var componentName = componentInfo[0];
       var isComponentDefined = checkComponentDefined(this, attrName) || data !== undefined;
-
       // Check if component is registered and whether component should be initialized.
       if (!components[componentName] ||
           (!isComponentDefined && !isDependency) ||
@@ -379,9 +376,6 @@ var proto = Object.create(ANode.prototype, {
       function updateComponent (name) {
         var attrValue = self.getAttribute(name);
         delete elComponents[name];
-        // turn null into undefined because getAttribute
-        // returns null in the absence of an attribute
-        attrValue = attrValue === null ? undefined : attrValue;
         self.updateComponent(name, attrValue);
       }
     }
@@ -398,16 +392,16 @@ var proto = Object.create(ANode.prototype, {
   updateComponent: {
     value: function (attr, attrValue) {
       var component = this.components[attr];
+      var isDefault = attr in this.defaultComponents;
       if (component) {
-        if (attrValue === null) {
+        if (attrValue === null && !isDefault) {
           this.removeComponent(attr);
           return;
         }
         // Component already initialized. Update component.
         component.updateProperties(attrValue);
         return;
       }
-      if (attrValue === null) { return; }
       // Component not yet initialized. Initialize component.
       this.initComponent(attr, attrValue, false);
     }
@@ -608,7 +602,7 @@ var proto = Object.create(ANode.prototype, {
     value: function (attr) {
       // If cached value exists, return partial component data.
       var component = this.components[attr];
-      if (component && component.attrValue !== undefined) { return component.attrValue; }
+      if (component) { return component.attrValue; }
       return HTMLElement.prototype.getAttribute.call(this, attr);
     },
     writable: window.debug

src/core/component.js

@@ -20,18 +20,18 @@ var styleParser = utils.styleParser;
  * by adding, removing, or updating components. Entities do not share instances
  * of components.
  *
+ * @member {object} el - Reference to the entity element.
+ * @member {string} attr - Component name exposed as an HTML attribute.
  * @member {object} data - Component data populated by parsing the
  *         mapped attribute of the component plus applying defaults and mixins.
- * @member {object} el - Reference to the entity element.
- * @member {string} name - Component name exposed as an HTML attribute.
  */
 var Component = module.exports.Component = function (el, attr, id) {
   this.el = el;
   this.id = id;
   this.attrName = this.name + (id ? '__' + id : '');
   this.updateCachedAttrValue(attr);
   if (!el.hasLoaded) { return; }
-  this.updateProperties();
+  this.updateProperties(this.attrValue);
 };
 
 Component.prototype = {
@@ -135,15 +135,8 @@ Component.prototype = {
    */
   updateCachedAttrValue: function (value) {
     var isSinglePropSchema = isSingleProp(this.schema);
-    if (value === '') {
-      this.attrValue = undefined;
-      return;
-    }
-    if (typeof value === 'string') {
-      this.attrValue = this.parseAttrValueForCache(value);
-      return;
-    }
-    this.attrValue = value !== undefined ? extendProperties({}, value, isSinglePropSchema) : this.attrValue;
+    var attrValue = this.parseAttrValueForCache(value);
+    this.attrValue = extendProperties({}, attrValue, isSinglePropSchema);
   },
 
   /**
@@ -314,7 +307,7 @@ module.exports.registerComponent = function (name, definition) {
  * @return {object} The component data
  */
 function buildData (el, name, schema, elData, silent) {
-  var componentDefined = !!elData;
+  var componentDefined = elData !== undefined && elData !== null;
   var data;
   var isSinglePropSchema = isSingleProp(schema);
   var mixinEls = el.mixinEls;
@@ -345,7 +338,7 @@ function buildData (el, name, schema, elData, silent) {
     return parseProperties(data, schema, undefined, silent);
   } else {
      // Parse and coerce using the schema.
-    if (isSinglePropSchema) { return parseProperty(elData !== undefined ? elData : data, schema); }
+    if (isSinglePropSchema) { return parseProperty(data, schema); }
     return parseProperties(data, schema, undefined, silent);
   }
 }

src/core/schema.js

@@ -117,8 +117,6 @@ module.exports.parseProperties = function (propData, schema, getPartialData, sil
  */
 function parseProperty (value, propDefinition) {
   value = (value === undefined || value === null) ? propDefinition.default : value;
-  if (typeof value !== 'string') { return value; }
-  if (typeof value === 'undefined') { return value; }
   return propDefinition.parse(value);
 }
 module.exports.parseProperty = parseProperty;

tests/core/a-entity.test.js

@@ -379,6 +379,18 @@ suite('a-entity', function () {
       assert.shallowDeepEqual(el.getAttribute('material'), {});
     });
 
+    test('returns null for a default component if it is not set', function () {
+      var el = this.el;
+      assert.shallowDeepEqual(el.getAttribute('position'), null);
+    });
+
+    test('returns parsed data if default component is set', function () {
+      var el = this.el;
+      var position = {x: 5, y: 6, z: 6};
+      el.setAttribute('position', position);
+      assert.shallowDeepEqual(el.getAttribute('position'), position);
+    });
+
     test('returns partial component data', function () {
       var componentData;
       var el = this.el;
@@ -403,6 +415,13 @@ suite('a-entity', function () {
       assert.notOk(el.getAttribute('sound'));
       assert.equal(el.getAttribute('sound__1').autoplay, true);
     });
+
+    test('retrieves default value for single property component when ' +
+         'the element attribute is set to empty string', function () {
+      var sceneEl = this.el.sceneEl;
+      sceneEl.setAttribute('debug', '');
+      assert.equal(sceneEl.getAttribute('debug'), true);
+    });
   });
 
   suite('getChildEntities', function () {
@@ -494,6 +513,13 @@ suite('a-entity', function () {
       assert.ok('height' in componentData);
     });
 
+    test('returns default value on a default component not set', function () {
+      var el = this.el;
+      var defaultPosition = {x: 0, y: 0, z: 0};
+      var elPosition = el.getComputedAttribute('position');
+      assert.shallowDeepEqual(elPosition, defaultPosition);
+    });
+
     test('returns full data of a multiple component', function () {
       var componentData;
       var el = this.el;
@@ -542,7 +568,7 @@ suite('a-entity', function () {
       var el = this.el;
       assert.ok('position' in el.components);
       el.removeAttribute('position');
-      assert.notEqual(el.getAttribute('position'), null);
+      assert.equal(el.getAttribute('position'), null);
       assert.ok('position' in el.components);
     });
 
@@ -579,7 +605,7 @@ suite('a-entity', function () {
     test('initializes dependency component and can set attribute', function () {
       var el = this.el;
       el.initComponent('material', undefined, true);
-      assert.equal(el.getAttribute('material'), '');
+      assert.shallowDeepEqual(el.getAttribute('material'), {});
     });
 
     test('initializes dependency component and current attribute honored', function () {

tests/core/component.test.js

@@ -118,6 +118,18 @@ suite('Component', function () {
       data = buildData(el, 'dummy', TestComponent.prototype.schema, 'green', 'green');
       assert.equal(data, 'green');
     });
+
+    test('returns default value for a single-property schema ' +
+         'when the attribute is empty string', function () {
+      var data;
+      var TestComponent = registerComponent('dummy', {
+        schema: { default: 'red' }
+      });
+      var el = document.createElement('a-entity');
+      el.setAttribute('dummy', '');
+      data = buildData(el, 'dummy', TestComponent.prototype.schema, 'red');
+      assert.equal(data, 'red');
+    });
   });
 
   suite('third-party components', function () {

tests/core/schema.test.js

@@ -62,13 +62,13 @@ suite('schema', function () {
     });
 
     test('returns already-parsed value', function () {
-      var schemaPropDef = { type: 'vec3' };
+      var schemaPropDef = processSchema({ type: 'vec3' });
       var parsed = parseProperty({ x: 0, y: 0, z: 0 }, schemaPropDef);
       assert.shallowDeepEqual(parsed, { x: 0, y: 0, z: 0 });
     });
 
     test('allows undefined default', function () {
-      var schemaPropDef = { type: 'vec3', default: undefined };
+      var schemaPropDef = processSchema({ type: 'vec3', default: undefined });
       var parsed = parseProperty(undefined, schemaPropDef);
       assert.shallowDeepEqual(parsed, undefined);
     });
@@ -82,7 +82,6 @@ suite('schema', function () {
         visible: { type: 'boolean' },
         width: { type: 'int', default: 2 }
       });
-
       var parsed = parseProperties({
         position: '1 2 3',
         visible: 'false',