Close a tiny gap in A-Frame loading order

[wmurphyrd]

Description:

There is a small period between document.readyState === 'interactive' and DOMContentLoaded, notably this is when deferred scripts (including modules) run, so the existing checks leave a gap where attaching an entity would cause it to initialze early and fail. fixes #5228

Changes proposed:

• Track whether DOMContentLoaded has fired with a static property on ANode
• Use that static property instead of readyState in connected callbacks to check if DOM is ready

[wmurphyrd]

I originally thought of adding this to window.AFRAME, but I see that's not a current pattern in the codebase to reference the global AFRAME from src files, so added it to the ANode static props instead.

[wmurphyrd]

Feels a little weird having this listener as a side-effect of the module. Another option would be to put ANode.DOMContentHasLoaded = true; inside doConnectedCallback, but that would entail repeating the assignment unnecessarily for each entity in the scene

[wmurphyrd]

Not thrilled about adding an import here - but it seemed better to keep the logic consistent

[vincentfretin]

Thanks @wmurphyrd, the changes looks simple and sound so I'm not afraid of regression here. If this fixes the edge case, that's great. @dmarcos What do you think?

[dmarcos]

I would like to understand better the sequence of events so we make sure we are addressing the root of the issue and not jus the symptoms. Why is there a gap between document.readyState === 'interactive' and DOMContentLoaded? Is it specified in the standard, an implementation quirk in some browsers? What's the interplay with deferred scripts?

[wmurphyrd]

I would like to understand better the sequence of events so we make sure we are addressing the root of the issue and not jus the symptoms. Why is there a gap between document.readyState === 'interactive' and DOMContentLoaded? Is it specified in the standard, an implementation quirk in some browsers? What's the interplay with deferred scripts?

Sure thing. This is specified in the standard:

The DOMContentLoaded event fires after the transition to "interactive" but before the transition to "complete", at the point where all subresources apart from async script elements have loaded.

https://html.spec.whatwg.org/multipage/dom.html#reporting-document-loading-status

So the (abbreviated) sequence of events is:

1. readyState: 'loading'
2. readyState: 'interactive'
3. Execute deferred scripts (including modules)
4. DOMContentLoaded
5. readyState: 'complete'

During step 3, the existing check in ANode.connectedCallback would tell the entity to try to initialize immediately even though DOMContentLoaded has not yet fired an thus the systems have not yet initialized, creating the error seen in the reprex in #5228

[dmarcos]

what about if we check for document.readyState !== 'complete' instead? I think that would wait until deferred scripts have already run

[dmarcos]

would need to call this.doConnectedCallback in the DOMContentLoaded event handler

[dmarcos]

alternatively we can listen to readystatechange and wait for document.readyState === 'complete' instead of listening to DOMContentLoaded

[wmurphyrd]

what about if we check for document.readyState !== 'complete' instead? I think that would wait until deferred scripts have already run

If we checked against readyState complete but still listened for DOMContentLoaded, that would open another gap in the loading order:

1. readyState: 'loading'
2. readyState: 'interactive'
3. Execute deferred scripts (including modules)
4. DOMContentLoaded
5. Execute DOMContentLoaded listeners
6. readyState: 'complete'

During step 5, the readyState would still be interactive, so any entities attached from inside a DOMContentLoaded listener would never initialize as they would exit early from their connectedCallback and stay forever waiting for the DOMContentLoaded event that has already passed

would need to call this.doConnectedCallback in the DOMContentLoaded event handler

It does call through to doConnectedCallback as-is because the listener for the ANode static property is registered first and thus updates ANode.DOMContentHasLoaded before this comes back in via DOMContentLoaded listener. However, if you're open to updating all of these listeners to call doConnectedCallback I would love to make that change because it makes it much easier to subclass these when they call into doConnectedCallback via this rather than prototype.doConnectedCallback like how a-entity does currently

alternatively we can listen to readystatechange and wait for document.readyState === 'complete' instead of listening to DOMContentLoaded

Yes I proposed this as one of the two possible solutions when opening #5228. So that would look like:

if (readyState !== 'complete') {
  // vital that the callback here is connectedCallback and not doConnectedCallback
  // to make sure we don't initialize early on the loading->interactive transition
  document.addEventListener('readystatechange', this.connectedCallback.bind(this));
  return;
}

Note this would conflict with the previous suggesting about changing the callback to doConnectedCallback. If we still wanted to do that as well, we'd need to add an event handler function that checked the current readyState to make sure it was complete before calling doConnectedCallback

[wmurphyrd]

oops on second thought the readystatechange solution proposed above would not work; it could stack listeners and initialize twice. Would have to go with the creating a readystatechange handler

onReadyStateChange () {
  if (document.readyState === 'complete') {
    this.doConnectedCallback()
  }
}

connectedCallback () {
  // Defer if DOM is not ready.
  if (document.readyState !== 'complete') {
    document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
    return;
  }
  this.doConnectedCallback()
}

[dmarcos]

readystatechange

Thanks for all the info and patience. I think this solution looks simpler. Can we incorporate in the PR?

[wmurphyrd]

Sounds good

[wmurphyrd]

@dmarcos I did the readystatechange version, but it occurred to me during the work that this could be a breaking change. Anyone using DOMContentLoaded listeners of their own with A-Frame probably expects the scene and entities to be initialized (although I guess that would depend on whether they registered their listerens before or after A-Frame), but this would change it so scene and entities are never initialized during DOMContentLoaded listener callbacks.

The unit tests may also be bearing this out, as the readystatechange version has one failure whereas this current version had none:

SUMMARY:
✔ 2302 tests completed
ℹ 16 tests skipped
✖ 1 test failed

FAILED TESTS:
    playSound
      ✖ plays sound if sound already playing when changing src
        Firefox 110.0 (Ubuntu 0.0.0)
      Timeout of 3000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

So just wanted to check in if you still want to go this direction with readystatechange. If so, I'll dig into the unit test failure

[dmarcos]

In A-Frame the 'loaded' event should be used. DOMContentLoaded is not very useful since won't guarantee the scene, assets and entities are ready to go.

[wmurphyrd]

readystatechange version is up - that test failure I noted above - cannot get it to happen again. Tried several times locally and it passes in CI as well. Must have been a fluke.

This version still resolves my issues with deferred scripts & SVAWC

[dmarcos]

Thanks so much! Let's merge and see how it goes. Unfortunate that document readystate, DOMContentLoaded and interplay with different kind of scripts (modules, deferred, async) has gotten so complicated. Hopefully it doesn't bite us again.