Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a11y api for adding PDOM headings #855

Open
Tracked by #953
zepumph opened this issue Aug 30, 2018 · 9 comments
Open
Tracked by #953

Implement a11y api for adding PDOM headings #855

zepumph opened this issue Aug 30, 2018 · 9 comments

Comments

@zepumph
Copy link
Member

zepumph commented Aug 30, 2018

From #795

Code nodes from that issue:

/**
* handle the general heading structure for nodes in the scene graph. This pseudo code relies on 
* a h1 having a child that is an h2. Note that this parent/child hierarchical structure doesn't matter 
* for the PDOM.
* 
* @param {string} headingText
* @param {string} descriptionText
*/
setHeadingStructure: function( headingText, description ){
  
  while(){
    if(this._accessibleParent.labelTagName.startsWith('h')){ // TODO: not always labelTagName?
      parentHLevel = this._accessibleParent.labelTagName.replace( 'h',''); 
      break;
    }
  }

  this.labelTagName = 'h' + parentHLevel+1;
  this.labelContent = headingText;
  this.tagName = 'div';
  setDescriptionyContent( description);
}

// possible names for header option
n.iAmHeader('heading', 'description');
n.a11yHeader('heading', 'description');
n.createHeader('heading', 'description');
n.makeHeader('heading', 'description');
n.dynamicNoninteractiveObjectDammit('heading', 'description');

Since writing the above method/pseudocode, we now have implemented "behavior" functions, that may be helpful in solving this problem. @jessegreenberg I think we should discuss this before bringing it back to a dev meeting for review.

maybe something like:

this.setHeaderBehavior( function( node, options, headerString){

 options.labelTagName = node.parentHLevel +1;
options.labelContent = headerString;
} );

And then we can have a function in node that can recurse up the tree to get the parentHLevel, or it is updated on addChild. I'm not exactly sure what the base case would be, maybe the sections are hard coded h2 elements, and so children know to be h3.

This sounds a little gross, but I'm unsure about how else to approach this.

@zepumph zepumph changed the title Implement a11y api for adding PDOM headers Implement a11y api for adding PDOM headings Sep 14, 2018
@zepumph
Copy link
Member Author

zepumph commented Sep 14, 2018

Today @jessegreenberg and I discussed and implemented a way of accomplishing this in the above commit. Here is a brief explanation:

  • It uses a "behavior" function just like helpText and accessibleName
  • We added a private "_headingLevel" number that is computed based on the computeHeadingLevel function, which recursively searches up the Node tree (via accessibleParent) to find the most recent headingLevel, and sets this heading level to be n + 1.
  • NOT IMPLEMENTED: then we add a headingLevelOverride in which you can explicitly set the proper heading structure if needed. For example, the navigation bar has no parents, but it is a Heading level 2 saying "Sim Resources"

This implementation matches that found in #795, so it would be good to hold off on further work until @jonathanolson is done with #832.

@zepumph
Copy link
Member Author

zepumph commented Sep 22, 2018

We should look at REVIEW: comments from JO from #832 regarding the new heading api in this issue:

        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.

pixelzoom added a commit that referenced this issue Sep 23, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@zepumph
Copy link
Member Author

zepumph commented Sep 24, 2018

#867 will inform this issue as well.

@zepumph
Copy link
Member Author

zepumph commented Mar 5, 2021

Greenhouse has a lot of headings, marking off hold.

@zepumph
Copy link
Member Author

zepumph commented Feb 23, 2023

Unassigning until we carve out time for this.

@zepumph zepumph removed their assignment Feb 23, 2023
@jessegreenberg jessegreenberg removed their assignment Feb 23, 2023
@zepumph zepumph closed this as completed May 4, 2023
@zepumph zepumph reopened this May 4, 2023
@zepumph zepumph closed this as completed May 4, 2023
@phet-dev
Copy link
Contributor

phet-dev commented May 4, 2023

Re-opening because there is a TODO marked for this issue.

@phet-dev phet-dev reopened this May 4, 2023
@zepumph zepumph closed this as completed May 4, 2023
@phet-dev phet-dev reopened this May 4, 2023
@phet-dev
Copy link
Contributor

phet-dev commented May 4, 2023

Reopening because there is a TODO marked for this issue.

@zepumph
Copy link
Member Author

zepumph commented May 4, 2023

Sorry about the shuffle, I was using this issue as a test for phetsims/chipper#946.

@jessegreenberg
Copy link
Contributor

Some review comments about this from #867, for when we continue with this.

      // 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 );
      }
       //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.
 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)
          //  base case to heading level 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants