DOMElement lifecycle issues

[gadicc]

1. Repurposing a DOMElement leaves existing/previous classes/attributes/properties intact, e.g:

   

   or as a pseudo-test (can I submit PRs just for tests? :))

   var FamousEngine = require('famous/core/FamousEngine');
   var DOMElement = require('famous/dom-renderables/DOMElement');
   
   var scene = FamousEngine.createScene('body');
   var clock = FamousEngine.getClock();
   FamousEngine.init();
   
   var node1 = scene.addChild();
   var domElement1 = new DOMElement(node1);
   domElement1.addClass('myClass');
   
   clock.setTimeout(function() {
       // node1.removeComponent(domElement1);
       scene.removeChild(node1);
   
       var node2 = scene.addChild();
       var domElement2 = new DOMElement(node2);
       clock.setTimeout(function () {
         // fail/false in 0.5.2 -> 0.7, pass with PR#266
         console.log('test pass:', 
           !document.querySelector('[data-fa-path="body/0"]')
             .className.match(/\bmyClass\b/));
       }, 17);
   }, 17);

   Note: unnoticeable without the setTimeout.

2. Fixed in 0.6. In DOMElement.onDismount, we recycle the element by (amongst other things), setting display: none and leaving it attached to the DOM. But the plane is still visible on WebGL animations:

   

   at least in Chrome stable 43 and beta 44. cc: @trusktr. Confirming that display: none is set (you can see the contents and background color all disappear).

3. Should node.removeComponent(domElement) do any of the stuff that DOMElement.onDismount does? i.e. recycle the domElement.

   What is the expected behaviour if we call removeComponent with and/or without removing the node too?

[trusktr]

Try it out live here: https://fview-lab2.meteor.com/pads/3WusgoWo7BRWpj5JX/4
Switch from page 4 to page 3 a few times, and you'll notice the issue.

[alexanderGugel]

Repurposing a DOMElement leaves existing/previous classes/attributes/properties intact, e.g:

This is definitely a bug IMO. The DOMElement should be completely cleaned up.

In DOMElement.onDismount, we recycle the element by (amongst other things), setting display: none and leaving it attached to the DOM. But the plane is still visible on WebGL animations:

Same as 1: We need to reset the cutout state. @redwoodfavorite

Should node.removeComponent(domElement) do any of the stuff that DOMElement.onDismount does? i.e. recycle the domElement.

Calling removeComponent on a Node results into the onDismount function being called. ( https://github.com/Famous/engine/blob/master/core/Node.js#L657 )

What is the expected behaviour if we call removeComponent with and/or without removing the node too?

Expected behavior should be the same as when removing the node with only the component in question added to it. This is definitely not the case currently.

I think this can be summed up as following: The DOMElement's onDismount function is lacking some cleanup logic!

Thanks for reporting this! I was planning to refactor the DOMElement on Monday anyways. This is very helpful input! Expect a PR early next week.

@DnMllr @redwoodfavorite

[gadicc]

Wow, super quick weekend response. Glad my timing was good :> Have a relaxing weekend, guys!

[solomon-gumball]

Yeah this is indeed an issue with the WebGL cutouts. They're simply not being deleted 'onDismount' which is definitely an oversight. Thanks, will fix this ASAP.

[solomon-gumball]

Looks like @alexanderGugel's PR should fix this issue. It's worth testing though.

[michaelobriena]

@arkadyp comment on same issue

Here is an example where I'm adding, then removing, then re-adding two nodes with DOMElements attached. Depending on the order that I add and remove them, I can end up in a state where the border-radius that is only supposed to be applied to the circle node gets applied to the square node. I think this is due to the way that Engine is recycling divs but I haven't gotten the change to dig into the code.

'use strict';

// Famous dependencies
var DOMElement = require('famous/dom-renderables/DOMElement');
var FamousEngine = require('famous/core/FamousEngine');

// Boilerplate code to make your life easier
FamousEngine.init();

// Initialize with a scene; then, add a 'node' to the scene root
var root = FamousEngine.createScene().addChild();
var circle;
var square;
var el;

function addCircle() {
    circle = root.addChild();
    circle.setSizeMode(1,1,1);
    circle.setAbsoluteSize(300, 300);
    el = new DOMElement(circle);
    el.setProperty('border', '5px solid red');
    el.setProperty('border-radius', '50%');
    el.setContent('circle');
}

function addSquare() {
    square = root.addChild();
    square.setSizeMode(1,1,1);
    square.setAbsoluteSize(300, 300);
    square.setPosition(400, 0);
    el = new DOMElement(square);
    el.setProperty('border', '5px solid green');
    el.setContent('square');
}

addCircle();
addSquare();

setTimeout(function (){
    root.removeChild(square);
    root.removeChild(circle)

    setTimeout(function() {

        // Seems like the node that was was allocated to circle
        // gets reallocated to square but the border-radius property
        // doesn't get properly wiped away.
        addSquare();

        setTimeout(function() {
            addCircle();
        }, 750)
    }, 750)
}, 750);

[gadicc]

Ok, belated update (from me, at least). Part 2 (Mesh cutouts) was of course solved by PR #247 ("fix: Reset cutout on dismount") - thanks @alexanderGugel! - which landed in 0.6.

Regarding the other issues, after updating the test code above from part 1 (DOMElement recycling), I can confirm that PR #266 ("fix: Refactor DOMElement") solves the issue. But it's never been merged. It does seem to have been rebased quite a few times over the last month though :>

@michaelobriena, this is reasonably serious issue. The current famous playbook is basically "don't ever remove nodes with DOMElements or don't ever use classes, styles, or attributes on them". It would be amazing if this landed before the next release :> If there's an issue with particular parts of the refactor, perhaps they could be worked on in isolation rather than blocking the fix (although it looks like a lot of them relate to part 3 too).

[michaelobriena]

I will look into that PR again. Right now with a lot going on here it would be better if this fix was a smaller fix than a full rewrite of DOMElement. If that was to happen I could merge very easily.

[gadicc]

Got it, thanks @michaelobriena!

[trusktr]

As long as the API doesn't change, I don't see why a full (tested) rewrite is not fine. I'm going to merge into infamous/engine and test...

[trusktr]

On second though, too much has changed and there's conflicts. I don't yet know enough to merge properly.

@gadicc Can you confirm if you still have any of the same problems with the latest Engine?