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

Add aria-labelledby, aria-describedby and required properties to the search input #254

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 23, 2016

See #21

@ghost ghost changed the title Add aria-labelledby property to the search input Add aria-labelledby, aria-describedby and required properties to the search input Jan 23, 2016
@cibernox
Copy link
Owner

Thanks for caring about a11y. I have very little knowledge on the subject myself.

I have a couple questions about this:

  • In the single select, the search box is not rendered when the dropdown is closed, and therefore you cannot relate the label to it until it's open, which defeats the purpose. Shouldn't the trigger itself be the one with aria-labelledBy and aria-describedBy attributes? Once focused it can be opened with the enter key (or can be automatically opened using the onfocus action)
  • The same changes should be applied to the multiple select.

@cibernox
Copy link
Owner

Another question came to mind right now.

Would you use labelledBy or aria-labelledBy as the public API of the component?

@ghost
Copy link
Author

ghost commented Jan 23, 2016

Yeah you are right, the properties might have to go on the trigger. I'll do some testing and see what works.

@cibernox
Copy link
Owner

I imagine that part of the work has to be done upstream in ember-basic-dropdown, on which ember-power-select relies for everything non directly related with being a select.

@ghost
Copy link
Author

ghost commented Jan 23, 2016

I think so, I was about to checkout the ember-basic-dropdown project to see if that was the case.

@ghost
Copy link
Author

ghost commented Jan 23, 2016

Hmm, the aria-required property only works on real html form-controls like input, select and textarea

@cibernox
Copy link
Owner

Can't aria-required be attached to elements with other aria labels that define them as inputs?

Also, if something cannot be pushed any further, I've always planed to create a {{power-select-native}} component that is just a regular ember-power-select that fallbacks to native select if certain circumstances are met (by example, fallback to native select on iOS/Android, or when it detects a screen reader).
Not all functionality can be expressed in native selects, so a11y in the general component has higher priority to me.

@ghost
Copy link
Author

ghost commented Jan 24, 2016

Found out that adding role="combobox" enables support for aria-required and aria-invalid.

class=(readonly concatenatedClasses)
dir=(readonly dir)
tabindex=(readonly tabindex)
role="combobox"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combobox role at the moment is added to the input where you type to search. I don't know where the correct place is, but having that in both is probably wrong.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be necessary on both, as the trigger and the real combobox are separate pieces of DOM.

@ghost ghost mentioned this pull request Feb 2, 2016
@jmacqueen
Copy link
Contributor

I'm not sure if these observations belong here or in the issue #21...

There are some other concerns to be addressed for accessibility beyond labelling.

For the single-select version, I think role=combobox belongs on the ember-power-select-trigger <div> that receives the tab stop instead of on the search box in the popup.

The role of combobox includes aria-haspopup=true and is expected to hold the final content of the listbox interactions. When using ember-wormhole for placement of the popup, you also need to associate the popup content with the trigger using aria-owns=<id of popup content> since it is placed outside of the regular DOM flow and there is no way for the browser to intuit that they are related. It's probable that the more correct answer is for the triggering <div> to set aria-owns directly to the id of the listbox inside the popup instead of the popup container, but I'm not entirely sure on that one.

The search box inside the popup needs to receive focus on opening (which it currently does) and then, instead of setting the role to combobox, set aria-controls=<id of the listbox> to signify that changes here will affect the contents of the listbox.

In the listbox itself, each option needs to set aria-selected=true/false (which is, conveniently, directly style-able in CSS) and best practice is to set tabindex=0 on the currently highlighted option and tabindex=-1 on the others so that if the box is reopened, you can pick up where you left off.

And I think that will provide all of the functional wiring necessary to make this sucker completely accessible!

@cibernox
Copy link
Owner

cibernox commented Feb 4, 2016

@jmacqueen Thank you so much for the insight! That makes sense, I felt uneasy about the search input having the combobox role. My intuition told me that it wasn't right.
And I haven't considered the fact that the div being attached to the root of the document requires extra aria treatment, now is evident.

If aria-controls and aria-owns need IDs i'll need to expand the components a bit to include IDs generated from the elementID of the parent component, so I can ensure they are unique.

For what I understand aria-selected is for hinting which one is the selected item. Is there an equivalent to high which one is the "highlighted" one?

If those attributes are required I might even consider to drop the ember-power-select--highlighted and ember-power-select--selected CSS classes in favour of the aria roles. I don't need to stress ember to add more attributes than strictly necessary and would also encourage people customizing the components to create accesible components is they want to reuse the default styles.

