Is invalidateAccessibleContent the best way to update the PDOM?

[zepumph]

As implementing "higher level" abstractions ontop of the current a11y api in #795, I am wondering if our current approach in optimal.

@jessegreenberg and I have long talked about the inefficiencies of needing to call invalidateAccessibleContent whenever any small piece of the PDOM needs to be redrawn, so I thought it deserved an issue.

My main concern, it is a bummer that the entire invalidateAccessibleContent needs to be redrawn if, for example, you want to change the description sibling from a <p> to a <span>. Currently that change involves recreating the entire Peer, so all of its siblings.

We have gone with this logic for so long because it is quite a bit easier than trying to figure out exactly what we need to redraw in the peer from changes in the Node.

Here is the added complexity this is bringing up in regards to #795 and the abstractions:
invalidateAccessibleContent needs a way to setHelpText, when it redraws the peer. This is to support multiple option order. So there needs a be an "implementation" function in the a11y mixin that the es5 setter and invalidateAccessibleContent can both call. We need this "implementation' function because we want the es5 setter to validate as if it is only called from the client. This is similar to the approach we took in #800.

This problem is complicated further (at least in my head) as we start to support {Property|string} for things like helpText. Although not yet implemented, it seems like if helpText is of type Property, then it will need to register listeners in setHelpTextImplementation. Right now, invalidateAccessibleContent would call the setHelpTextImplementation every time the peer is redrawn, such that we add Property (de)registration to the list of inefficiencies that would occur potentially every frame.

Steps I would like to see in this issue:

1. Discuss any potential overhauls that would eradicate invalidateAccessibleContent and move to an improved, more efficient, and more intelligent system.
2. If not 1, then figure out a pattern that we can use to be a bit more efficient for (a) general redrawing of the PDOM (perhaps only redrawing pieces of the Peer on redrawn), or (b) supporting a good system for type {Property} values.

Tagging @mbarlow12 @jonathanolson @jessegreenberg for discussion. Sorry this is a long one, but it seems like this summer is the time to do this; otherwise it probably won't get done until it is really in the way and a pain in the butt.

[zepumph]

Here is the issue devoted to supporting Property types in a11y api setters, #796

[jessegreenberg]

If we can just recreate the specific DOM elements when we need to that would be a better for performance, and really reduce the size/complexity of invalidateAccessibleContent. So i like option 1. There will still be times where we need to redraw everything, so maybe we coculd keep invalidateAccessibleContent, but refactor things into redrawPrimaryElement, redrawLabelSibling, and so on that the Accessibility trait can then use as well?

[jessegreenberg]

Removing my assignment until we discuss next meeting.

[jonathanolson]

Figuring out what can be permanent, what can be mutated, and what needs to be recreated sounds nice for performance and memory/GC.

Scenery's usual approach would be to have a poolable wrapper around the mutable objects needed, and objects would get created/unpooled/mutated/pooled as needed.

If desired, let me know, as it would be good to discuss via audio.

[jonathanolson]

Super rough notes:

// somewhere, called by peer when it needs to know whether it has a label element (and what tag name)
getLabelTagNameForPeers() {
    // using low-level
    if ( this._labelTagName !== null ) {
        return this._labelTagName;
    }
    else if ( this._accessibleName && this._tagName === 'input' ) {
        return 'label';
    }
    else {
        return null;
    }
}

pool: {
    div: [ .... ],

}


// in peer
invalidateLabelElement() {
    if ( we had one before && we want one && they had the same tag name ) {
        // do inline updates
    }
    else if ( we had one before ) {
        labelelement.freeToPool()
    }
    else if ( we want one ) {
        labeleElement = LabelElement.createFromPool( ... );
        // mutate it 
    }
}



// in Peer update:
function( node ) {
    var options = { // iterate through option keys instead of hardcoded. for efficiency, may be a subset of option keys.
      labelTagName: node._labelTagName,
      ...
      ..
      ...
      ...
    };
    options.accessibleNameBehavior( node, options, options.accessibleName );

    // THEN you create/mutate DOM based on these options
} 

// default behavior
accessibleNameBehavior( node, options, accessibleName ) {
    if ( node.labelTagName ) {
        options.labelContent = accessibleName;
    }
    else if ( node.tagName === 'input' ) {
        options.labelTagName = 'label';
        options.labelContent = accessibleName;
    }
    else {

    }
}

// custom behavior
accessibleNameBehavior( options, accessibleName ) {
    options.ariaLabel = accessibleName;
}



var accessibleOptions = {
    accessibleName: 'lksnrlaksnrt',
    tagName: 'input',
    accessibleNameBehavior: Accessibility.USE_LABEL_ELEMENT   Accessibility.USE_ARIA_LABEL
}

var acccessibleOptions = {
    accessibleName: this._accessibleName,
    tagName: this._tagName
}
accessibleOptions = accessibleNameBehavor( accessibleOptions, this._accessibleName );


function getLabelTagName( << of all of the internal Accessibility.js state >> ) {
    
}


// in Accessibility.js
setAccessibleName: {
    if ( ... ) {
        this._accessibleName = accessibleName;

        for ( var peer in this.peers ) {
            peer.totallyRebuildEverything();

            assert && assert( peer....srltnasrltknasr )
        }
    }
}

// in the peer
audit() {
    // browses the full DOM structure of the peer, and makes sure it is identical to what we want given the node's current state/model.
}





=> labelTagName: 'label'


{
    tagName: 'div'
}

=> labelTagName: null



{
}

=> labelTagName: 'foo'



model/peer:


node.getWhatOurActualLabelTagNameShouldBe() {
    
}


node.labelTagName = 'foo';
node.tagName = 'input';
node.accessibleName = 'laksrntalskrnt'; /// overrides the above labelTagName

[zepumph]

Most of the above notes actually have more to do with #795 than this issue, so I will attempt to consolidate the above notes into work for this issue:

The work needed to be done can be outlines in the following ordered list (ordered on when to implement). More and more we are thinking of Node.js (so Accessibility.js) as the model, and AccessiblePeer.js as the view:

1. Separate the model from the view in a11y scenery. A few things to do, there may be more:
   • Right now there are many spots in Accessibility.js where we are calling updateAccessiblePeers and actually setting the DOMElement content from Accessibility.js, where instead AccessiblePeer should be responsible for that.
   • Make sure that accessibleName/helptext setter isn't reentrant
2. Separate invalidateAccessibleContent so that each model property has its own devoted view code that can set that property functionality on the AccessiblePeer modulary. Follow a pattern similar to marking things "dirty" for SVG text nodes.
3. Using 2, make invalidateAccessibleContent mutable, with an ability in Accessibility.js to just redraw specific pieces of the peer, rather than tearing down the whole thing and rebuilding.
4. Head over to Improve Accessibility.js API #795 and implement the abstraction functions. With these mutable pieces.
5. (as a reach goal, potentially not to be implemented in this issue) Make pieces of AccessiblePeer poolable so that we don't have to recreate DOMElements as much. Instead of traditional pooable, it may instead be an object with an array for each tagName type like: {div: [...], p:[...], ...} because I don't think you can rename the tagName once created. This is especially hard because we will need to be diligent in how to "clear" an element before releasing it back to the pool.

[zepumph]

Today, with help from @jonathanolson, I managed to trace a large bug with the current implementation. We were treating accessibleContentDisplayed differently than how AccessibilityTree and other internal model types were expecting Node to have an accessibleContent value. From here we will want to make sure that we figure out exactly how we want to support this, and implement around that design.

[zepumph]

I spoke to @jessegreenberg recently, and he said that we could potentially get away with removing accessibleContentDisplayed and just using visibility instead. I am going to do that right now, and we can add it back in if we need to.

[zepumph]

After the above commit, the showing of modal dialogs will not toggle the display of other view content in the PDOM, but the scenery code is working as expected and can now be simplified greatly. I will hold off on implementing a solution for the dialogs until I have a better understanding of the final picture of AccessiblePeer, AccessibleInstance, and Accessibility.

[zepumph]

Today I focused on getting aria-labelledby working within the new model/view separation. It was very tiresome to do, and posed a lot of difficulty for me because of the pointers that nodes have to other nodes, we need to make sure that we update other nodes association objects correctly when a node changes (added/removed/or moved around).

I haven't done aria-describedby at all, but will work on that next.

[zepumph]

Now describedby is working as well. From here I want to work on removing the dialog usage of setAccessibleContentDisplayed and to test that on IE/edge to make sure it will work as desired.

[zepumph]

In cdd68f4 I deleted invalidateAccessibleContent and all mentions of it.

In the above commit I did many minor things needed to clean up Accessibility.js and AccessiblePeer.js
I worked to try to make as much of the mixin's functionality mutable as possible, working on ariaRole and containerAriaRole options in part. I also made it so that focusHighlight doesn't recompute the accessible content, because it doesn't effect the PDOM.

From here the only a11y methods that cause the peer to be completely redrawn are as follows:

setTagName
setLabelTagName
setDescriptionTagName
setAppendLabel
setAppendDescription
setContainerTagName
setAccessibleNamespace

This list seems reasonable for now. It is mostly methods that have to do with the elements themselves, and their order. As we need more performance heavy subsystems later on, we can look into converting these triggers into mutable pieces of the AccessiblePeer.

Using #814 (comment) as a guide, it looks like I completed most of number 2, and some of number 3 (electing to not go for updating the mutability of the above functions).

I think that enough work has been done in this issue to begin work back on #795. I am going to merge this branch back into master, because I have tested well and don't think it will mess up anything big. It will be better to hash out bugs now rather than later.

@jonathanolson can you review? I'm not sure how best to go through this. Almost all the changes are in Accessibility and AccessiblePeer. I would especially like help on the TODO at the end of initializeAccessiblePeer.

[zepumph]

I forgot about #832, unassigning.

[pixelzoom]

This issue is ready-for-review, but unassigned. No comments since Jul 2018. Nothing in #832 (parent issue?) since Oct 2018.

@zepumph what needs to be done here? How should this issue be labeled/assigned?

[zepumph]

Looks like #832 covered this review. Closing