fix: Allow for unattached elements during inject call

[ewpatton]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide
• I ran npm run format and npm run lint

The details

Resolves

Fixes an issue where a Blockly workspace cannot be injected into a yet-to-be-attached element.

Proposed Changes

Check whether the ownerDocument of the injection div is the current document if the initial check for containment fails (e.g., due to the element not having been append elsewhere to the DOM).

Behavior Before Change

If a div is created and inject is called to create a workspace before that div has been attached, an error is thrown.

Behavior After Change

The check for membership in the document is made more permissive, so if the div is owned by the document it will still be used to create a workspace (to be attached at a later time).

Reason for Changes

In App Inventor, screens are separated into design and blocks, with the design being shown first. We must create a workspace and have it contain the blocks to participate in code generation, but the workspace will not be shown until the user clicks on the "Blocks" button. The existing check is too restrictive in the cases where a workspace needs to be created before it is shown.

Test Coverage

Tested by loading the playground (semantics of existing working Blockly apps should not change) and also in the branch of App Inventor being brought up to date with mainline Blockly.

Documentation

No documentation updates should be required as part of this change.

Additional Information

[BeksOmega]

If the first check returns true, won't the second check always return true as well? In this case, can we just remove the first check?

[ewpatton]

Technically yes that should be the case, but I haven't fully tested that theory with iframes, e.g., if we create containerElement in an iframe will document.contains(containerElement) pass if document is the document for top rather than the frame. I think it's less risky to add an additional allowance rather than wholesale replace the original logic in the short term.

[BeksOmega]

@maribethb any opinion on this? The check goes back to the beginning of time, but App Inventor has been monkey-patching it out for forever, and they haven't had problems. So I think it is safe to remove.

[maribethb]

@maribethb any opinion on this? The check goes back to the beginning of time, but App Inventor has been monkey-patching it out for forever, and they haven't had problems. So I think it is safe to remove.

I'm fine with the general idea. This has also been requested by folks using web components, e.g. #1114 and I don't know if the new check would work for that use case. I wonder if all we really need is to check that the containerElement exists (i.e. the getElementById call did not fail)?

[BeksOmega]

This has also been requested by folks using web components, e.g. #1114 and I don't know if the new check would work for that use case. I wonder if all we really need is to check that the containerElement exists (i.e. the getElementById call did not fail)?

Makes sense. Since this probably affects several different use cases and would require some manual testing, I'm going to punt on that for now, and just merge this change to unblock app inventor. We can circle back to this later!