removeChild isn't calling invalidateAccessibleContent sometimes

[zepumph]

From creating unit tests as part of #701, I found that the PDOM wasn't updating the arialabelledby associations appropriately when removing child. I'm not too familiar with the AccessibleTree code, but I don't see potential discrepancies between removeTree which calls

        // The full trail doesn't have the child in it, so we temporarily add that for tree removal
        partialTrail.fullTrail.addDescendant( child );
        partialTrail.accessibleInstance.removeInstancesForTrail( partialTrail.fullTrail );
        partialTrail.fullTrail.removeDescendant( child );```
 
and `addTree` which calls 

```js
        partialTrail.fullTrail.addDescendant( child );
        var childInstances = AccessibilityTree.createTree( partialTrail.fullTrail, parentInstance.display, parentInstance );
        partialTrail.fullTrail.removeDescendant( child );

        parentInstance.addConsecutiveInstances( childInstances );

I also know that the accessibility fuzzer is testing the removal of children, so maybe this is a weird edge case, or a bug where I'm doing a bad practice in the aria-labelledby tests without realizing it, like in #811.

I will need to take a closer look at it, and maybe talk to @jonathanolson about it.

[zepumph]

Right now in the tests I'm working around things, by manually calling invalidateAccessibleContent:

    // Moving this node around the scene graph should not change it's aria labelled by associations.
    checkOnYourOwnAriaLabelledByAssociations( j );
    rootNode.addChild( new Node( { children: [ j ] } ) );
    checkOnYourOwnAriaLabelledByAssociations( j );
    rootNode.removeChild( j );

    // TODO: remove this manual call once bug is fixed in removeChild, see https://github.com/phetsims/scenery/issues/816
    j.invalidateAccessibleContent();
    checkOnYourOwnAriaLabelledByAssociations( j );

[jessegreenberg]

I am not sure I understand this test, do we expect the correct attributes when we remove a child from the scene graph? The node's accessible content is no longer in the PDOM at that point, right?

Does this work?

    // Moving this node around the scene graph should not change it's aria labelled by associations.
    checkOnYourOwnAriaLabelledByAssociations( j );
    rootNode.addChild( new Node( { children: [ j ] } ) );
    checkOnYourOwnAriaLabelledByAssociations( j );
    rootNode.removeChild( j );
    rootNode.addChild( j );
    checkOnYourOwnAriaLabelledByAssociations( j );

[zepumph]

Yes @jessegreenberg, your above code snippet does work, because the aria-labelledby attribute values aren't being updated on removeChild, but then you add back the child, it will update the values (and the element ids will all be the same because the trail is the same for j).

Basically think about it this way (this is untested pseudo code )

            rootNode.addChild( new Node({ tagName: 'div', labelContent: 'label',
              ariaLabelledbyAssociation: {
              thisElementName: PRIMARY_SIBLING,
                otherNode: this,
                otherElementName: LABEL_SIBLING
            } } ) );

 // here the primary element's aria-labelledby attribute will have the label id as the value.
rootNode.removeChild( j );
// here the node will have the EXACT SAME aria-labelledby attribute value. This is because the 
// removeChild() function isn't "redrawing the view" (via `invalidateAccessibleContent`).

I'm happy to explain more here or over zoom.

[jessegreenberg]

We discussed 6/27/18. @zepumph demonstrated the problem. We discovered that when we remove Node from the scene graph we do not update any part of the AccessiblePeer, and until now there hasn't been a need to. But in this case, we need to broadcast a message that notifies that aria-labelledby and aria-describedby associations need to be updated when the Node is removed. We made sure that the associations are updated onAccessibleRemoveChild.

However, we were aware that this fix will definitely change with the model/view separation of Accessibility, see #814.

[jessegreenberg]

We can close this issue after the above commits which add #816 (comment).