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

[vincentfretin]

Description:

Initially discussed on slack https://aframevr.slack.com/archives/C0FAACNA0/p1663208880632399 and https://aframevr.slack.com/archives/C0GG937RN/p1663912806511639
@kylebakerio reported to have an issue in several projects where he had 1/5 chance that the camera used was not the one he defined but the auto injected one.
This can happen in projects with lots of entities defined in the html and there is more chance to reproduce the issue when the camera is defined at the end of the page.
He did a glitch to reproduce the issue every time by triggering an interruption with a script tag just before the camera. https://glitch.com/edit/#!/aframe-camera-glitch?path=index.html%3A31%3A62
He did the initial fix and I made some changes to it.

So what is the issue? The camera system is searching for camera in the DOM with

aframe/src/systems/camera.js

Line 46 in d780484

cameraEls = sceneEl.querySelectorAll('a-camera, :not(a-mixin)[camera]');

and if none was found, it creates a default camera.
The issue here with querySelectorAll is that it doesn't find anything because the DOM content is not fully loaded.

Changes proposed:

• The fix is to wait for DOMContentLoaded before initializing the systems. The change in this PR fixes the issue on the glitch above and all current tests are passing. I wanted to add a unit test (this is why I found that all the tests weren't executed really run all tests and fix broken tests #5123) but I don't know how to write a failing test for this case.

[kylebakerio]

Also possibly seems worth mentioning that this auto-injection of a camera bug seems to have come up and caused the hubs team some pain, and possibly contributed to their beginning to transition away from a-frame.

[dmarcos]

If this doesn't reproduce consistently there might be something we are misunderstanding or a bug somewhere. As per MDN DOMContentLoaded only fires when the DOM has been completely parsed. In my mental model none of the systems / components should initialize before DOMContentLoaded fires. Obviously something is not right: either my mental model or we have a subtle bug somewhere.

[kylebakerio]

Not sure what you're referring to; this bug does reproduce consistently, and I have isolated with breakpoints in aframe code systems initializing before the DOM has finished loading.

[kylebakerio]

See here for reproducible: https://glitch.com/edit/#!/aframe-camera-glitch?path=index.html%3A31%3A62

[kylebakerio]

As you can see in this screenshot, if we use aframe 1.3.0, and stick a breakpoint in systems/camera.js, we can observe the camera system initialization happening, freeze there, and see that document.body is not yet populated with the rest of the body that include the camera declared in the HTML:

[kylebakerio]

In my mental model none of the systems / components should initialize before DOMContentLoaded fires.

This seems to be the mistake; at times, this seems to not be true. Here's I've shown one example that forces it to not be true in a very simple scene, but I've also observed it in two other complex projects (one where I solved it by declaring the camera at the top of my HTML instead of the bottom), and it is also consistent with a comment made by the Hubs team who seem to have encountered this behavior.

As far as I can tell, there is no code guaranteeing that A-Frame wait until domcontentloaded, which is why I tried adding that here--and doing so seems to fix the issue.

[dmarcos]

We need to understand why in some cases systems / components initialize before DOMContentLoaded fires. I don't think they should. Also worth checking if behavior is consistent across browsers

[dmarcos]

A good experiment could be moving the setupInitialCamera to an update method of the system. See if that makes any difference.

[kylebakerio]

I just tested. The bug also appears in firefox, and the fix also works in firefox.

Can you explain why the systems shouldn't initialize before DOMContentLoaded without this patch? What in the code is supposed to make A-Frame wait to initialize its systems until the dom content is loaded?

While we ended up going with this listener as per my original suggestions, Vincent suggested trying several other options before finally agreeing this seemed best. As per slack, here are several of his comments from this discussion:

Instead of your ready function, can't you just use the loaded event listener on the scene? It surely would be better if you want to create a PR for this.
this.addEventListener('loaded', function () {
self.initSystems();
});
All systems don't need to wait until the DOM is loaded. And if you look at the light system, it's using this.sceneEl.addEventListener('loaded', bind(this.setupDefaultLights, this)) in init already, so the above fix will break the light system because it will never add the default lights because the loaded event was already fired. I think you should better make your fix in the camera system. Can you try replacing
this.setupInitialCamera();
with

this.addEventListener('loaded', function () {
 self.initSystems();
});

?

the commit e0c8ff7 introduced initializing the camera before the scene is fully loaded, it introduced the nodeready event for that in a-node attachedCallback so when the camera element is in the dom

aframe/src/core/a-node.js

Line 38 in 5759e99

this.emit('nodeready', undefined, false);

, it's used here

aframe/src/systems/camera.js

Line 65 in d780484

cameraEls[i].addEventListener('nodeready', function () {

but it doesn't make much sense, cameraEls array would be empty anyway if querySelectorAll returned nothing.

can you try
document.addEventListener("DOMContentLoaded", bind(this.setupInitialCamera, this))

In the end I think your fix is the best, some other third party systems may have the same issue of using querySelector(All), so fixing it for all systems is great.

I tried all (I think three?) of Vincent's proposed alternate solutions, none fixed the issue and/or did not produce other bugs.

If there is no existing guarantee on A-Frame to wait for the DOM to load, then we should probably add one. We are seeing that in the wild, it does not always wait for the DOM to load, it just happens to incidentally not fire too early most of the time. Is there any argument to not make this explicit? Are you wondering whether we should also add this condition elsewhere?

I guess my thing is, asking kind of the same thing in a different way: Why do A-Frame's systems not just try to init before the body has even begun to be parsed? What triggers it? It goes back to some attachedCallback iirc, but it wasn't clear to me what fires that during my brief digging.

[kylebakerio]

A good experiment could be moving the setupInitialCamera to an update method of the system. See if that makes any difference.

I can try this tomorrow, but honestly if it works I would be more worried that this is a hack that works because of an interruption, more akin to relying on something like setTimeout(f, 0) and hoping the HTML parsing task has completed during the async jump, though maybe there's something there I'm misunderstanding.

[vincentfretin]

There is no guarantee that DOMContentLoaded is executed before initSystems, from my understanding the scene initialization starts with the first attachCallback of a-scene node while the DOM is constructed and the whole page may not have been parsed yet, that's also why we have all the loaded logic in a-node to wait for DOM nodes to be initialized.
Only a script tag with defer=true has the guarantee that the html is parsed, the script is evaluated and then DOMContentLoaded fired. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

[vincentfretin]

Moving setupInitialCamera from init to update won't change anything. update is called after init right away.

aframe/src/core/system.js

Lines 42 to 43 in 30c3faf

	this.init();
	this.update({});

[vincentfretin]

We have another place in the code where we encounter the issue, see 7f017a3

We also have the issue with component init in general where a querySelector in init may randomly fail to find an element in the page. I saw this several times in the past myself and on some glitch examples that other persons wrote when I was helping with networked-aframe on slack. If you want to use querySelector in init, you really need to ensure you do that when document.readState !== "loading" or in DOMContentLoaded listener or when the scene is loaded.
We also have the long standing issue in networked-aframe networked-aframe/networked-aframe#267 where the networked component may not find the template with querySelector and will default silently to the default networked schema. It's actually a huge issue, I had an app that worked for months, and one day some weird behavior happened in my app, it was because of this issue.
Anyway this is just to say that system init and component init may execute before DOMContentLoaded and querySelector(All) won't work.

[dmarcos]

We can try to defer attachedCallback logic until DOMContentLoaded fires? should solve the issue for systems and components. Not sure about side effects.

[kylebakerio]

Is there a clear description of exactly how A-Frame bootstraps and how order of execution works in A-Frame? When I was just attempting to read the source, like I said, I got blocked at trying to figure out when the attachedCallback fires.

What I would be getting at is: is it possible that solving this for system init solves this entirely? Do we know that we need to fix this for components as well? I don't know of any reported bugs related to that, but without knowing exactly the order of execution when A-Frame is starting up, I can't really speak to it either way, I just know that waiting for the DOM to init before A-Frame decides if it should inject cameras or lights makes sense.

Also: would be nice if there were a flag to turn that functionality off, if nothing else. Honestly, imo, we should actually remove the auto-injection functionality, and just include a camera and a light in the hello world demos. I think that would be much cleaner and more sane, and is a bit over-helpful and adding unnecessary complexity in A-Frame. A-Frame makes adding those two things easy enough with primitives as-is, auto-injecting just makes it a bit more confusing and mysterious and adds potential for bugs like this. (But that's probably a discussion for another time.)

[vincentfretin]

@dmarcos I'll do more tests later with attachedCallback and let you know.

@kylebakerio No I don't think there is a open issue about the component init where querySelector can randomly fail. I didn't considered it as an aframe issue but more like an issue in my own code until now so I never reported it or worked on fixing it in the aframe code.
As I already said on slack, the light system has a defaultLightsEnabled option, I think it would be nice to have defaultCameraEnabled in the camera system as well. You can probably prepare a PR for that separately later.

[vincentfretin]

@dmarcos you should merge #5123 first so we can really make sure there is no side effect, at least with the existing tests coverage.

[vincentfretin]

The attachedCallback function is called from the registerElement polyfill defined in
https://github.com/dmarcos/document-register-element/blob/nativeSetAttributeBuild/src/document-register-element.js#L523
I think this is called there
https://github.com/dmarcos/document-register-element/blob/cd665644367686d5645048f853d22fef9584bbf0/src/document-register-element.js#L518
from the MutationObserver callbacks defined here
https://github.com/dmarcos/document-register-element/blob/cd665644367686d5645048f853d22fef9584bbf0/src/document-register-element.js#L561

[vincentfretin]

Components initialization is done with load(), so using waitForDOMContentLoaded before calling load() in a-entity attachedCallback like you suggested will fix the querySelector randomly failing in a component init.
For a-scene, it has its own attachedCallback, so adding waitForDOMContentLoaded there is the best thing to do for systems init.
All the tests are still passing.

[dmarcos]

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.

[dmarcos]

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

[vincentfretin]

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.

[dmarcos]

Not sure if introducing an exception for entities is a good idea. It breaks consistency. We can try to defer all component life cycle methods here:

https://github.com/aframevr/aframe/blob/master/src/core/a-register-element.js#L159

That way the model will be simple to think about with no ad-hoc exceptions.

[vincentfretin]

The line you gave here will wrap for the given prototype the four methods attachedCallback, attributeChangedCallback, createdCallback, detachedCallback to call the super method (the method of the prototype we inherit from). For registered elements that inherits from AEntity prototype, this wrapMethods function is called twice

aframe/src/core/a-register-element.js

Lines 114 to 115 in 7fb1d75

	wrapMethods(newObj, ANodeMethods, obj, ANode.prototype);
	wrapMethods(newObj, AEntityMethods, obj, AEntity.prototype);

So doing the change here to call waitForDOMContentLoaded will add waitForDOMContentLoaded twice, once for ANode.attachedCallback and second time for AEntity.attachedCallback. Like I said wrapping in a Promise the method on anything other than the method in the top calling chain is not good. And using the waitForDOMContentLoaded for ANode make some assets tests fail. Also it doesn't make sense to me to wait for DOMContentLoaded on attributeChangedCallback or detachedCallback for example, we could do it but it's really unnecessary. I tried to patch on createdCallback instead of attachedCallback, some tests fail.

[kylebakerio]

not sure if I made some mistake while quickly trying to troubleshoot, but figured I'd mention this:

1. I just ran into this camera bug again, by coincidence, in a new client project
2. Vincent, I tried to pull your latest changes and run npm dist and get the result--unfortunately, that build with doing it this way did not seem to fix the issue for me? Perhaps I didn't notice a build process error, but you may want to remix my original repro glitch and verify that your proposed fixes work if you aren't doing so already.
3. My original prototype patch does fix the issue, so I'll just use that build for now...

I did not triple-check my experience, so it's possible I made some error, or that something went wrong with the build tools, etc.

[vincentfretin]

I just verified right now with the latest changes in the branch, it's working for me
https://glitch.com/edit/#!/aboard-malleable-basket

[kylebakerio]

That does seem to work for me--must have been a build tool error, perhaps I should wipe my old node_modules and do a clean npm install.

[dmarcos]

I opened this #5136 that I think will help us address the root cause of the issue and also modernizes the code base.

I would love to add a test that reproduces the camera issue to make sure it gets fixed. Do you think you can help with that? Thanks so much for the effort and patience.

[vincentfretin]

Thanks for reviving the custom elements v1 work. I'll review your PR later this weekend.
For the test, with the current way of writing tests that use document.createElement to create the elements and the tests run via karma inside the same page with the mocha test runner, it's not possible to write a test that reproduces this issue. You really need to properly load a html file to write an automated test, like we did here with the glitch example. I don't think you can write a test like that with the current testing framework, like you would do with any other e2e test framework like selenium or puppeteer

[dmarcos]

I think we can close this in favor of #5136. Hopefully the entity interdependency issues are gone for good now

[vincentfretin]

I just verify again on the glitch example and latest aframe master build with some logs in a-node and a-entity, the addEventListener('DOMContentLoaded', ...) logic works as intended.