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

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

Closed
eikawata opened this issue Jul 27, 2018 · 13 comments · Fixed by #5782
Closed

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

eikawata opened this issue Jul 27, 2018 · 13 comments · Fixed by #5782

Comments

@eikawata
Copy link

Bug Report

  • Package version(s): master
  • Browser and OS versions: Any browsers/OS

Priorities and help requested:

Are you willing to submit a PR to fix? No. (But, I might when I have time)

Requested priority: Low

Products/sites affected: N/A

Describe the issue:

The ComboBox header items have role="heading" set, but it doesn't have the aria-level attribute. The Keros tool gives the following error:
Required ARIA attribute not present: aria-level

This seems to be a side effect of this PR#2495

Actual behavior:

The ComboBox header item should have aria-level attribute set.

Expected behavior:

The ComboBox header item doesn't have aria-level attribute set.

@micahgodbolt
Copy link
Member

@jspurlin i'm kinda curious what we're using role=heading at all. Is that actually necessary?

@hoovercj
Copy link
Member

This is an issue in a couple other places as well:

@jspurlin
Copy link
Contributor

Those options are headings, they are not actionable items. According to aria 1.0, aria-level was not a required property: https://www.w3.org/TR/wai-aria-1.0/roles#heading and even in aria 1.1, it's not clear that it's required, note both 1.0 and 1.1state:

If headings are organized into a logical outline, the aria-level attribute is used to indicate the nesting level.

even though aria 1.1 also now has aria-level under "Required States and Properties" for role="heading" where aria 1.0 had it listed under "Supported States and Properties"

(see: https://www.w3.org/TR/wai-aria-1.1/#heading)

I think this is somewhat unclear on the standards part, but sure let's throw an aria-level="1" for role="heading" items...

@metapzl
Copy link

metapzl commented Aug 1, 2018

No, aria-level should not be 1 for a heading that isn't the megaroot heading for the entire webpage. At most 2. Possibly 3 in this control, hard to tell without specifics in mind.

The standards are clear. 1.0 had it as supported, but 1.1 has taken that recommendation a step further by making it required.

Edit: Some advice about not having multiple heading elements with aria-level=1 seem to be outdated. I lack a definitive source from W3C though.

@micahgodbolt
Copy link
Member

seems this is similar to #5481

@jspurlin
Copy link
Contributor

jspurlin commented Aug 2, 2018

Those headings are associated with the menu so aria-level 1 definitely makes sense.

Additionally, it's not valid to have an aria-level that has a higher value than 1 without all preceding ancestors (e.g. if you have an element with aria-level = 3 but it does not have at least one ancestor with aria-level = 2 and another one with aria-level = 1)

@micahgodbolt
Copy link
Member

@jspurlin as a controls library should we assume that some heading has already been set on the page? Or should every control assume it starts at level 1, unless told otherwise?

Options:

  1. as in my PR Added consistent aria levels to any role='heading' #5782 - assume level 1 is only for page level, and always start on 2
  2. Allow users to pass in a starting aria-level, but default to level 2
  3. Allow users to pass in starting aria-level, but default to level 1

@jspurlin
Copy link
Contributor

jspurlin commented Aug 2, 2018

@micahgodbolt no the library should not assume there is another heading on the page.

The aria-level for role="heading" is relative/contextual, in the ContextualMenu case, it is related to the levels of heading in the context of the menu, so the first level should always be marked with level 1. Take for example if someone creates a multi sectioned (e.g. leveled) menu, for example:

  • Fonts
    • Recently Used Fonts
    • All Fonts
      • Common Fonts
      • Symbol Fonts
      • Downloadable Fonts

The aria-level should correspond to the actual level for the heading. If level for Fonts was 2 and it went up from there, a user could be confused looking for a level 1 heading that doesn't exist

It looks like #5782 should be updated to reflect this

@micahgodbolt
Copy link
Member

Okay, so option # 3 then? allow user to provide starting level, and then the component increments from there?

@jspurlin
Copy link
Contributor

jspurlin commented Aug 2, 2018

Yes, # 3 sounds correct

@micahgodbolt
Copy link
Member

prop name suggestion?

startingAriaLevel
ariaLevel
startingLevel
baseHeadingLevel
???

@jspurlin
Copy link
Contributor

jspurlin commented Aug 2, 2018

I'm not opposed to either ariaLevel or startingAriaLevel, either way there should be a comment describing how it's going to be used

@metapzl
Copy link

metapzl commented Aug 2, 2018

Hold on -- I'm beginning to come around to having multiple heading elements with aria-level=1 now that we're in a different HTML5 world, but I thought that only meant "each section can have its own aria-level=1 heading. A menu is not a section on its own, no? Also, what is a heading doing inside a menu?

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants