address REVIEW comments in a11y code

[pixelzoom]

I stumble across multiple "REVIEW" comments in Accessibility.js, see below.

"REVIEW" comments are supposed to be used for a code review, and fully removed (or replaced with a TODO and associated issue) when the code review is completed.

        setAccessibleHeadingBehavior: function( accessibleHeadingBehavior ) {
          assert && A11yBehaviorFunctionDef.validateA11yBehaviorFunctionDef( accessibleHeadingBehavior );

          // REVIEW: Note that this doesn't seem to trigger updating the heading levels of siblings correctly:
          // REVIEW: For example, do the following:
          // REVIEW:   var a = new scenery.Node();
          // REVIEW:   var b = new scenery.Node();
          // REVIEW:   scene.addChild( a );
          // REVIEW:   a.addChild( b );
          // REVIEW:   b.accessibleHeading = 'B';
          // REVIEW:   b.tagName = 'p';
          // REVIEW:   a.accessibleHeading = 'A';
          // REVIEW:   a.tagName = 'p';
          // REVIEW: You will end up with two nested H1s. Presumably this should cascade properly and update the child
          // REVIEW: to a H2 in this case.

        computeHeadingLevel: function() {
          // REVIEW: I'd recommend looking at how node handles this with things. Usually it has an invalidate pattern
          // REVIEW: but in this case, it may be easier to just update the entire subtree whenever an ancestor changes.

        setLabelContent: function( label ) {
          assert && assert( label === null || typeof label === 'string', 'label must be null or string' );

          // REVIEW: It's also concerning that setting this ends up changing another field (especially when it's set to
          // REVIEW: the same value!). Can't we handle some of this in the with an accessible*Behavior pattern?
          // REVIEW: It seems setting and unsetting the labelContent (from null to something to null) makes permanent
          // REVIEW: changes to the labelTagName, which is weird.
          // REVIEW: Can we just have a behavior that does this instead?
          // ZEPUMPH: perhaps worked on as part of https://github.com/phetsims/scenery/issues/867

          // REVIEW: Also this is really weird:
          // REVIEW:   var n = new scenery.Node();
          // REVIEW:   n.labelTagName; // null
          // REVIEW:   n.labelContent = n.labelContent;
          // REVIEW:   n.labelTagName; // "P" !!!

        setDescriptionContent: function( descriptionContent ) {
          assert && assert( descriptionContent === null || typeof descriptionContent === 'string', 'description must be null or string' );

          if ( this._descriptionContent !== descriptionContent ) {
            this._descriptionContent = descriptionContent;

            // REVIEW: See notes in setLabelContent. Setting the descriptionTagName feels wrong,
            // REVIEW: where a behavior would be better.

            this.trigger( 'focusHighlightChanged' );
            // REVIEW: Should it call onAccessibleContentChange()

[pixelzoom]

In the previous commit, I added TODO comments that reference this issue. If these REVIEW comments should be broken out into their own GitHub issue(s), please do so.

[pixelzoom]

Same problem in A11yBehaviorFunctionDef and AccessiblePeer. Altogether, I see 132 occurrences of "REVIEW" in scenery that are related to a11y. Please replace these with TODO comments and associated GitHub issues.

[zepumph]

Agreed! Related issues:

#867
#832

[zepumph]

Removed and moved comments to #867 (comment). Closing