Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for DOMContentLoaded before checking if we have a camera in the DOM #5124

Closed
wants to merge 4 commits into from
Closed

Wait for DOMContentLoaded before checking if we have a camera in the DOM #5124

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1dfad06
be sure that the DOM content is loaded before checking for user defin…
vincentfretin Sep 24, 2022
29115f9
move waitForDOMContentLoaded to utils
vincentfretin Oct 1, 2022
4aef7d8
wait for DOMContentLoaded before initializing components on entity
vincentfretin Oct 1, 2022
2a3b14b
patch attachedCallback to use waitForDOMContentLoaded if registered e…
vincentfretin Oct 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions src/core/a-entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,21 @@ var proto = Object.create(ANode.prototype, {
// Don't .load() scene on attachedCallback.
if (this.isScene) { return; }

// Gracefully not error when outside of <a-scene> (e.g., tests).
if (!sceneEl) {
this.load();
return;
}
utils.waitForDOMContentLoaded().then(function () {
Copy link
Member

@dmarcos dmarcos Oct 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just deferring in line 70 do:

utils.waitForDOMContentLoaded.then(this.attachedCallback.bind(this));

it should solve for both scene and entities I believe.

Copy link
Member

@dmarcos dmarcos Oct 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better would be to do it in the attachedCallback for a-node.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried to put that in a-node attachedCallback first, but some tests failed.
But actually I don't think it makes sense, see a-scene attachedCallback is calling a-entity attachedCallback which is calling a-node attachedCallback. Putting a Promise in a-node attachedCallback means the caller a-entity attachedCallback may not have some attributes set yet like this.sceneEl. The promise needs to be at the top of the calling chain, so this probably make more sense to do patch attachedCallback to do thhe waitForDOMContentLoaded in registerElement after we do the wrapMethods.
This is what I did in the latest commit, I patched the attachedCallback for any registered elements that uses AEntity prototype (so a-scene, a-entity and all primitives a-camera a-gltf-model etc).
I didn't do it for registered elements that uses the ANode prototype (a-assets, a-mixin), some a-assets tests are failing if I do that, but there is not really an issue of system or component init in those anyway.

// Gracefully not error when outside of <a-scene> (e.g., tests).
if (!sceneEl) {
self.load();
return;
}

// Wait for asset management system to finish before loading.
assetsEl = sceneEl.querySelector('a-assets');
if (assetsEl && !assetsEl.hasLoaded) {
assetsEl.addEventListener('loaded', function () { self.load(); });
return;
}
this.load();
// Wait for asset management system to finish before loading.
assetsEl = sceneEl.querySelector('a-assets');
if (assetsEl && !assetsEl.hasLoaded) {
assetsEl.addEventListener('loaded', function () { self.load(); });
return;
}
self.load();
});
}
},

Expand Down
6 changes: 5 additions & 1 deletion src/core/scene/a-scene.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,11 @@ module.exports.AScene = registerElement('a-scene', {
self.attachedCallbackPostCamera();
});

this.initSystems();
// Some systems use querySelectorAll so we need to be sure the DOM content is loaded.
// For example the camera system is using querySelectorAll to check for user defined
// camera before injecting a default camera, but it fails to find the camera if the
// querySelectorAll is executed before DOMContentLoaded.
utils.waitForDOMContentLoaded().then(this.initSystems.bind(this));

// WebXR Immersive navigation handler.
if (this.hasWebXR && navigator.xr && navigator.xr.addEventListener) {
Expand Down
15 changes: 15 additions & 0 deletions src/utils/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,5 +359,20 @@ module.exports.findAllScenes = function (el) {
return matchingElements;
};

/**
* Create a Promise that resolves once the DOM content is loaded.
*
* @returns {Promise}
*/
module.exports.waitForDOMContentLoaded = function () {
if (document.readyState !== 'loading') {
return Promise.resolve(null);
} else {
return new Promise(function (resolve) {
document.addEventListener('DOMContentLoaded', resolve);
});
}
};

// Must be at bottom to avoid circular dependency.
module.exports.srcLoader = require('./src-loader');