It might be a bit harsh but it's for a good cause.

@cibernox
Copy link
Owner

cibernox commented Feb 4, 2016

@jmacqueen Also, when it comes to with groups like these http://www.ember-power-select.com/docs/groups what is the proper aria treatment of the hierarchy? Or having the groups being nested <ul> is enough?

@jmacqueen
Copy link
Contributor

Heh. You've gotten ahead of me. I'm still looking through the various setups to make more recommendations 😄

Any time something in the ARIA spec wants you to reference another part of the DOM, it is done by id, so aria-controls and aria-owns will need them! Building off of the containing elementID is the route I highly recommend.

aria-disabled=true is also one to add to the list for your disabled options. I believe it doesn't have to be present at all for the false case.

For hinting the currently highlighted option, aria-current=true was added to the 1.1 spec, so I'd say that's good to use. Before that was added, most people went with setting tabindex on all of the options (0 for the highlighted one, -1 for the others...which should be done anyway) and used the :focus CSS pseudo element handle the styling (since it's in the tab order) and :hover for...well...mouse hovering.

Styling using the ARIA attributes directly is the recommendation from WCAG, so feel free to drop the redundant classes.

I'm fairly certain the nested listbox approach is not correct for the groups. But I didn't have a better recommendation yet so I didn't want to say anything until I'd poked around some more. Right now, I think you probably just need to change the <ul> with role listbox to role group when nested.

@ghost
Copy link
Author

ghost commented Feb 5, 2016

I had some work in progress going, that addresses some of the issues described above, I pushed it in a WIP state, I promise I will continue working on it soon!

@cibernox
Copy link
Owner

cibernox commented Feb 5, 2016

#292 Takes care of using aria-disabled, aria-current and aria-selected for the options.

It also uses the latest version of ember-basic-dropdown from github, that has changed the --disabled class to aria-disabled, but also changed it from the basic-dropdown to the trigger. Therefore that means that now is .ember-power-select-trigger the div that has aria-disabled=true, not the .ember-power-select. I think that it's the correct behaviour because it is also the element with the rest of the aria-*.

Styles have been changed to target those roles. That will force people to create accesible components if they want to have default styles working.

I haven't applied any tabindex to the options yet because I wasn't sure if it could interfere with the current behaviour (focus the next input after the select)

@cibernox
Copy link
Owner

cibernox commented Feb 5, 2016

Just to recap the current state of a11y now, this is the markup of a single select right now:
(I've removed a few irrelevant markup for this topic to remove noise)

<div id="ember477" class="ember-power-select">
  <div id="ember-power-select-trigger-ember476" class="ember-power-select-trigger" 
    aria-haspopup="true" aria-expanded="true" aria-disabled="false" tabindex="0">
    two
    <span class="ember-power-select-status-icon"></span>
  </div>
</div>

<!-- Somewhere else in the DOM -->
<div id="ember-basic-dropdown-wormhole">

  <div class="ember-power-select-dropdown ember-power-select-dropdown-ember476">
    <div class="ember-power-select-search">
      <input type="search" autocomplete="off" autocorrect="off" autocapitalize="off" spellcheck="false" role="combobox">
    </div>
    <ul id="ember595" role="listbox" class="ember-power-select-options">
      <li class="ember-power-select-option" role="option" aria-selected="false" aria-current="true">
        one
      </li>
      <li class="ember-power-select-option" role="option" aria-selected="true" aria-current="false">
        two
      </li>
      <li class="ember-power-select-option" role="option" aria-selected="false" aria-current="false">
        three
      </li>      
    </ul>
  </div>

</div>

from the information shared here I understand that it should be like this:

<div id="ember477" class="ember-power-select">
  <div id="ember-power-select-trigger-ember476" class="ember-power-select-trigger" 
+   role="combobox" aria-owns="ember-power-select-dropdown-ember476"
    aria-haspopup="true" aria-expanded="true" aria-disabled="false" tabindex="0">
    two
    <span class="ember-power-select-status-icon"></span>
  </div>
</div>

<!-- Somewhere else in the DOM -->
<div id="ember-basic-dropdown-wormhole">

  <div class="ember-power-select-dropdown ember-power-select-dropdown-ember476">
    <div class="ember-power-select-search">
-      <input type="search" autocomplete="off" autocorrect="off" autocapitalize="off" spellcheck="false" role="combobox">
+      <input aria-controls="ember-power-select-listbox-ember476" type="search" autocomplete="off" autocorrect="off" autocapitalize="off" spellcheck="false">
    </div>
+    <ul id="ember-power-select-listbox-ember476" role="listbox" class="ember-power-select-options">
      <li class="ember-power-select-option" role="option" aria-selected="false" aria-current="true">
        one
      </li>
      <li class="ember-power-select-option" role="option" aria-selected="true" aria-current="false">
        two
      </li>
      <li class="ember-power-select-option" role="option" aria-selected="false" aria-current="false">
        three
      </li>      
    </ul>
  </div>

</div>

Am I missing something? I also know there is some aria roles for expanded, but perhaps doesn't apply here.

@jmacqueen
Copy link
Contributor

The changes look good, except I think that the combobox needs aria-owns set to the listbox itself, not the popup container.

You are correct, the combobox should definitely have aria-expanded=true/false to reflect the expanded state of the listbox and search area.

It hadn't really registered to me before that you can't tab into the list. You can probably dispense with the tabindex dance, then.

aria-describedby, aria-label, and aria-labelledby should be able to be set on the combobox <div> itself, as that is the element that gets tab focus, and since it's a <div> instead of a text <input>, you can't use <label for='id of the combobox'>.

I still haven't found anything definitive on the nested listbox approach to grouping options. I still suspect that role=group will be the way to go, though. But I'll have to do some testing when I can find a moment.

@cibernox
Copy link
Owner

cibernox commented Feb 5, 2016

The changes look good, except I think that the combobox needs aria-owns set to the listbox itself, not the popup container.

This might be unfortunate, although doable.
I'll explain why.

Ember-power-select is built on top of ember-basic-dropdown. Ember basic dropdown is the component in charge of showing the popup, so I has hoping to be able to manager everything related to the popup functionality in it. As I understand, aria-owns is meant to express relationship between two elements of the DOM that are not contiguous.

I'd expect the trigger to own the whole dropdown box, not just a part (the list of options inside). Can the entire dropdown be the listbox. More accurately the question is: A listbox must contain [role=option]s elements and only [role=option]s or is ok if it contains also the searchbox that controls it?

If that is not forbidden, the entire dropdown can have the role listbox, have a unique ID (it already has a unique class, I would change it to be an ID) and the aria-owns of all dropdowns (not only selects) just points to the floating box.

@jmacqueen
Copy link
Contributor

Holy crap. I just had a major breakthrough!

I've been torturing the spec trying to make this setup work under the role=combobox definitions, but the fact that the main trigger element isn't an editable text element and the presence of the extra search box blow it all up. So I started over. Tell me what you think of this (similar to this example):

<div id="ember477" class="ember-power-select">
  <div id="ember476-trigger" class="ember-power-select-trigger" 
    role="button" aria-pressed="true" aria-owns="ember476-dropdown"   //new stuff
    aria-haspopup="true" aria-expanded="true" aria-disabled="false" tabindex="0">
      two <span class="ember-power-select-status-icon"></span>
  </div>
</div>

<!-- Somewhere else in the DOM -->
<div id="ember-basic-dropdown-wormhole">

  <div class="ember-power-select-dropdown" id="ember476-dropdown">
    <div class="ember-power-select-search">
      <input aria-controls="ember476-listbox" type="search" autocomplete="off" autocorrect="off" autocapitalize="off" spellcheck="false">
    </div>
    <ul id="listbox-ember476" role="listbox" aria-controls="ember476-trigger" class="select-options">
      <li role="option" aria-selected="false" aria-current="true" class="select-option">one</li>
      <li role="option" aria-selected="true" aria-current="false" class="select-option">two</li>
      <li role="option" aria-selected="false" aria-current="false" class="select-option">three</li>      
    </ul>
  </div>

</div>

The main differences are on the trigger element:

  • role=button forget combobox, this should also be good for the basic dropdown component
  • aria-pressed since it has an open and closed state. This can by styled against
  • aria-owns can now be set on the containing <div> inside ember-wormhole

And then aria-controls references on:

  • the search box, pointing to the listbox (like before)
  • the listbox pointing to the trigger (since selection changes the contents of the trigger)

And I did find a reference that suggests using role=group in place of <optgroup>. Which should also be nest-able.

@cibernox cibernox mentioned this pull request Apr 4, 2016
18 tasks
@jmacqueen
Copy link
Contributor

This can probably be closed now since #433 is merged.

@cibernox cibernox closed this Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants