DOMElement containing 'img' tag kills all descendant DOMElements

[oldschooljarvis]

If you add a DOMElement containing an 'img' tag to a Node, and that node has descendant nodes which also contain DOMElements, those descendant DOMElements simply disappear.

Note that:

• Image type appears to have no impact (tested: .jpg, .png)
• It doesn't matter if the image src URL resolves or not. Even if it 404s (as in these examples), the behavior is identical to properly linked images.
• Hierarchy depth doesn't matter. Everything beneath dies.
• Mesh renderables and the underlying nodes themselves appear completely unaffected.

Examples + source:

Example 1 (works)

Source: https://gist.github.com/oldschooljarvis/881569559e8964f9f67e

Example 2 (broken)

Source: https://gist.github.com/oldschooljarvis/0e48bbf12f6c72a771cc

Example 3 (broken)

Source: https://gist.github.com/oldschooljarvis/b67d2341d9162630894d

[alexanderGugel]

Thanks for filing the issue!

I'm not sure if we want to support nested img - DOMElements, since it doesn't really make sense when we have an actual nested DOM (as opposed to a flat DOM as in 0.5). E.g. what would the expected DOM hierarchy look like? <img><img><img></img></img></img>?

We should either consider supporting that case by wrapping the elements within a DIV tag (:-1: on that, I think we should "stick to the truth") or throw an error in case of nested self-closing elements.

The described issue doesn't only occur with img tags, but with all nested void elements: area, base, br, col, embed, hr, img, input, keygen, link, meta, param, source, track, wbr (see http://www.w3.org/TR/html5/syntax.html#elements-0).

@DnMllr Do you have an opinion on that?

Update Ok, @oldschooljarvis (nearly) convinced me. It seems like this is far more common than I expected it to be. Creating dummy wrapper nodes is never a good idea IMO.

[michaelobriena]

@alexanderGugel not sure if you and @oldschooljarvis talked offline about this but I am still unsure about supporting nested tags that are not meant to nest. That said, we can definitely give a warning in that case.

[alexanderGugel]

@michaelobriena This has already been addressed in #234
I think this can be closed.
We might want to reconsider nested void elements in the future anyways.

[oldschooljarvis]

@michaelobriena We talked about it in Slack briefly. Basically the argument I put forth was that I found the issue by simply trying to have a node, with an image, rotate around another node, also with an image. Parenting one node to another for translational purposes is an extremely common use case I imagine.

There's a couple ways of solving that:

1. Make the second node a sibling of the first, then continually position it to an empty placeholder node parented to the first node for translational purposes. Obtuse and hacky.
2. Wrap the first image in a div so it doesn't kill off descendant DOMElements:

new DOMElement(node, { tagName: 'div' }).setContent('<img src="images/myImage.png">');

The use of setContent() makes this solution somewhat hacky as well. Perhaps if there was a way to programatically nest child tags/elements within an existing DOMElement without having to resort to setting raw content, it might be better.

Either way, at a minimum there definitely needs to be a warning when descendant DOMElements are being silently killed off.

From a conceptual point of view, I would argue that having the scene graph grind to a halt due to DOM nesting limitations is not optimal. Ideally, the scene graph should be used freely since it's an abstraction, with the engine making the appropriate workarounds manifest in the output.

@alexanderGugel Thanks for the fix! I'll give it a try later today.

[alexanderGugel]

The use of setContent() makes this solution somewhat hacky as well. Perhaps if there was a way to programatically nest child tags/elements within an existing DOMElement without having to resort to setting raw content, it might be better.

I like the idea of being able to programmatically nest DOM elements. I think that's a great idea and something we really need. E.g. Currently we can only support <select> using setContent (fixed in same PR). Being able to programmatically nest DOMElements without actually being controlled through the scene graphs seems like a useful feature in such a scenario. This could be as simple as having an option on the DOMElement to "opt out" from the node's transform being applied (this might break when nesting DOMElements that are being controlled through the scene graph).

Either way, at a minimum there definitely needs to be a warning when descendant DOMElements are being silently killed off.

An error will be thrown in the DOMRenderer in the described situation. PR is submitted.

From a conceptual point of view, I would argue that having the scene graph grind to a halt due to DOM nesting limitations is not optimal. Ideally, the scene graph should be used freely since it's an abstraction, with the engine making the appropriate workarounds manifest in the output.

Agreed. Kind of. If we allow nested <img> elements, we would also have to allow nested <input> elements for consistency (in general all void elements). I really don't like the idea of having the scene graph represent an invalid DOM hierarchy.

[oldschooljarvis]

Agreed. Kind of. If we allow nested <img> elements, we would also have to allow nested <input> elements for consistency (in general all void elements). I really don't like the idea of having the scene graph represent an invalid DOM hierarchy.

Fair enough, just so long as it throws (which it does - I just tested your fix on the void branch). Admittedly both solutions aren't without drawbacks, so it does represent a somewhat agonizing choice.

I was mulling over other solutions besides favoring one hierarchy over the other, and thought maybe a new renderable might offer a solution. Something like "VirtualDOMElement" perhaps, where it's completely decoupled from the notion of idiomatic DOM. On the other hand, I fear that may be going too heavily in the 0.3.5 direction.

Also, one thing I forgot to mention earlier: when you use the setContent() workaround to nest an <img> within a <div>, the setContent() method itself creates another <div> to contain its content, so you're effectively wrapping the image twice.

[alexanderGugel]

@oldschooljarvis

I was mulling over other solutions besides favoring one hierarchy over the other, and thought maybe a new renderable might offer a solution. Something like "VirtualDOMElement" perhaps, where it's completely decoupled from the notion of idiomatic DOM. On the other hand, I fear that may be going too heavily in the 0.3.5 direction.

Definitely worth considering in the longterm I think. From my understanding this would represent an Element in a flat DOM hierarchy. This is likely to introduce a ton of complexity, so I'm not sure if that's something we will tackle in the near future.

Also, one thing I forgot to mention earlier: when you use the setContent() workaround to nest an within a

, the setContent() method itself creates another

to contain its content, so you're effectively wrapping the image twice.

We started wrapping the content into an additional DIV since the content was pushing down the children. Getting rid of the wrapper element would mean we would have to apply the styling to the img directly. This again would mean we would have to mutate the img style. I think it's much cleaner to just always wrap the content in a DIV, otherwise we would have to check if the content has an element within it.

[oldschooljarvis]

Getting rid of the wrapper element would mean we would have to apply the styling to the img directly.

Sorry, I didn't mean to imply wrapping was bad or should be removed. I just brought it up since it seemed to bolster the case for supporting programmatic nesting within an individual DOMElement.

Appreciate the feedback. Thanks.

[solomon-gumball]

Hey guys we can continue the discussion here if we want to but I think I'm going to close the issue if it's okay with you guys @alexanderGugel @oldschooljarvis @michaelobriena . It seems like @alexanderGugel commit addresses the issue for the time being.

[alexanderGugel]

Close please.

[oldschooljarvis]

Closing. Thanks.