Skip to content

Commit e9c27cc

Browse files
dmarcosdmarcos
authored
Address the parentNode === null error in Firefox due to the use of MutationObserver in the registerElement polyfill. Default lights are first addded to the scene and immidiately removed by the light system when the scene lights initialize. This cause that in the attachedCallback for the default lights the parentNode === null. The solution is postponing the default light creation until the scene has loaded. This makes the light and camera systems consistent (#1792)
1 parent 47e3cbe commit e9c27cc

File tree

3 files changed

+163
-134
lines changed

3 files changed

+163
-134
lines changed

‎src/systems/camera.js‎

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ var DEFAULT_USER_HEIGHT = 1.6;
1212
module.exports.System = registerSystem('camera', {
1313
init: function () {
1414
this.activeCameraEl = null;
15-
this.setupDefaultCamera();
15+
// Wait for all entities to fully load before checking for existence of camera.
16+
// Since entities wait for <a-assets> to load, any cameras attaching to the scene
17+
// will do so asynchronously.
18+
this.sceneEl.addEventListener('loaded', this.setupDefaultCamera.bind(this));
1619
},
1720

1821
/**
@@ -21,33 +24,27 @@ module.exports.System = registerSystem('camera', {
2124
* Default camera offset height is at average eye level (~1.6m).
2225
*/
2326
setupDefaultCamera: function () {
24-
var self = this;
2527
var sceneEl = this.sceneEl;
2628
var defaultCameraEl;
2729

28-
// Wait for all entities to fully load before checking for existence of camera.
29-
// Since entities wait for <a-assets> to load, any cameras attaching to the scene
30-
// will do so asynchronously.
31-
sceneEl.addEventListener('loaded', function checkForCamera () {
32-
// Camera already defined.
33-
if (sceneEl.camera) {
34-
sceneEl.emit('camera-ready', {cameraEl: sceneEl.camera.el});
35-
return;
36-
}
30+
// Camera already defined.
31+
if (sceneEl.camera) {
32+
sceneEl.emit('camera-ready', {cameraEl: sceneEl.camera.el});
33+
return;
34+
}
3735

38-
// Set up default camera.
39-
defaultCameraEl = document.createElement('a-entity');
40-
defaultCameraEl.setAttribute('position', '0 0 0');
41-
defaultCameraEl.setAttribute(DEFAULT_CAMERA_ATTR, '');
42-
defaultCameraEl.setAttribute('camera', {active: true, userHeight: DEFAULT_USER_HEIGHT});
43-
defaultCameraEl.setAttribute('wasd-controls', '');
44-
defaultCameraEl.setAttribute('look-controls', '');
45-
defaultCameraEl.setAttribute(constants.AFRAME_INJECTED, '');
46-
sceneEl.appendChild(defaultCameraEl);
47-
sceneEl.addEventListener('enter-vr', self.removeDefaultOffset);
48-
sceneEl.addEventListener('exit-vr', self.addDefaultOffset);
49-
sceneEl.emit('camera-ready', {cameraEl: defaultCameraEl});
50-
});
36+
// Set up default camera.
37+
defaultCameraEl = document.createElement('a-entity');
38+
defaultCameraEl.setAttribute('position', '0 0 0');
39+
defaultCameraEl.setAttribute(DEFAULT_CAMERA_ATTR, '');
40+
defaultCameraEl.setAttribute('camera', {active: true, userHeight: DEFAULT_USER_HEIGHT});
41+
defaultCameraEl.setAttribute('wasd-controls', '');
42+
defaultCameraEl.setAttribute('look-controls', '');
43+
defaultCameraEl.setAttribute(constants.AFRAME_INJECTED, '');
44+
sceneEl.appendChild(defaultCameraEl);
45+
sceneEl.addEventListener('enter-vr', this.removeDefaultOffset);
46+
sceneEl.addEventListener('exit-vr', this.addDefaultOffset);
47+
sceneEl.emit('camera-ready', {cameraEl: defaultCameraEl});
5148
},
5249

5350
/**

‎src/systems/light.js‎

Lines changed: 74 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -1,62 +1,76 @@
11
var registerSystem = require('../core/system').registerSystem;
22
var constants = require('../constants/');
3-
4-
var DEFAULT_LIGHT_ATTR = 'data-aframe-default-light';
5-
6-
/**
7-
* Light system.
8-
*
9-
* Prescribes default lighting if not specified (one ambient, one directional).
10-
* Removes default lighting from the scene when a new light is added.
11-
*
12-
* @param {bool} defaultLightsEnabled - Whether default lighting is active.
13-
*/
14-
module.exports.System = registerSystem('light', {
15-
init: function () {
16-
this.defaultLightsEnabled = null;
17-
this.setupDefaultLights();
18-
},
19-
20-
/**
21-
* Notify scene that light has been added and to remove the default.
22-
*
23-
* @param {object} el - element holding the light component.
24-
*/
25-
registerLight: function (el) {
26-
var defaultLights;
27-
var sceneEl = this.sceneEl;
28-
29-
if (this.defaultLightsEnabled && !el.hasAttribute(DEFAULT_LIGHT_ATTR)) {
30-
// User added a light, remove default lights through DOM.
31-
defaultLights = document.querySelectorAll('[' + DEFAULT_LIGHT_ATTR + ']');
32-
for (var i = 0; i < defaultLights.length; i++) {
33-
sceneEl.removeChild(defaultLights[i]);
34-
}
35-
this.defaultLightsEnabled = false;
36-
}
37-
},
38-
39-
/**
40-
* Prescibe default lights to the scene.
41-
* Does so by injecting markup such that this state is not invisible.
42-
* These lights are removed if the user adds any lights.
43-
*/
44-
setupDefaultLights: function () {
45-
var sceneEl = this.sceneEl;
46-
var ambientLight = document.createElement('a-entity');
47-
var directionalLight = document.createElement('a-entity');
48-
49-
ambientLight.setAttribute('light', {color: '#BBB', type: 'ambient'});
50-
ambientLight.setAttribute(DEFAULT_LIGHT_ATTR, '');
51-
ambientLight.setAttribute(constants.AFRAME_INJECTED, '');
52-
sceneEl.appendChild(ambientLight);
53-
54-
directionalLight.setAttribute('light', {color: '#FFF', intensity: 0.6});
55-
directionalLight.setAttribute('position', {x: -0.5, y: 1, z: 1});
56-
directionalLight.setAttribute(DEFAULT_LIGHT_ATTR, '');
57-
directionalLight.setAttribute(constants.AFRAME_INJECTED, '');
58-
sceneEl.appendChild(directionalLight);
59-
60-
this.defaultLightsEnabled = true;
61-
}
62-
});
3+
4+
var DEFAULT_LIGHT_ATTR = 'data-aframe-default-light';
5+
6+
/**
7+
* Light system.
8+
*
9+
* Prescribes default lighting if not specified (one ambient, one directional).
10+
* Removes default lighting from the scene when a new light is added.
11+
*
12+
* @param {bool} defaultLights - Whether default lighting are defined.
13+
* @param {bool} userDefinedLights - Whether user lighting is defined.
14+
*/
15+
module.exports.System = registerSystem('light', {
16+
init: function () {
17+
this.defaultLights = false;
18+
this.userDefinedLights = false;
19+
// Wait for all entities to fully load before checking for existence of lights.
20+
// Since entities wait for <a-assets> to load, any lights attaching to the scene
21+
// will do so asynchronously.
22+
this.sceneEl.addEventListener('loaded', this.setupDefaultLights.bind(this));
23+
},
24+
25+
/**
26+
* Notify scene that light has been added and to remove the default.
27+
*
28+
* @param {object} el - element holding the light component.
29+
*/
30+
registerLight: function (el) {
31+
if (!el.hasAttribute(DEFAULT_LIGHT_ATTR)) {
32+
// User added a light, remove default lights through DOM.
33+
this.removeDefaultLights();
34+
this.userDefinedLights = true;
35+
}
36+
},
37+
38+
removeDefaultLights: function () {
39+
var defaultLights;
40+
var sceneEl = this.sceneEl;
41+
42+
if (!this.defaultLights) { return; }
43+
defaultLights = document.querySelectorAll('[' + DEFAULT_LIGHT_ATTR + ']');
44+
for (var i = 0; i < defaultLights.length; i++) {
45+
sceneEl.removeChild(defaultLights[i]);
46+
}
47+
this.defaultLights = false;
48+
},
49+
50+
/**
51+
* Prescibe default lights to the scene.
52+
* Does so by injecting markup such that this state is not invisible.
53+
* These lights are removed if the user adds any lights.
54+
*/
55+
setupDefaultLights: function () {
56+
var sceneEl = this.sceneEl;
57+
var ambientLight;
58+
var directionalLight;
59+
60+
if (this.userDefinedLights || this.defaultLights) { return; }
61+
ambientLight = document.createElement('a-entity');
62+
directionalLight = document.createElement('a-entity');
63+
ambientLight.setAttribute('light', {color: '#BBB', type: 'ambient'});
64+
ambientLight.setAttribute(DEFAULT_LIGHT_ATTR, '');
65+
ambientLight.setAttribute(constants.AFRAME_INJECTED, '');
66+
sceneEl.appendChild(ambientLight);
67+
68+
directionalLight.setAttribute('light', {color: '#FFF', intensity: 0.6});
69+
directionalLight.setAttribute('position', {x: -0.5, y: 1, z: 1});
70+
directionalLight.setAttribute(DEFAULT_LIGHT_ATTR, '');
71+
directionalLight.setAttribute(constants.AFRAME_INJECTED, '');
72+
sceneEl.appendChild(directionalLight);
73+
74+
this.defaultLights = true;
75+
}
76+
});

‎tests/systems/light.test.js‎

Lines changed: 68 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,50 +1,68 @@
1-
/* global assert, process, setup, suite, test */
2-
var constants = require('constants/');
3-
var entityFactory = require('../helpers').entityFactory;
4-
5-
suite('light system', function () {
6-
setup(function (done) {
7-
var el = this.el = entityFactory();
8-
el.addEventListener('loaded', function () {
9-
done();
10-
});
11-
});
12-
13-
test('adds default lights to scene', function () {
14-
var el = this.el;
15-
var sceneEl = el.sceneEl;
16-
var i;
17-
var lights = sceneEl.querySelectorAll('[light]');
18-
var lightsNum = 0;
19-
20-
// Remove lights to re-test.
21-
for (i = 0; i < lights.length; ++i) {
22-
sceneEl.removeChild(lights[i]);
23-
}
24-
assert.notOk(document.querySelectorAll('[light]').length);
25-
26-
sceneEl.systems.light.setupDefaultLights();
27-
lights = sceneEl.querySelectorAll('a-entity');
28-
29-
// Remove lights to re-test.
30-
for (i = 0; i < lights.length; ++i) {
31-
if (!lights[i].components.light) { continue; }
32-
lightsNum += 1;
33-
assert.ok(lights[i].hasAttribute(constants.AFRAME_INJECTED));
34-
}
35-
assert.equal(lightsNum, 2);
36-
});
37-
38-
test('removes default lights when more lights are added', function (done) {
39-
var el = this.el;
40-
var lightEl = document.createElement('a-entity');
41-
lightEl.setAttribute('light', '');
42-
el.appendChild(lightEl);
43-
process.nextTick(function () {
44-
setTimeout(function () {
45-
assert.notOk(document.querySelectorAll('[data-aframe-default-light]').length);
46-
done();
47-
});
48-
});
49-
});
50-
});
1+
/* global assert, process, setup, suite, test */
2+
var constants = require('constants/');
3+
var entityFactory = require('../helpers').entityFactory;
4+
var DEFAULT_LIGHT_ATTR = 'data-aframe-default-light';
5+
6+
suite('light system', function () {
7+
setup(function (done) {
8+
var el = this.el = entityFactory();
9+
el.addEventListener('loaded', function () {
10+
var i;
11+
var lights = el.sceneEl.querySelectorAll('[light]');
12+
// Remove lights to re-test.
13+
for (i = 0; i < lights.length; ++i) {
14+
el.sceneEl.removeChild(lights[i]);
15+
}
16+
done();
17+
});
18+
});
19+
20+
test('adds default lights to scene', function () {
21+
var sceneEl = this.el.sceneEl;
22+
var i;
23+
var lights;
24+
var lightsNum = 0;
25+
26+
assert.notOk(document.querySelectorAll('[light]').length);
27+
sceneEl.systems.light.setupDefaultLights();
28+
lights = sceneEl.querySelectorAll('[light]');
29+
30+
for (i = 0; i < lights.length; ++i) {
31+
if (!lights[i].components.light) { continue; }
32+
lightsNum += 1;
33+
assert.ok(lights[i].hasAttribute(constants.AFRAME_INJECTED));
34+
}
35+
assert.equal(lightsNum, 2);
36+
});
37+
38+
test('it does not add default lights to scene if there are used define lights', function (done) {
39+
var el = this.el;
40+
var lightEl = document.createElement('a-entity');
41+
lightEl.setAttribute('light', '');
42+
43+
assert.notOk(document.querySelectorAll('[light]').length);
44+
45+
lightEl.addEventListener('loaded', function () {
46+
el.sceneEl.systems.light.setupDefaultLights();
47+
assert.notOk(document.querySelectorAll('[' + DEFAULT_LIGHT_ATTR + ']').length);
48+
done();
49+
});
50+
el.appendChild(lightEl);
51+
});
52+
53+
test('removes default lights when more lights are added', function (done) {
54+
var el = this.el;
55+
var lightEl = document.createElement('a-entity');
56+
lightEl.setAttribute('light', '');
57+
58+
assert.notOk(document.querySelectorAll('[light]').length);
59+
el.sceneEl.systems.light.setupDefaultLights();
60+
assert.ok(document.querySelectorAll('[' + DEFAULT_LIGHT_ATTR + ']').length);
61+
62+
lightEl.addEventListener('loaded', function () {
63+
assert.notOk(document.querySelectorAll('[' + DEFAULT_LIGHT_ATTR + ']').length);
64+
done();
65+
});
66+
el.appendChild(lightEl);
67+
});
68+
});

0 commit comments

Comments
 (0)