Added consistent aria levels to any role='heading' #5782

micahgodbolt · 2018-08-02T18:33:31Z

Pull request checklist

Addresses an existing issue: Fixes ComboBox: Header item doesn't have aria-level set #5706 Fixes Dialog[Accessibility]: Required ARIA Attribute "aria-level" not present #5481
Include a change request file using $ npm run change

Description of changes

As per conversation, level 1 is reserved for page level headings (which our components will rarely be). So I've set everything at a default of 2, unless they are items inside of a component that already has a level 2, in which case it changes to level 3.

This switching behavior was already in contextual menu, so I made combobox consistent.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

micahgodbolt · 2018-08-02T18:35:31Z

@ekawatani @hoovercj @jspurlin @philipzloh @cschlechty

I found all of the role="heading" in fabric and tried to come up with a solution based on your feedback. Tell me if the above guidance appears correct.

jspurlin · 2018-08-02T19:15:25Z

Didn't mean to close the PR... The guidance is not correct in terms of an application or "widgets". aria-level can be relative/contextual, for example: menu, treeview, or tree. The current guidance seems to be thinking to literally about the h1 through h6 html element for traditional web pages

micahgodbolt · 2018-08-02T19:17:07Z

can/should we add aria-level to our basecomponent? So it could be passed in to all components (and obviously only used by the ones that need it).

micahgodbolt · 2018-08-02T19:17:39Z

nah....that'd infer that aria-level would be placed onto the root...no good

jspurlin · 2018-08-02T19:19:23Z

also, aria-level can be contextual so basing it off of a parent might not make sense

cliffkoh · 2018-08-02T21:40:54Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

@@ -1119,7 +1121,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
    const { onRenderOption = this._onRenderOptionContent } = this.props;

    return (
-      <div key={item.key} className={this._classNames.header} role="heading">
+      <div key={item.key} className={this._classNames.header} aria-level={this.props.label ? 3 : 2} role="heading">


this.props.label ? 3 : 2 [](start = 74, length = 24)

Please do leave a comment as to why label informs 3/2 here. To someone without the context that you have, this can look very arbitrary.

cschlechty · 2018-08-02T21:54:56Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

@@ -344,6 +344,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
            required={required}
            htmlFor={id + '-input'}
            className={this._classNames.label}
+            aria-level={2}
+            role="heading"


I doubt we'd want headings built in to combo boxes or other simple controls. Headings are mean to provide hierarchical page structure.

micahgodbolt · 2018-08-02T22:09:45Z

So what are the places that actually need role="heading" and therefore aria-level? Is it just panel and dialog right now? And if so, can we simply go with a default value of 2, or do we need to leave that open for customization?

@cschlechty ?

cschlechty · 2018-08-02T22:19:53Z

Having those as level 2 seems reasonable as long as we don't put them as descendants of things level 2 or lower.

…nd panel with aria-level 2

eikawata · 2018-08-03T07:28:34Z

If aria-level is contextual, then the default value for these heading elements in popup boxes like ContexualMenu, and Panel should be 1 as this comment mentions. If it's set to 2, user will look for heading level 1 that doesn't exist.

We should open it up for customizing the aria-level if the component provides a way to custom-render parts of the component. For example, the Panel has a onRenderHeader prop for custom-rendering the header, and people may be injecting several headings in there.

cschlechty · 2018-08-03T15:19:29Z

No. There should be only one heading of level 1 on a page. The levels are contextual in that it should make sense hierarchically, under h2s, you have h3s, etc. Where the controls are in the DOM structure will partially determine the level, so it needs some level of customization.

hoovercj · 2018-08-03T17:51:11Z

I'm a bit confused. For dialogs and such that appear overlaid over the content, with their root element perhaps being outside the main h1 of the page (last child of the body tag, for example, with the h1 being a few divs deep in the body), what is the expected heading level of the dialog heading?

Hierarchically in the dom it would be 1, but semantically on the page, perhaps 2? What does the spec suggest here?

And then consider a form on a page. Page header is level 1, form header is level 2, then if you start rendering menu items in the form, would their headings not be level 3+?

But what if, again, these menu items actually get rendered elsewhere in the dom but are "owned" by the form? In the dom they may not have a parent with a heading level. Does the spec give guidance here?

metapzl · 2018-08-03T20:34:17Z

As in my other comment, I don't really understand or see the correctness in having a heading inside a menu.

The point is not that there can only be one thing that is aria-level=1 onscreen. The argument that @cschlechty is making (that is supported by a lot of other developers in the accessibility space) is that there should only be one heading of aria-level=1 on a page. The argument that others are bringing up is that in HTML5, as we've evolved our understanding of a webpage (not just a document), having a heading of aria-level=1 for each different section may make sense. I can see that reason. However, I remain unconvinced that a menu is a different section, nor should it contain a heading.

jspurlin · 2018-08-03T20:34:55Z

It's technically incorrect to have an aria-level > 1 without all of the previous levels accounted for in the same section (in your example, a dialog)

jspurlin · 2018-08-03T20:37:49Z

@philipzloh these are heading in a menu, what else would you call them?

cschlechty · 2018-08-03T21:06:50Z

You probably just want that to be the label for a group of options. Using options, that'd be an optgroup. With custom markup, it's probably just a vanilla role="group". see https://www.w3.org/WAI/tutorials/forms/grouping/

…

On Fri, Aug 3, 2018 at 1:37 PM, jspurlin ***@***.***> wrote: @philipzloh <https://github.com/philipzloh> these are heading in a menu, what else would you call them? [image: image] <https://user-images.githubusercontent.com/7033561/43664607-5720ac4c-9722-11e8-85e6-325cb6592448.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5782 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATa0j8TzPRNKzevGTHw4FCp_1GFimbsWks5uNLSigaJpZM4Vsy6e> .

metapzl · 2018-08-03T21:07:28Z

@jspurlin If I were super pedantic, that's incorrect / non-standard usage of menu? I understand that it shows up in our menus, but in a standard menu it should have been represented as:

Theme fonts
- Arial Black
- Times New Roman
- Comic Sans MS
Other Options
- Option d
- Option e

In our case, we should probably represent this in pseudo-DOM as

<menu aria-label="font">
    <group aria-labelledby="themeid">
        <span id="themeid" aria-hidden="true">Theme Fonts</span>
        <menuitemradio>Arial Black</menuitemradio>
        <menuitemradio>Times New Roman</menuitemradio>
        <menuitemradio>Comic Sans MS</menuitemradio>
    </group>
    <group aria-labelledby="otherid">
        <span id="otherid" aria-hidden="true">Other Options</span>
        <menuitemradio>Option d</menuitemradio>
        <menuitemradio>Option e</menuitemradio>
    </group>
</menu>

jspurlin · 2018-08-03T21:15:20Z

@philipzloh and @cschlechty yep, that's an alternative approach to fix this and would actually result in a better readout

micahgodbolt · 2018-08-07T07:02:10Z

Is there a final approach we all agree on? Is this PR good as is, or do we need to support custom aria-level values for these two surfaces.

And if there is further work to be done for the ComboBox and contextualMenu, can we put that into a separate issue?

micahgodbolt · 2018-08-08T16:56:08Z

Let me toss out resolution for this PR.

Remove aria-role="heading" from ComboBox and ContextualMenu. Sounds like there's a better approach suggested for improving those.
aria-level needs to be customizable in the case that there is no other level 1 on the page. So I'll add props for panel and dialog to set that level.
merge this PR :D

jspurlin · 2018-08-08T18:09:28Z

We should only remove the role=heading if we are also doing the aria-labelledby group (with aria-hidden on the actual heading element) approach. Otherwise if a use is consuming the menu with virtual focus it will announce those elements a " group" which is worse than it currently is.

cschlechty

We should eventually add menu sections per W3C, but this is a solid improvement.

micahgodbolt requested a review from joschect as a code owner August 2, 2018 18:37

micahgodbolt mentioned this pull request Aug 2, 2018

ComboBox: Header item doesn't have aria-level set #5706

Closed

jspurlin closed this Aug 2, 2018

jspurlin reopened this Aug 2, 2018

cliffkoh reviewed Aug 2, 2018

View reviewed changes

cschlechty reviewed Aug 2, 2018

View reviewed changes

micahgodbolt added 2 commits August 2, 2018 15:31

Removed heading role from combo and contextualmenus, updated dialog a…

fd3be68

…nd panel with aria-level 2

Changefile

22585ef

joschect approved these changes Aug 9, 2018

View reviewed changes

cschlechty approved these changes Aug 9, 2018

View reviewed changes

dzearing merged commit d5129e5 into microsoft:master Aug 9, 2018

micahgodbolt deleted the update-aria-level branch August 9, 2018 21:39

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added consistent aria levels to any role='heading' #5782

Added consistent aria levels to any role='heading' #5782

micahgodbolt commented Aug 2, 2018 •

edited by fabricteam

Loading

micahgodbolt commented Aug 2, 2018

jspurlin commented Aug 2, 2018

micahgodbolt commented Aug 2, 2018

micahgodbolt commented Aug 2, 2018

jspurlin commented Aug 2, 2018

cliffkoh Aug 2, 2018

cschlechty Aug 2, 2018

micahgodbolt commented Aug 2, 2018

cschlechty commented Aug 2, 2018

eikawata commented Aug 3, 2018 •

edited

Loading

cschlechty commented Aug 3, 2018

hoovercj commented Aug 3, 2018 •

edited

Loading

metapzl commented Aug 3, 2018

jspurlin commented Aug 3, 2018

jspurlin commented Aug 3, 2018

cschlechty commented Aug 3, 2018 via email

metapzl commented Aug 3, 2018 •

edited

Loading

jspurlin commented Aug 3, 2018

micahgodbolt commented Aug 7, 2018

micahgodbolt commented Aug 8, 2018

jspurlin commented Aug 8, 2018 •

edited

Loading

cschlechty left a comment

Added consistent aria levels to any role='heading' #5782

Added consistent aria levels to any role='heading' #5782

Conversation

micahgodbolt commented Aug 2, 2018 • edited by fabricteam Loading

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

micahgodbolt commented Aug 2, 2018

jspurlin commented Aug 2, 2018

micahgodbolt commented Aug 2, 2018

micahgodbolt commented Aug 2, 2018

jspurlin commented Aug 2, 2018

cliffkoh Aug 2, 2018

Choose a reason for hiding this comment

cschlechty Aug 2, 2018

Choose a reason for hiding this comment

micahgodbolt commented Aug 2, 2018

cschlechty commented Aug 2, 2018

eikawata commented Aug 3, 2018 • edited Loading

cschlechty commented Aug 3, 2018

hoovercj commented Aug 3, 2018 • edited Loading

metapzl commented Aug 3, 2018

jspurlin commented Aug 3, 2018

jspurlin commented Aug 3, 2018

cschlechty commented Aug 3, 2018 via email

metapzl commented Aug 3, 2018 • edited Loading

jspurlin commented Aug 3, 2018

micahgodbolt commented Aug 7, 2018

micahgodbolt commented Aug 8, 2018

jspurlin commented Aug 8, 2018 • edited Loading

cschlechty left a comment

Choose a reason for hiding this comment

micahgodbolt commented Aug 2, 2018 •

edited by fabricteam

Loading

eikawata commented Aug 3, 2018 •

edited

Loading

hoovercj commented Aug 3, 2018 •

edited

Loading

metapzl commented Aug 3, 2018 •

edited

Loading

jspurlin commented Aug 8, 2018 •

edited

Loading