Issues for JO to review regarding zepumph's scenery-internal accessibility work

[zepumph]

I don't want to loose track, but @jonathanolson I'll let you know when we are ready for a larger review, since most of these are touching the same places in the same files.

• Why do we always need containerParent to hold children. #715
• Improve Accessibility.js API #795
• Is invalidateAccessibleContent the best way to update the PDOM? #814
• Add labelledby/describedby association setters #819

[zepumph]

@jonathanolson we are ready for a batch review of most of the work I did in scenery for accessibility this summer. I'm unsure about your workload and I think this priority is such that we don't want this to drop off and never get done.

@ariel-phet please assign a priority.

[zepumph]

Tagging @jessegreenberg so he is aware of this as well.

[ariel-phet]

@zepumph assigning as high priority. Please mention at status meeting tomorrow.

[jonathanolson]

Ok sounds good. Is most of the code in master, or in branches?

[zepumph]

All is in master. Let me know if you want to discuss general/specifics at any point during it or before you dive in.

[jonathanolson]

Most of the review should be in the above commit (inline REVIEW comments, feel free to promote anything to an issue). Can you look through all of the commit, as I also made minor updates instead of tagging things.

I'd like to get a bit more background on #819 before completing the review.

I'll be available to discuss anything mentioned. Can we schedule a time to discuss the aria associations, and in general to discuss the current behavior pattern? (See the wall of text in A11yBehaviorFunctionDef).

[zepumph]

I think a time to talk sounds very nice! @jessegreenberg and I have 4 hours carved out Friday, sept 28th that would work well to meet for these issues, between 11-3MT. In the mean time I will try to make time to deeply dive into your review so far.

[jonathanolson]

I'll be available then.

[zepumph]

Notes from reviewing the review:

• We should make sure that default behavior functions assert appropriate keys aren't set that would interfere with the higher level api, otherwise a client could have buggy behavior. Maybe we should discuss this further before implementing
• Fix this todo in setHelpText: helptext should only be set on interactive Elements?
• Make a typeDef (or type) for associationObject.
• Decide whether an accessibleAttribute deserves its own POJO
• Discuss/document the difference between which attributes you MUST use the association API for (right now just aria-labelledby and aria-describedby).
• (maybe??) create and enum for the 4 peer elements.

For Friday Discussion with @jonathanolson and @jessegreenberg:

• talk about the generalities of behavior functions as part of Fix a11y behavior functions when using low level and high level API at the same time #867
• Discuss when to have a convenience mixin function call to accessibleAttribute api (like containerAriaRole), and when it should be its own thing completely (inputType).
• Search for ZEPUMPH: and discuss a few questions that came up while looking at a few review comments.
• Describe the association api for JO to review

[zepumph]

In the above commits I got through to setAppendLabel in Accessibility.js, still to do:

• Accessibility.setAccessibleHeadingBehavior, labelContent, focusHighlight
• behavior function def see Fix a11y behavior functions when using low level and high level API at the same time #867
• AccessiblePeer

[zepumph]

@jonathanolson my understanding is that all that is left to review is #819 (I marked the check boxes in the first comment accordingly). Can you confirm that, and if so you can unassign yourself from this and just look at the association issue specifically from here.

[zepumph]

Much of this has been done. Removing the high priority label on it as we finish stuff off.

[zepumph]

#867

[jessegreenberg]

I reviewed this issue. Many items are done or have been done since we last looked at this, but there are still comments pointing to this issue that would be good to review. Also, take another look at #832 (comment).