Skip to content

Commit

Permalink
promote review comments to issue comments, #930 #867
Browse files Browse the repository at this point in the history
  • Loading branch information
@zepumph
zepumph committed Feb 25, 2019
1 parent 7bef4fc commit 21022e3
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 162 deletions.
65 changes: 0 additions & 65 deletions js/accessibility/A11yBehaviorFunctionDef.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,71 +4,6 @@
* "definition" type for generalized type of accessibility option. A "behavior function" takes in options, and mutates
* them to achieve the correct accessible behavior for that node in the PDOM.
*
* REVIEW: There seem to be a lot of attributes that don't seem to be supported well for behavior functions, but I don't
* REVIEW: see any documentation of it. I'd prefer that things would work, but I understand the challenges if we just
* REVIEW: need to have things documented.
* REVIEW: To reproduce an example where this leaves things in a bad state, I ran the scenery playground:
* REVIEW: window.assertions.disableAssert(); // phet.scenery usage breaks some cases, I just use this to work around.
* REVIEW: var n = new scenery.Node();
* REVIEW: n.tagName = 'p';
* REVIEW: n.ariaRole = 'example';
* REVIEW: n.helpTextBehavior = function( node, options, helpText ) {
* REVIEW: options.labelTagName = 'h1';
* REVIEW: options.labelContent = node.ariaRole; // NOTE! we depend on something that does NOT call onAccessibleContentChange
* REVIEW: return options;
* REVIEW: };
* REVIEW: scene.addChild( n );
* REVIEW: n.helpText = 'Bogus';
* REVIEW: n.ariaRole = 'this-should-change';
* REVIEW: So according to our behavior, our labelContent should have changed to 'this-should-change' since it's
* REVIEW: supposed to be a copy of the ariaRole (something I chose that does not call onAccessibleContentChange when
* REVIEW: it updates). Instead it isn't updated immediately:
* REVIEW: <div class="accessibility">
* REVIEW: <h1 tabindex="-1" id="label-2-11">example</h1>
* REVIEW: <p tabindex="-1" id="2-11" role="this-should-change"></p>
* REVIEW: </div>
* REVIEW: There are many cases where this "broken" behavior can happen. Notably, this can also happen in the REVERSE.
* REVIEW: Notably, the "behavior" setup only overrides options WHEN things are rebuilt fully. When a peer is updated,
* REVIEW: it ignores all behaviors (which is concerning!). Maybe we should be evaluating the behaviors on all updates.
* REVIEW: Example (again in the Scenery playground):
* REVIEW: window.assertions.disableAssert(); // phet.scenery usage breaks some cases, I just use this to work around.
* REVIEW: var n = new scenery.Node()
* REVIEW: scene.addChild( n );
* REVIEW: n.tagName = 'p'
* REVIEW: n.helpText = 'Help text'; // using the "default" since the problem happens with it. SHOWS "Help text"
* REVIEW: n.descriptionContent = 'Oops'; // after this change, our behavior for helpText is not checked. SHOWS "Oops"
* REVIEW: n.tagName = 'b'; // some unrelated change that triggers rebuild. NOW it SHOWS "Help text" again since
* REVIEW: // behaviors were checked/executed.
* REVIEW: So to have some usages of setters called from update() where things can be overridden, and others called
* REVIEW: directly WITHOUT behaviors seems to get into really buggy behavior.
* //ZEPUMPH: Let's talk about this more as part of https://github.com/phetsims/scenery/issues/867
*
* REVIEW: I sorted through a lot of the logic, and here's my current assessment. "ok" basically means that the value
* REVIEW: always gets set, and in a consistent way. So all of the "options" on the node that the update uses:
* REVIEW: tagName (ok)
* REVIEW: accessibleNamespace (ok)
* REVIEW: containerTagName (ok)
* REVIEW: containerAriaRole (setter doesn't call onAccessibleContentChange, but the setAccessibleAttribute/removeAccessibleAttribute probably work)
* REVIEW: labelTagName (ok)
* REVIEW: descriptionTagName (ok)
* REVIEW: appendLabel from orderElements (ok)
* REVIEW: appendDescription from orderElements (ok)
* REVIEW: labelContent (setter doesn't call onAccessibleContentChange, can introduce buggy behavior)
* REVIEW: innerContent (setter doesn't call onAccessibleContentChange, can introduce buggy behavior)
* REVIEW: descriptionContent (NOT included) -- check
* REVIEW: inputType (setter doesn't call onAccessibleContentChange, can introduce buggy behavior)
* REVIEW: ariaLabelledbyAssociations (ok, probably won't override with behaviors?)
* REVIEW: ariaDescribedbyAssociations (ok, probably won't override with behaviors?)
* REVIEW: activeDescendantAssociations (ok, probably won't override with behaviors?)
* REVIEW: accessibleAttributes (we WANT to override these with behavior, but can't right now) from onAttributeChange
* REVIEW: inputValue (doesn't look like behaviors can override)
* REVIEW: node.focusHighlight (looks like updates to this are completely broken)
* REVIEW: node.focusable (looks like updates to this are completely broken)
* REVIEW: It would be nice to have all of these consistent, and have one path for setting if needed (that can both be
* REVIEW: used by setters in Accessibility.js AND the "update()" for good performance).
* //ZEPUMPH: Let's talk about this more as part of https://github.com/phetsims/scenery/issues/867
*
*
* @author Michael Kauzmann (PhET Interactive Simulations)
*/

Expand Down
35 changes: 0 additions & 35 deletions js/accessibility/Accessibility.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -689,20 +689,6 @@ define( function( require ) {
setAccessibleHeadingBehavior: function( accessibleHeadingBehavior ) {
assert && A11yBehaviorFunctionDef.validateA11yBehaviorFunctionDef( accessibleHeadingBehavior );

//TODO https://github.com/phetsims/scenery/issues/930 address REVIEW comments
// 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.

if ( this._accessibleHeadingBehavior !== accessibleHeadingBehavior ) {

this._accessibleHeadingBehavior = accessibleHeadingBehavior;
Expand Down Expand Up @@ -748,9 +734,6 @@ define( function( require ) {
* @returns {number}
*/
computeHeadingLevel: function() {
//TODO https://github.com/phetsims/scenery/issues/930 address REVIEW comments
// 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.

// TODO: assert??? assert( this.headingLevel || this._accessibleParent); see https://github.com/phetsims/scenery/issues/855
// Either ^ which may break during construction, or V (below)
Expand Down Expand Up @@ -1129,20 +1112,6 @@ define( function( require ) {
setLabelContent: function( label ) {
assert && assert( label === null || typeof label === 'string', 'label must be null or string' );

//TODO https://github.com/phetsims/scenery/issues/930 address REVIEW comments
// 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" !!!

if ( this._labelContent !== label ) {
this._labelContent = label;

Expand Down Expand Up @@ -1217,10 +1186,6 @@ define( function( require ) {
if ( this._descriptionContent !== descriptionContent ) {
this._descriptionContent = descriptionContent;

//TODO https://github.com/phetsims/scenery/issues/930 address REVIEW comments
// REVIEW: See notes in setLabelContent. Setting the descriptionTagName feels wrong,
// REVIEW: where a behavior would be better.

// if there is no description element, assume that a paragraph element should be used
if ( !this._descriptionTagName ) {
this.setDescriptionTagName( DEFAULT_DESCRIPTION_TAG_NAME );
Expand Down
68 changes: 6 additions & 62 deletions js/accessibility/AccessiblePeer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,38 +173,6 @@ define( function( require ) {

var options = this.node.getBaseOptions();

// REVIEW: Since this includes rewriting of options, does the long-term order matter? I could imagine writing code.
// REVIEW:
// REVIEW: Ahh yup, found one where it creates buggy behavior with defaults. heading/accessibleName both mess with
// REVIEW: labelTagName/labelContent.
// REVIEW: In the scenery playground:
// REVIEW: var n = new scenery.Node();
// REVIEW: n.accessibleName = 'aname';
// REVIEW: n.tagName = 'input';
// REVIEW: scene.addChild( n );
// REVIEW: n.accessibleHeading = 'header';
// REVIEW: The accessible name (executed here first) then gets overwritten with the header. I'm also not quite sure
// REVIEW: why, but the tag is 'hnull', e.g.:
// REVIEW: <div class="accessibility">
// REVIEW: <hnull tabindex="-1" id="label-2-11">header</hnull>
// REVIEW: <input tabindex="0" id="2-11" style="width: 1px; height: 1px;">
// REVIEW: </div>
// REVIEW: Ideally the order is "what is best", and hopefully the defaults (with normal usage) would not run into
// REVIEW: this. But think about what may happen in the future (all settings), and about all the potential ways
// REVIEW: we would want to overwrite behaviors.
//ZEPUMPH: These were developed as two fully separate apis, not ever to be used together in their default form.
//ZEPUMPH: I'm not sure how best to document that though. From a use case perspective, you either have a heading,
//ZEPUMPH: Or an accessibleName.

//ZEPUMPH: This is something JG and I have discussed quite a bit. On one hand these options are all so complicated
//ZEPUMPH: and hard to fully understand everything (lower level api), on the other, when we try to create higher level
//ZEPUMPH: abstractions, there will be conflicts with each other.
// REVIEW:
// REVIEW: As a generalization, how would we handle "arbitrary" behaviors that don't depend on one parameter?
// REVIEW: What if there was a "pipeline" of behaviors that the client could insert general things into?
// REVIEW: It's probably overkill and just brainstorming but if you have this._behaviors = [ b1, b2, b3 ],
// REVIEW: then options = b3( node, b2( node, b1( node, options ) ) )?
//ZEPUMPH: Intereseting! Let's talk about this on Friday
if ( this.node.accessibleName !== null ) {
options = this.node.accessibleNameBehavior( this.node, options, this.node.accessibleName );
}
Expand All @@ -219,10 +187,6 @@ define( function( require ) {


// create the base DOM element representing this accessible instance
//REVIEW: Do we want to check options.focusable instead? Should no accessibleNameBehavior/helpTextBehavior be
//REVIEW: able to modify whether something is focusable?
//ZEPUMPH: I think we need to have a larger discussion about what behavior functions' role should be, I totally
//ZEPUMPH: understand your thought here.
// TODO: why not just options.focusable?
this._primarySibling = AccessibilityUtil.createElement( options.tagName, this.node.focusable, {
namespace: options.accessibleNamespace
Expand Down Expand Up @@ -275,8 +239,6 @@ define( function( require ) {

// if element is an input element, set input type
if ( options.tagName.toUpperCase() === INPUT_TAG && options.inputType ) {
// REVIEW: This looks like something that should be a behavior?
//ZEPUMPH: TODO: Let's talk about this more as part of https://github.com/phetsims/scenery/issues/867
this.setAttributeToElement( 'type', options.inputType );
}

Expand All @@ -288,14 +250,6 @@ define( function( require ) {
this.onActiveDescendantAssociationChange();

// update all attributes for the peer, should cover aria-label, role, and others
// REVIEW: Maybe it's unlikely, but I can TOTALLY see our behaviors above wanting to adjust arbitrary attributes.
// REVIEW: Since this ONLY inspects things directly on the node (not the overridden a11y options above), this is
// REVIEW: not possible. Is that something to consider?
// REVIEW: Upon further review, this totally seems like the case (since it's already being kinda done for aria-label).
// REVIEW: Can the accessible attributes be included with the "options" object, and can behaviors adjust them as
// REVIEW: they see fit?
//ZEPUMPH: TODO: Let's talk about this more as part of https://github.com/phetsims/scenery/issues/867

this.onAttributeChange( options );

// update input value attribute for the peer
Expand All @@ -309,8 +263,6 @@ define( function( require ) {
/**
* Handle the internal ordering of the elements in the peer, this involves setting the proper value of
* this.topLevelElements
* REVIEW: Maybe change the parameter name a bit? I can see someone saying "but options should be optional" and
* REVIEW: the object isn't optional here. Or actually make it optional.
* @param {Object} options - the computed mixin options to be applied to the peer.
* @private
*/
Expand Down Expand Up @@ -445,12 +397,6 @@ define( function( require ) {
var value = dataObject.value;

// allow overriding of aria-label for accessibleName setter
// TODO: this is a specific workaround, it would be nice to sort out a general case for this, https://github.com/phetsims/scenery/issues/832#issuecomment-423770701
// REVIEW: See note in update() above, handling the general case of this seems nice. We're likely to run into
// REVIEW: other cases in the future.
// REVIEW: ALSO we run into weird cases right now of "did you update the aria-label attribute or something that
// REVIEW: ran update() last?" -- the attribute could potentially change unpredictably.

if ( attribute === 'aria-label' && a11yOptions && typeof a11yOptions.ariaLabel === 'string' && dataObject.options.elementName === PRIMARY_SIBLING ) {
value = a11yOptions.ariaLabel;
}
Expand Down Expand Up @@ -805,8 +751,6 @@ define( function( require ) {
AccessibilityUtil.setTextContent( this._labelSibling, content );

// if the label element happens to be a 'label', associate with 'for' attribute
// REVIEW: Should we check _labelTagName directly? Or use a behavior-like strategy for this?
// ZEPUMPH: perhaps implemented with https://github.com/phetsims/scenery/issues/867
if ( this._labelSibling.tagName.toUpperCase() === LABEL_TAG ) {
this.setAttributeToElement( 'for', this._primarySibling.id, {
elementName: AccessiblePeer.LABEL_SIBLING
Expand Down Expand Up @@ -863,7 +807,7 @@ define( function( require ) {
// mark all ancestors of this peer so that we can quickly find this dirty peer when we traverse
// the AccessibleInstance tree
var parent = this.accessibleInstance.parent;
while( parent ) {
while ( parent ) {
parent.peer.childPositionDirty = true;
parent = parent.parent;
}
Expand Down Expand Up @@ -949,7 +893,7 @@ define( function( require ) {
/**
* Update positioning of elements in the PDOM. Does a depth first search for all descendants of parentIntsance with
* a peer that either has dirty positioning or as a descendant with dirty positioning.
*
*
* @public (scenery-internal)
*/
updateSubtreePositioning: function() {
Expand Down Expand Up @@ -1014,11 +958,11 @@ define( function( require ) {
//--------------------------------------------------------------------------
// Helper functions
//--------------------------------------------------------------------------

/**
* Get a matrix that can be used as the CSS transform for elements in the DOM. This matrix will an HTML element
* dimensions in pixels to the global coordinate frame.
*
*
* @param {HTMLElement} element - the element to receive the CSS transform
* @param {number} clientWidth - width of the element to transform in pixels
* @param {number} clientHeight - height of the element to transform in pixels
Expand All @@ -1044,7 +988,7 @@ define( function( require ) {
* Gets an object with the width and height of an HTML element in pixels, prior to any scaling. clientWidth and
* clientHeight are zero for elements with inline layout and elements without CSS. For those elements we fall back
* to the boundingClientRect, which at that point will describe the dimensions of the element prior to scaling.
*
*
* @param {HTMLElement} siblingElement
* @returns {Object} - Returns an object with two entries, { width: {number}, height: {number} }
*/
Expand All @@ -1065,7 +1009,7 @@ define( function( require ) {
* Set the bounds of the sibling element in the view port in pixels, using top, left, width, and height css.
* The element must be styled with 'position: fixed', and an ancestor must have position: 'relative', so that
* the dimensions of the sibling are relative to the parent.
*
*
* @param {HTMLElement} siblingElement - the element to position
* @param {Bounds2} bounds - desired bounds, in pixels
*/
Expand Down

0 comments on commit 21022e3

Please sign in to comment.