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

[Discussion] Rethink cases for setAccessibleContent #686

Closed
jessegreenberg opened this issue Oct 11, 2017 · 39 comments
Closed

[Discussion] Rethink cases for setAccessibleContent #686

jessegreenberg opened this issue Oct 11, 2017 · 39 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

@zepumph found a few issues with setAccessibleLabel and setAccessibleLabelAsHTML. The default behavior for these functions is to set the label value as the innerText or innerHTML of the node. This is confusing because for nodes with children that also have accessible content it will produce html like

<div> (node's accessible tag)
  <button>Child Button</button> (child of node)
  <input> (child of node)
  "Accessible Label" (accessible label of node)
<div>

More intuitive default behavior would be the following (the label should be outside of the child content):

<div> (container element
  <div> (node's accessible tag)
    <button>Child Button</button> (child of node)
    <input> (child of node)
  </div>
  <p>Accessible Label</p> (accessible label of node
</div>

With setAccessibleLabelAsHTML it is easy to blow away accessible content of child nodes because the default is to set the innerHTML of the Node's DOM element. (Very scary!)

We would like to change the behavior of setAccessibleLabel to only set content for a label element (maybe default to p) or set the content for aria-label. New functions like setAccessibleInnerHTML and setAccessibleInnerText will explicitly set the innerHTML or innerText to handle cases like

<button>My label as inner text</button>

This change would likely make this kind of mix of inner content unsuported:

<div>
  <p>Some text content</p>
  Other text content
</div>

In general I think this is OK, I can't think of a case where this would be better than

<div>
  <p>Some text  content</p>
  <p>Other text content</p>
</div>

But @terracoda we wanted to check this limitation with you before continuing, does this seem OK?

@terracoda
Copy link

@zepumph and @jessegreenberg, I need to think this over a bit to understand it.

There are a few different ways to create and "accessible name" for an element, and we should be building in all the ways to do that. You need different "naming" solutions for different situations.

@jessegreenberg
Copy link
Contributor Author

@zepumph @terracoda and I discussed this today, and @terracoda said it was OK to proceed with this as long as we still support the common ways of naming elements with labels: aria-labelledby, label element with the for attribute and innerText/innerHTML.

@terracoda
Copy link

terracoda commented Oct 13, 2017

All interactive elements must have an "accessible name" in the Accessibility Tree. The API should make sure all techniques for providing an accessible name are provided.

The most common ways to provide an accessible name (preferably in this order) are to:

  • provide understandable content inside an interactive element, like a button <button>Accessible Name for My Button</button>
  • associate the label element to the interactive element it is labeling with a for attribute <label for="my-checkbox">My Checkbox's Accessible Name</label><input type="checkbox" id="my-checkbox>
  • provide the name with the aria-label attribute inside the interactive element <input aria-label="My Reset's Accessible Name" type="reset">
  • associate the html content of one element to another element with the aria-labelledby attribute when one of the three methods above are not naturally available, or to associate one name to a group or region. Examples for aria-labelledby are more diverse and complex. Several examples are available at MDN.

@terracoda terracoda removed their assignment Oct 18, 2017
@zepumph
Copy link
Member

zepumph commented Oct 19, 2017

@jessegreenberg, @mbarlow12 and I just spent some time discussing this one. It seems to be a pretty multi-faceted api. We want to be able to handle quite a bit of flexibility in how we have a "label" (what we are calling a separate tag that goes either before or after the node's tag) or set and Accessible Name (supported examples of Accessible Name: (1) <label> tag with for attribute; (2) aria-label attribute set on another "label" tag; and (3) aria-labeledby on the node itself (not supported by this api change;).

Currently we are looking into trying to be more explicit with the calls, and less clever in the mixin to make the resulting html more expected and clear for all developers.

One thing we thought about was to have 2 options, instead of just accessibleLabel plus flags. These options would be to create a separate tag to hold content (like a "label"), and the other is to directly set the innerHTML/textContent of the node (only allowed if the node does't have any children).

We will continue discussing the next time we meet. Perhaps next Wednesday?

@jessegreenberg @mbarlow12 please refine my comment as you see fit.

@zepumph
Copy link
Member

zepumph commented Oct 26, 2017

Today @jessegreenberg @mbarlow12 and I spoke again, and tried to gain a better understanding of our use cases. @terracoda discussed them out nicely in #686 (comment) and we tried to go off of that.

Imagine a new mixin option called accessibleName. It only supports creating an accessible name for something, rather than the current accessibleLabel which has other uses too. Since @terracoda said there are 4 ways to have an accessible name, here are the 4 use cases:

// javascript
accessibleName: "George", use cases below

//1
<button>George</button>

//2
<label for="input">George</label>
<id="input" input/>

//3
<ul aria-label="Geoge"></ul> // Barlow: Redundant? perhaps we can just use aria-labelledby exclusively with another tag.

//4
<div> // parent container
<h3 id="accessibleName">George</h3> // SR says "George". Or does SR just negate this because it know that it labelling something else?
<ul aria-labelledby="accessibleName"></ul> // SR says "George" again?
</div>

Besides case 4, we would also address the case of having another node far away label your node.
@terracoda we have a question for you. Currently there is no support in our new thought process for

<div>
<h3>George</h3>
<ul></ul>
</div>

But this is used quite often in our code currently. We are wondering if:

<div>
<h3 id="accessibleName">George</h3>
<ul aria-labelledby="accessibleName"></ul>
</div>

is better/ preferred replacement. Do we want to always have both options above? What do you think about it?

is

<div>
<h3 id="accessibleName">George</h3>
<ul aria-labelledby="accessibleName"></ul>
</div>

actually not a better option at all?

Thanks!
the a11y dev team.

@mbarlow12
Copy link
Contributor

One other valid way to provide AT with an accessible name is

<label>
    <input type="something">Input name
</label>

This may be a more concise option for case 2 in #686 (comment).

@mbarlow12
Copy link
Contributor

mbarlow12 commented Oct 26, 2017

@terracoda We also discussed possibly not supporting case 3 (using aria-label). Since it appears that aria support in general is somewhat limited, we thought it may be more straightforward to default to case 4 and explicitly delineate the desired labelling element. Also, in that vein, we're curious if the 2 html patterns (aria-label vs aria-labelledby) affect how virtual cursors move through the page.

From what I understand, we currently provide the following:

<div>
<h3>George</h3>
<ul></ul>
</div>

And screen readers will see the heading since that is their default behavior. I'm curious as to the behavior of the following:

<div>
<h3 id="labelForUL">George</h3>
<ul aria-labelledby="labelForUL"></ul>
</div>

Will the curser pass over the h3, knowing that it provides the accessible name for the ul, and pass focus to the ul or will it behave as the former, first reading the contents of the h3 then moving to the ul—effectively repeating the ul's accessible name?

@zepumph
Copy link
Member

zepumph commented Oct 26, 2017

Unassigning, to be continued next meeting.

@zepumph zepumph removed their assignment Oct 26, 2017
@jessegreenberg
Copy link
Contributor Author

Initially this issue was to discuss the problematic case of accessible content that looks like:

<div> (node's accessible tag)
  <button>Child Button</button> (child of node)
  <input> (child of node)
  "Accessible Label" (accessible label of node)
<div>

Instead of reworking supported HTML structures and API, what if we kept a similar API but remove this confusing case?

@zepumph @mbarlow12 what do you think of an API that looks like this:

//--------------------------------------------------------------------------
new Node( {
  tagName: 'button',
  innerAccessibleLabel: 'My Button' // will error if this node has any children
} );

// creates HTML
<button>My Button</button>
//--------------------------------------------------------------------------

new Node( {
  tagName: 'ul'
  accessibleLabel: 'My Label'
} );

// creates HTML
<div>
  <p>My Label</p>
  <ul></ul>
</div>
//--------------------------------------------------------------------------

new Node( {
  tagName: 'input'
  accessibleLabel: 'My Label',
  labelTagName: 'label'
} );

// creates HTML
<div>
  <input id='my-node'>
  <label for='my-node'>My Label</label>
</div>
//--------------------------------------------------------------------------

new Node( {
  tagName: 'input',
  accessibleLabel: 'My Label',
  useAriaLabel: true
} );

// creates HTML:
<input aria-label="My Label">

While aria-labelledby and aria-describedby go through different functions.

@zepumph
Copy link
Member

zepumph commented Oct 26, 2017

I like that! By doing this are we only ever setting innerHTML, or do we still support textContent?

Also how to handle a situation where where we aren't trying to add a label, but just apply content to a paragraph:

<p>The dog went to the park</p>

@jessegreenberg
Copy link
Contributor Author

By doing this are we only ever setting innerHTML, or do we still support textContent?

Good point, we will still need accessibleLabelAsHTML and innerAccessibleLabelAsHTML to support

//--------------------------------------------------------------------------
new Node( {
  tagName: 'button',
  innerAccessibleLabelAsHTML: '<b>My Button<b>' // will error if this node has any children
} );

// creates HTML
<button><br>My Button</br></button>
//--------------------------------------------------------------------------

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Nov 2, 2017

Also how to handle a situation where where we aren't trying to add a label, but just apply content to a paragraph

My thought was the we treat this just like buttons, so it would look like

//--------------------------------------------------------------------------
new Node( {
  tagName: 'p',
  innerAccessibleLabel: 'The dog went to the  park.'
} );

// creates HTML
<p>The dog went to the park.<p>
//--------------------------------------------------------------------------

So that through the API the only difference between p and button is tagName, innerAccessibleLabel behaves the same for both.

@terracoda
Copy link

@jessegreenberg and @zepumph, you do want to support things like this:

<p>The <b>big dog</b> went to the park and the little one stayed home.</p>

But I would never want to see <br> or <br/> which are non-closing tags inside a <button> element's content. Also, a pair of <b> tags should be unnecessary inside a button element as one could use CSS to style the label of a button.

The difference between the button element and the p element is that the button requires an accessible name in the accessibility tree, whereas the content of a p is content. I don't think it is an "accessible name" or "label" in the same way.

@jessegreenberg
Copy link
Contributor Author

But I would never want to see <br> or <br/>

Thanks, I meant to say <b> instead of <br>, sorry.

I don't think it is an "accessible name" or "label" in the same way.

I agree, and that is one of the reasons I like the proposal in #686 (comment). This is not the "accessible name" used in the accessibility tree, this is "inner accessible content" for whatever tag name was specified with the function setTagName. The resulting HTML will look the same whether it is button or p.

@terracoda
Copy link

terracoda commented Nov 2, 2017

@jessegreenberg, I agree that #686 (comment) is a good approach. I would like to consider a couple changes to the list example in order to provide code validity and perhaps better semantics.

The button example and the input examples make perfect sense.

The list example, however, seems a little strange to me. I am not sure a paragraph would provide the best semantics of a label for a list without using aria-labelledby. That said, a heading (of which there are 6 levels) could be interpreted more like a label. This is just my feeling (not sure how to find an absolute answer on this, maybe on whatsock.com).

Also, the generated code for a list should include at least one pair of list item tags, <li></li>. A list is not valid without list items. I'm not sure what the Javascript should be, but I think the generated HTML would more accessible for our purposes if it looked something like this:
//--------------------------------------------------------------------------
new Node( {
tagName: 'ul'
accessibleLabel: 'My Label'
accessibleLabelLevel: 2 - not sure how this is done in JS
} );

// creates HTML

<div>
  <h2>My List's Label</h2>
  <ul>
    <li>List item content</li>
  </ul>
</div>

//--------------------------------------------------------------------------

Does that make sense to you?

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Nov 2, 2017

@terracoda sounds good, the p for the list was just an example, it could be any arbitrary tag. I agree that lists should have content in use, and that would be supported by #686 (comment). To create your example, code it might look like this:

var listNode = new Node( {
  tagName: 'ul',
  labelTagName: 'h2',
  accessibleLabel: 'My Lists Label'
} );

var itemNode = new Node( {
  tagName: 'li',
  innerAccessibleLabel: 'List item content'
} );

listNode.addChild( itemNode );

To create

<div>
  <h2>My List's Label</h2>
  <ul>
    <li>List item content</li>
  </ul>
</div>

@terracoda
Copy link

@jessegreenberg, thanks for the example. Sounds good to me.

@jessegreenberg jessegreenberg assigned zepumph and unassigned zepumph Jan 31, 2018
@terracoda
Copy link

terracoda commented Jan 31, 2018

@jessegreenberg, @mbarlow12, and @zepumph, content tags do not always need labels (i.e., and accessible name) in the technical sense.

It would be ideal if the following HTML was supported by the API

<div>
  <h2>My List's Main Label (i.e., heading)</h2>
  <ul>
    <li><h3>Sub List Item Label (i.e., heading)</h3>
      <ul>
         <li>Sub list item content</li>
      </ul>
    </li>
  </ul>
</div>

Keep in mind that a heading is not always needed in content, and the following three code examples are also valid HTML

1. Nested list with a paragraph

<div>
  <h2>My List's Main Label (i.e., heading)</h2>
  <ul>
    <li><p>Sub List Item Label</p>
      <ul>
         <li>Sub list item content</li>
      </ul>
    </li>
  </ul>
</div>

2. Nested list with a formatting tag

<div>
  <h2>My List's Main Label (i.e., heading)</h2>
  <ul>
    <li><strong>Sub List Item Label </strong>
      <ul>
         <li>Sub list item content</li>
      </ul>
    </li>
  </ul>
</div>

3. Nested list with no tag at all

<div>
  <h2>My List's Main Label (i.e., heading)</h2>
  <ul>
    <li>Sub List Item Label 
      <ul>
         <li>Sub list item content</li>
      </ul>
    </li>
  </ul>
</div>

OOPs @jessegreenberg, @mbarlow12, and @zepumph, I posted too soon.
I edited the code examples after first post.

@terracoda
Copy link

Re @mbarlow12 #686 (comment), I agree that code is efficient and is valid.

My understanding is that it is interpreted the same as

<label for="myInput">My Input's Label</label>
    <input id="myInput" type="something">

Though, I imagine their could be subtle differences in how focus is managed by browsers between the two code examples - especially when using the cursor keys.

@jessegreenberg
Copy link
Contributor Author

It would be ideal if the following HTML was supported by the API

@terracoda that structure is definitely supported in this proposal.

"3. Nested list with no tag at all" is what is currently NOT supported by the proposed API for the same reason as your comment #686 (comment). When we discussed this before we agreed that it isn't necessary to support mixing Text and Element child nodes because equally accessible HTML could be chosen.

@terracoda
Copy link

OK, sounds good. We can probably live without option 3. Nested lists with no tag at all.

@zepumph
Copy link
Member

zepumph commented Feb 20, 2018

Adding to a11y dev meeting just to get refreshed on this, and to make sure I understand how to proceed.

@zepumph
Copy link
Member

zepumph commented Mar 6, 2018

In the keyboard nav meeting today, we (@terracoda leading) discussed the need for multiple aria-labelledby and aria-describedby tags on a single element, we should add that to the api in this issue.

@zepumph
Copy link
Member

zepumph commented Mar 6, 2018

Marking as high so that we make sure to get to it tomorrow.

@jessegreenberg
Copy link
Contributor Author

We would like to reconsider having both setAccessibleLabelAsHTML and setAccessibleLabel. It is possible we only need one, and having both complicates the API. To determine whether or not we can get rid of one, we need see if there is a detectable performance degredation by using setAccessibleLabelAsHTML.

@jessegreenberg
Copy link
Contributor Author

@zepumph found this link https://jsperf.com/innerhtml-vs-textcontent, testing the performance of textContent vs innerHTML. Here are the results running on @jessegreenberg's machine.
capture

textContent is two orders of magnitude faster.

@jonathanolson we are trying to decide if this makes it worthwhile to support both textContent and innerHTML. Would you recommend that we support both, or is this performance difference irrelevant in the scheme of things?

@jonathanolson
Copy link
Contributor

@jonathanolson we are trying to decide if this makes it worthwhile to support both textContent and innerHTML. Would you recommend that we support both, or is this performance difference irrelevant in the scheme of things?

Depends on how many times this will need to be called per frame, and if the performance is worse on iOS/etc. Also depends on convenience. If it takes 0.01ms for a 16.7ms budget for each frame, and is only called a few times per frame, I'm definitely not concerned about performance.

@jonathanolson jonathanolson removed their assignment Mar 8, 2018
@zepumph
Copy link
Member

zepumph commented Mar 14, 2018

We discussed the textContent vs innerHTHML performance more in dev meeting today. @jessegreenberg @mbarlow12 and I think that the performance difference is not prohibitive. In fact on Safari @mbarlow12 found that innerHTML is actually 37% faster, and firefox had them as equals.

If there are problems with performance in the future, then there are other pieces of the mixin that can (and should eventually) be updated. One of these that was discussed was batching invalidateAccessibleContent calls to once a frame. @mbarlow12 also spoke to Web Assembly as holding answers.

@zepumph zepumph changed the title Rethink cases for setAccessibleContent [Discussion] Rethink cases for setAccessibleContent Mar 14, 2018
@zepumph zepumph mentioned this issue Mar 15, 2018
9 tasks
@zepumph
Copy link
Member

zepumph commented Mar 15, 2018

We finished discussing the way that we want the new A11y API to look in scenery:

  • Remove *AsHTML methods and only use innerHTML settings on DOM elements. Performance considerations in the above comment [Discussion] Rethink cases for setAccessibleContent #686 (comment)

  • Reimplement accessibleLabel: {{string}} so that it only ever set's a sibling DOM element, tag declared in labelTagName. labelTagName should default to label if not specified when accessibleLabel is given.

  • innerContent: sets the innerHTML on tagName DOM element of the Node itself.

  • Reimplement useAriaLabel: {{boolean}} to setAriaLabel: {{string}} which will set the aria label attribute on the Node's DOM Element

  • prependLabels: {{default false}} will be split into two options:

    • `appendLabel: {{default false}}
    • appendDescription: {{ default false}}

    Notice that the default value has switched, and the default now places the label/description before the Node's DOM Element.

  • parentContainerTagName exists if ever there is a label or description DOM Element, defaults to div

  • If parentContainerTagName is given without label/description content, then just surround the single Node's tagName

  • Label and description DOM Elements cannot be children to the Node's DOM element, only siblings.

Closing, to be implemented in #748

@zepumph zepumph closed this as completed Mar 15, 2018
@zepumph
Copy link
Member

zepumph commented Mar 15, 2018

Great work everyone! This was a honker.

@jessegreenberg
Copy link
Contributor Author

Great work @zepumph, thank you for taking the lead on this.

@terracoda
Copy link

@zepumph can Scenery handle two id's for an aria-labelledby attribute yet?

@zepumph
Copy link
Member

zepumph commented May 16, 2018

It cannot, but we have not updated the aria-labelledby or aria-describedby apis yet, please see #701 for that issue. Feel free to add comments and capability suggestions.

@terracoda
Copy link

@zepumph, thanks, I needed the issue number.

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