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

Fix NVDA reading blank on Select2 multiple option #5571

Closed
wants to merge 1 commit into from
Closed

Fix NVDA reading blank on Select2 multiple option #5571

wants to merge 1 commit into from

Conversation

stevenbriscoeca
Copy link

@stevenbriscoeca stevenbriscoeca commented Jul 17, 2019

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

  • add unique id on ul
  • add aria-labelleby on input that is pointing to parent ul

If this is related to an existing ticket, include a link to it as well.
#4418

@kevin-brown
Copy link
Member

Alright, now that I understand what we are doing here, I can give some recommendations.

We are going to want to assign the aria-labelledby attribute when binding the Search decorator for the selection. This is the decorator where the search field on a multiple select is injected, and it's only used for that. Note that it is possible for multiple selects to exist without a search field, so we don't like to assume that one exists.

Search.prototype.bind = function (decorated, container, $container) {

We can predict what the ID for the results should be, since we calculate it already for single selects, and we should calculate it the same way for multiple selects. Note that we use container.id as the prefix so we have a consistent ID prefix for all of our elements, which makes it easier for automated tests to verify the state of Select2.

var id = container.id + '-container';
this.$selection.find('.select2-selection__rendered')
.attr('id', id)

We want to set the ID on the $search element, which is the input that is used for searching and is the one always injected into the multiple select.

this.$search.attr('aria-labelledby', id);

The other useful thing to have would be tests, so we know that this is being consistently assigned. And so when we come in later and wonder why we did this, we will have a test fail when we go to remove it. We already have a few basic ones at

https://github.com/select2/select2/blob/d66f8716197567fb95a680211f3c32e9c94f6679/tests/a11y/search-tests.js

@stevenbriscoeca
Copy link
Author

Hi @kevin-brown ,

Thank you so much for the detailed feedback, i'll try to put hours in next week to get this done!

@kevin-brown kevin-brown added this to the 4.0.9 milestone Jul 21, 2019
@afercia
Copy link

afercia commented Jul 25, 2019

Sorry I don't want to sound dismissing and I appreciate the effort made here but this approach based on aria-labelledby doesn't seem ideal to me.

aria-labelledby is a replacement for the <label> element. Changing the aria-labelledby value actually changes the name of the select, which is far from ideal.

Instead, the UI pattern fo follow is the combobox one described in the ARIA Authoring Practices:

https://www.w3.org/TR/wai-aria-practices-1.1/#combobox

There are a few examples there, here's one of them:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/combobox/aria1.1pattern/listbox-combo.html

They're based on aria-activedescendant and aria-selected.

Previously:
#4349
#4350
#3735

See also: Making Select2 accessible, a possible plan
https://github.com/wpaccessibility/select2-a11y/issues/1

@stevenbriscoeca
Copy link
Author

Thank you for the feedback, I have no trouble changing the approach. The goal here is advancing this issue with the best possible solution.

The examples you give only have one option selected, what would be the solution when focused in the input with multiple items selected like this :

image

@afercia
Copy link

afercia commented Jul 25, 2019

There's no established UI pattern for that in the ARIA Authoring Practices, nor anything applicable in the ARIA spec I can think of. The closest one is Example 2 here: https://www.w3.org/TR/wai-aria-practices-1.1/examples/listbox/listbox-rearrangeable.html which is closer to a<select> with the mutiple attribute.

Though those "tags" are a common design in select-replacement libraries, they aren't exactly a "native" UI or something close to a native UI. Also, those "tags" are actually outside of the input field. They represent the list of selected options so maybe I'd just use a list, preferably with some text to explain what the list is about.

@stevenbriscoeca
Copy link
Author

@afercia It is already a list :
image

How would you link the list and input when focused together?

@afercia
Copy link

afercia commented Jul 26, 2019

Yeah but what I see in the multi-select boxes example, is that also the input field is within a list item:

Screenshot 2019-07-26 at 08 47 47

That defeats the purpose of using a list 🙂What the current list is actually communicating? Semantically (and assistive technologies will get this info) it's saying:

  • hey, this is a list with 3 items
  • Alaska, 1 of 3
  • Minnesota, 2 of 3
  • edit text (unlabelled input field), 3 of 3

How that can be helpful for users?

Additionally:

  • the title attributes e.g. title="Alaska" are redundant: there's already visible text with the same value
  • the "times" character is announced by screen readers, as role="presentation" doesn't hide content from assistive technologies. It just removes semantics (if any). This: <span class="select2-selection__choice__remove" role="presentation">×</span> is meaningless.

Re: a way to "link" the selected options to the input field; I'm not sure there's a good way to do it in a way that can be easily understood by assistive technologies users. I'd just use plain text. Maybe visually hidden text.

@stevenbriscoeca
Copy link
Author

Alright so from what I understand (correct me if i'm wrong)

  • Remove input from list
  • Remove title attributes from list items
  • Add alternative text from list above input in hidden text (so it would be label, arrow down, then the alternative text (so we put aria-hidden on current list?)
  • Add aria-hidden on the times character span, when the user does backspace he can erase it anyway

anything else to add to this list @afercia ?

@afercia
Copy link

afercia commented Jul 31, 2019

Add alternative text from list above input in hidden text (so it would be label, arrow down, then the alternative text (so we put aria-hidden on current list?)

Not sure I understand this part 🙂 What I meant is, in pseudo-code:

<p class="visuallyhidden">Currently selected options:</p>
<ul>
  <li>Alaska</li>
  <li>Minnesota</li>
</ul>

@afercia
Copy link

afercia commented Jul 31, 2019

Add aria-hidden on the times character span, when the user does backspace he can erase it anyway

For what is worth, in the new WordPress block editor (Gutenberg) the "X" icons are actually buttons so they're focusable and operable with a keyboard.

Screenshot 2019-07-31 at 14 35 33

Also, they have an aria-label and an aria-describedby attributes to properly label the button and give more context. Simplified code:

<span id="components-form-token-field__token-text-7">
	<span class="screen-reader-text">claycold (1 of 2)</span>
	<span aria-hidden="true">claycold</span>
</span>
<button type="button" aria-label="Remove Tag" aria-describedby="components-form-token-field__token-text-7"></button>

This way, the button is announced as "Remove Tag {brief pause} claycold (1 of 2)".

I do realize this is a bit out of the scope of this pull though 🙂

@stevenbriscoeca
Copy link
Author

@afercia would be possible for you to add issues of all the things you mentions that are out of scope of this issue? Because all this great info will be lost and in this PR I don't think we will do every accessibility issue that you list cc @kevin-brown

I understand for the text before the list, but how do we link a focus into the input to the list? Because that visually hidden text will only be accessible if you navigate with the arrow up and arrow down in nvda and jaws. If someone navigates through labels and forms only they will miss this info

@afercia
Copy link

afercia commented Jul 31, 2019

One option could be adding an aria-describedby attribute on the input field, pointing to a hidden element that contains text with info on the selected options. This element can be hidden with display: none and aria-describedby should work regardless.

Something like:

<input aria-describedby="selected-options-description" />

<span id="selected-options-description" class="hidden-with-display-none">No options selected</span>

or:

<span id="selected-options-description" class="hidden-with-display-none">Currently selected options: Alaska, Minnesota</span>

It could be a bit redundant but it's better than a resounding silence.

would be possible for you to add issues of all the things you mentions that are out of scope of this issue? Because all this great info will be lost

Thanks, yep I do realize there's the risk some feedback will be lost. However, it took me 5 days to reply to the previous comment. I'm afraid I'm a bit busy 🙂

@stevenbriscoeca
Copy link
Author

That's a great solution thank you so to we will go with your last solution.

@kevin-brown can you give the green light so we can start working on this please?

@afercia are you okay with me creating the issues, i'll reference to your comments in each issue

@afercia
Copy link

afercia commented Jul 31, 2019

@StephenBe I'm super okay, thanks!

@stevenbriscoeca
Copy link
Author

stevenbriscoeca commented Jul 31, 2019

@afercia because there is already a list there I suggest we do :

<span id="selected-options-description" class="hidden-with-display-none">Currently selected options:</span>
<ul>
  <li> Alaska</li>
<li>Minnesota</li>
</ul>
<input aria-describedby="selected-options-description" />

@stevenbriscoeca
Copy link
Author

I nevermind that won't work because the user would need to do the arrow to access the list

@afercia
Copy link

afercia commented Jul 31, 2019

I wouldn't be too worried for that. With screen readers, the main mode to explore content is by using arrows. The tab key is used mainly within forms or when users discover a quick path to get where they want (e.g. jump to second heading, then press right arrow once, then press tab twice).

@stevenbriscoeca
Copy link
Author

stevenbriscoeca commented Jul 31, 2019

I understand, but in this case the user will be focused in the input so even an arrow down or up won't help if I remember the aria-describedby correctly.

@afercia
Copy link

afercia commented Jul 31, 2019

Not sure I understand 🙂

  • in "browse mode" (arrow keys navigation): the info is available in the list and I'd tend to think some content exploration to get this info would be acceptable for users
  • in "forms mode" (tab key navigation): the info will be automatically provided by aria-describedby when the input gets focus

@stevenbriscoeca
Copy link
Author

stevenbriscoeca commented Jul 31, 2019

well the id that is linked with the aria-describedby is only on the span not the list, but actually we could wrap the span and list in a div with the proper id like this :

<div  id="selected-options-description">
  <span class="hidden-with-display-none">Currently selected options:</span>
  <ul>
    <li> Alaska</li>
    <li>Minnesota</li>
  </ul>
</div>

@afercia
Copy link

afercia commented Jul 31, 2019

Well, aria-describedby needs to be set on the input field:

<input class="select2-search__field" type="search" role="textbox" aria-describedby="selected-options-description" ... />

<span id="selected-options-description" class="hidden-with-display-none">Currently selected options:</span>

As per using a list wrapper as the target for aria-describedby: though the referenced element's content is technically flattened to a string, personally I'd rather reference an element which has the sole responsibility of providing the description.

@stevenbriscoeca
Copy link
Author

stevenbriscoeca commented Jul 31, 2019

yeah I miswrote, I edited it out to an id.

Ok correct me if i'm wrong, but the problem I see with having a span with the id and the list underneath is this :

If I focus on the input, the aria-describedby will look for an id and find the span and read :

Currently selected options:

The list will not be called out and you wont have access to it because you are currently focused on the input. Is that correct?

@afercia
Copy link

afercia commented Jul 31, 2019

Ahh yes, sorry I mis-coded the pseudo-code 🙂 I'm assuming there will be some code responsible to put the actual selected values within the hidden span, as in one of the previous examples see #5571 (comment)

<span id="selected-options-description" class="hidden-with-display-none">
    Currently selected options: Alaska, Minnesota
</span>

or, when no option is selected:

<span id="selected-options-description" class="hidden-with-display-none">
    No options selected
</span>

@stevenbriscoeca
Copy link
Author

Alright great we will go with that once @kevin-brown gives the green light. Thanks for your valuable feedback, very much appreciated!

@kevin-brown
Copy link
Member

kevin-brown commented Aug 4, 2019

Sorry for the late response here, I've been following the conversation here but wanted to write up a longer response and have decided that I should throw out that idea.

@kevin-brown can you give the green light so we can start working on this please?

Go for it. I will gladly review the new/updated pull request.

Some things to note along the way:

  • Make sure any new hard-coded text is added to the English translation and uses that mechanism. This is important because Select2 supports 60+ languages.
  • I would recommend using the title data (which is currently injected onto the selection) when flattening the text out for display. The actual text might be templated and look odd, but the title is supposed to be the standard for text-only display.

For what is worth, in the new WordPress block editor (Gutenberg) the "X" icons are actually buttons so they're focusable and operable with a keyboard.

This is something I've been looking at, since it's an area where Select2 is very much so not accessible at the moment. The only way to remove selected items in a multi-select using your keyboard is to make them appear in the results again and deselect them there. If someone wants to write up a quick ticket (doesn't have to be super detailed, but link back to here), this is an area I would love to improve on for 4.1.0.

@Hosch250
Copy link

Not a review, but I just want to say THANKS for taking care of this.

@kevin-brown kevin-brown modified the milestones: 4.0.10, 4.0.11 Aug 28, 2019
@kevin-brown kevin-brown modified the milestones: 4.0.11, 4.0.12 Oct 13, 2019
@kevin-brown kevin-brown modified the milestones: 4.0.12, 4.0.13 Nov 6, 2019
@jensinefayedabeast
Copy link

@kevin-brown, @StephenBe, and @afercia, thank you for all of the time, thought and effort that you have put into this.

@dotherightthing
Copy link

I found this pull request after reproducing the 'times' role="presentation" clear button issue in VoiceOver.

@afercia 26 July

the "times" character is announced by screen readers, as role="presentation" doesn't hide content from assistive technologies. It just removes semantics (if any). This: × is meaningless.

@StephenBe 27 July

  • Add aria-hidden on the times character span, when the user does backspace he can erase it anyway

This is one of many accessibility issues discussed in the comment thread. However the pull request only addresses one specific issue.

Are there existing pull requests addressing the other issues, or do you have a plan for tackling and testing them en masse, or should I log individual issues so they can be addressed individually? Thanks.

@kevin-brown
Copy link
Member

I have opened a pull request at #5842 which aims to redo the accessibility of the selection area with multiple and single selects to make them more accessible to screen readers. One of the main changes involves changing the selection to ensure the selected items are read out to the user when the search and other areas are focused.

The pull request includes a list of notable changes, but to be honest the majority of the changes we are putting into place were discussed throughout the years on this pull request as well as many related accessibility tickets.

While I understand that it has been a while, it would be greatly appreciated if anyone could help us test out these changes to ensure this ticket has been fixed. You can test these on your own by recompiling Select2 from the lastest develop branch, or you can wait for the next beta release (which should be out once that pull request is merged). Our goal is to significantly improve the accessibility in time for the upcoming 4.1.0 release of Select2.

@Hosch250
Copy link

Thank you for your work! I've been holding off selecting a multiselect control in a project I'm developing due to this issue; I'm happy to help once I can get it off a CDN.

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

Successfully merging this pull request may close these issues.

None yet

6 participants