Accessibility status changes

[silviuaavram]

Mainly addressing #621 but also the process overall. I had a hunch about what caused Safari not to read the options, and it was the fact that we are using aria-selected wrong (true, many use it this way).

According to Aria, aria-selected marks the currently highlighted option in the list, not the actual selected item (or items if we have multiple choice). Came across this github issue and also the aria spec here, specifically at the Listbox Popup section.

Changes I made, explained.

1. When user navigates through the options, don't update the aria-live message anymore. Will leave the default screen reader behaviour in reading (Black, 1 of 6).
2. Will fix Safari by adding aria-selected="true" to the highlighter element. Bad news is that it will break the implementations that use it incorrectly. Good news is that we can add the highlight style using that class, rather than using the style attribute (not sure if an improvement, but saw this being done many times).
3. Will narrate how many options exist any time their number change. I think it makes sense that the user should be notified about this. Use case: going through the options (so having a highlighted item) and also changing the query string in the input (so the number of options may change).
4. Making these announcements polite. It is not a warning / error message, like 'Form not submitted/Problem with connection', we should not interrupt. For instance, navigating to the dropdown, pressing Down will first narrate the first option, and then will narrate the number of options.
5. Some minor changes related to the actual message. I got these suggestions from the accessibility team I am working with, I think they are a nice touch.
6. Added highlightedItem to the TS types A11yStatusMessageOptions<Item>, as it was missing, and in downshift.js that method is also called with the highlightedItem.
7. Changed the unit tests to match new behaviour.
8. Nit: const status = this.props.getA11yStatusMessage({ was called with highlighted item twice, once with the one from array, then was overridden with the one from state. I removed the first, please take a look and see if it correct.

[kentcdodds]

I love these changes! Thank you for your thorough research and work on this! Just one thing, and then I think this is mergeable 💯

[kentcdodds]

I was unable to try Chrome (it seems that Chrome Canary crashes when using VoiceOver at the moment.... 🤔).

But this is what I got when using Safari. Notice that there's not enough time to read the message before VoiceOver moves on to another message. Could this be due to the assertive->polite change in aria-live?

[silviuaavram]

I was unable to try Chrome (it seems that Chrome Canary crashes when using VoiceOver at the moment.... 🤔).

But this is what I got when using Safari. Notice that there's not enough time to read the message before VoiceOver moves on to another message. Could this be due to the assertive->polite change in aria-live?

That one is true, although on Chrome it works ok. Not sure why Safari does a double reading and interrupting. I will test it some more tomorrow. I really hope that it is not because of the elephant in the room, which I wanted to point out for some time: that Downshift has by default both the <input> and the <ul> inside the <div combobox>. This results in virtual cursor for Jaws not working (it skips the combobox when navigating by virtual cursor).

Aria DOM is saying to have only the <input> in the <div combobox> and then the <ul> to be next to the <div combobox>, at the same level in the DOM. In my component, I worked around this by using the getRootProps() and having the aria DOM order. However at that time we did not support Safari, and for all the other combinations it was working ok.

I will take another look over this issue on the Downshift example and my own component. Will update here. Also will address your first comment. I removed that highlightedItem as I thought getting it from the state in de-structuring was good enough. But will revert it back tomorrow.

Thank you for your feedback and hopefully we will agree on a solution to fix the double reading and to make it work for Safari properly.

[silviuaavram]

also, is that cypress test supposed to fail? not sure I changed anything related to /home/travis/build/paypal/downshift/cypress/screenshots/combobox.js/combobox -- resets when tapping outside on a touch screen (failed).png

[kentcdodds]

Ah, I wasn't aware of the fact that the <ul> needs to be outside the <div combobox>. That should be solvable today via:

<Downshift>
  {({getRootProps, getInputProps, getMenuProps}) => (
    <div>
      <div {...getRootProps()}>
        <input {...getInputProps()} />
      </div>
      <ul {...getMenuProps()}>{/* etc... */}</ul>
    </div>
  )}
</Downshift>

I think that we should probably update the docs to show how to do that, then maybe plan for a breaking change to make that more ergonomic somehow... Not sure how to improve on that though.

If you update the test example to do that though, does that fix this issue?

[kentcdodds]

As for the cypress tests. I don't know for sure. I don't think you changed anything that should make them fail. Do they fail for you locally?

[silviuaavram]

Ok, did some digging. That double reading when writing, which you pointed out, can also be found in the current implementation. I think it's just how Safari works with Voiceover, and by default the Screen Reader does not expect our custom message "x results are available, press ..." so it just goes ahead with that reading. Chrome, for example, only reads the last key pressed, while on Safari it will read the last key pressed, wait a bit, then read the whole text.

Also reverted the aria-live to assertive and maintained the rest of the changes, but still no effect. I've changed it to polite because of the case when user toggles the dropdown with up/down, the aria-live message will take over the default reading of the first/list item. With the message polite, it should first wait for the Black, one of five / clickable, then "Five results are available..." should be read.

[silviuaavram]

Will now take a look on why the cypress test fails. Maybe I've broken something.

Related to the changing of the structure of the DOM, since it's not responsible for the double reading bug you pointed out, we can leave it as a topic for another day.

As for this PR, it will fix the issues I've mentioned in the original comment. I was hoping to get input from the issue reporter, to make sure it fixes his case as well.

[silviuaavram]

The cypress touch test fails for me locally as well. Also fails for me when I remove my changes.

I think it's a test error since I saw another PR with that failure as well. I think it started to fail shortly after #618 got merged.

[kentcdodds]

I've fixed the cypress tests 👍

[kentcdodds]

Rebased your PR onto master to get the fixed cypress tests. You'll need to do a pull to get the latest changes. I hope that's ok.

[kentcdodds]

Hi @dstapleton92, could you please give some input here? Thanks!

[silviuaavram]

Hello and happy new year! Just came back from vacation and noticed this is still on. Should we push it without @dstapleton92? To re-iterate through the changes:

• changed aria-selected to be true when the item is highlighted. Will make Safari read the option now.
• changed getA11yStatusMessage to stop updating on highlighted option change. Will stop the double-reading effect on other browsers (aria-live interrupting the normal screen reader reading).
• changed getA11yStatusMessage to give the results number update each time they change.
• changed getA11yStatusMessage to give the results number update in a 'polite' way so it does not interrupt when the screen reader will narrate an option. Ex: when you open the dropdown with Down Arrow, now it will narrate the first option, and then will inform about the number of options.
• minor change to some aria-live messages, just to make them more clear.
• small type fix for A11yStatusMessageOptions<Item>

I tested the changes again and they work for me, on Safari and Chrome, both with Voiceover.

Voiceover with virtual cursor navigation is still broken in the docs example, but that I think is related to the DOM structure (I pointed out that the combobox should have only the input inside, nothing else, as ARIA suggests). But that is a topic for another time, will come back to this hopefully with a fix.

And talking about the docs, the build fails (not only for my PR) at the cypress task. Locally, it works. Any ideas?

Thank you very much and let me know how can I help further to get these changes in master.
@kentcdodds

[kentcdodds]

Ok, I'm going to merge this as-is, then we can work out the build issues and get this released. I'm really happy with these changes. Thanks!

[kentcdodds]

🎉 This PR is included in version 3.1.9 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[giuseppeg]

hi folks I am investigating a regression in our app which is caused by this patch.

Basically in Chrome items are not announced when navigating the menu items with key up/down. highlightedIndex is updated correctly though.

In the first dropdown I loop through a list of items

<div>
  <button {...getToggleButtonProps()}>toggle</button>
  <div {...getMenuProps()}>
    {items.map((item, key) => 
      <div {...getItemProps({ item })} key={key}>hi</div>
    )}
  </div>
</div>

In the second I call getItemProps manually.

Obviously I have a itemToString.

[silviuaavram]

@giuseppeg basically this PR removed the aria-live narration when you went through the items.

If previously it was reading just the string from the item, without the position (example: 'Red, 1 of 6') , it wasn't working as expected in the first place.

The code above does not give much information for me to help you. I don't know what you use for itemToString or what do you have as items.

I just had a hunch: if you have <ul> and <li> in your DOM instead of those <div> elements, will that help the narration? Try that and let me know. If yes, we need to create an issue and add some aria-role to getMenuProps and getItemProps if the items you use are not semantically accurate.

[giuseppeg]

Hi @silviuavram thanks for your quick reply! I made a testcase https://codesandbox.io/s/z6ok4qjqpp (I am testing with VoiceOver on Chrome)

[silviuaavram]

Try my last suggestion and see what happens. @giuseppeg

[giuseppeg]

I tried that but unfortunately it doesn't seem to work either. It seems that the menu and items get the right role anyway

[silviuaavram]

ooooh now I got it. you are creating a Dropdown that acts as a Single Selection (toggle button and list).

Downshift is a library that provides (for now) default support only for Search Selection (toggle button, edit text and list). If you look at our example, you will see the difference. The screen reader does not narrate anything because there is no <input> to have aria-activedescendant attribute.

That being said, you can still get a desired behaviour for your Dropdown if you manipulate our methods a bit. Take a look here https://stardust-ui.github.io/react/components/dropdown where I am using Downshift to make something similar to what you have. It's the Single Selection example.

You can also search the code and see how Dropdown component is being created. But long story short, you need to add on the menu some object properties from getInputProps(), mostly the aria-activedescendant attribute is important. Take a look at renderItemsList in Dropdown.tsx. And also you need to manually move focus from button to the menu when it is toggled.

We don't have this Single Selection dropdown currently handled by default, maybe we will have it in the future. It's just a different way of handling things. You can see aria specifications for this widget here.

[giuseppeg]

I tried to copy the aria attribute and the event handler over but without luck. I'd appreciate if you could tweak that codesandbox testcase as I think that it might be faster than a written back and forth.

[giuseppeg]

Also just to clarify then previously it worked but by chance since the behavior wasn't spec compliant right?

In the second I call getItemProps manually.

Would a workaround like the one you are suggesting work when I don't have non contiguous items (in different places in the subtree rather than siblings)?

[silviuaavram]

And also you need to manually move focus from button to the menu when it is toggled.

[giuseppeg]

@silviuavram thanks for your the patience and help. I ended up with a much simpler solution https://codesandbox.io/s/z6ok4qjqpp let me know if this is wrong in any way

[giuseppeg]

oh nevermind it is VoiceOver reading the item but the custom message is never read.

[silviuaavram]

@giuseppeg I just tried to provide you with a workaround, as we don't support your scenario. if you want to make it work:

• on toggle button, focus on menu.
• grab the keydown listener from the input and put it on the menu. (for the Up/Down to work)
• grab the activedescendant from the input and put it on the menu.
• on menu close/option select, move focus back to toggle button.

I think these are the exact steps you need to make in order for you to make that scenario work. Good luck!

[giuseppeg]

yup noted. Probably I need to focus trap and prevent blur events from closing the menu too.

Anyway thanks for your patience and help! <3

Would it make sense to clarify that in order to implement a classic drop down menu with Downshift you need to go that extra mile as this behavior is not built-in yet? One might choose Downshift because the docs explicitly mention dropdown.

[juliusv]

I have one question (though keep in mind that the frontend is not my home turf): https://github.com/silviuavram/downshift/blob/bbc52866889f2d8bd84e22027f5138d42d4e09a5/src/set-a11y-status.js#L15 appends arbitrarily many status child divs when one selects the same value again and again (in my case I noticed it by pressing Return a lot in my input). While it would only become a noticeable memory leak after a while, is this the intended behavior or should the history be limited?

[silviuaavram]

@juliusv as far as I know, that function checks if the message div exists, and if it does, it will just replace the message. if it does not exist, it will create one, will reuse it for future message updates, and appends the message.

But if you can reproduce a bug, please open an issue with steps, details, GIFs etc. and we can take it from there :)

[kentcdodds]

The reason it works the way it does is that if the same message needs to be read to the user twice you have to add a brand new div, otherwise it wont be read (because the message is unchanged). And no, I tried and removing the old one and adding a new one with the same message doesn't work.

I'm guessing that we probably could remove a few after we hit a threshold or something, but the chances that this div fills up with any degree of significant number of divs is highly unlikely. Considering they're not even rendered by the browser (so no layout effects), we could probably render hundreds of them and I doubt it'd ever get to more than 20 in a normal scenario, so it just doesn't seem like it's worth the effort (or code) to optimize.

[juliusv]

@kentcdodds Yeah, you're probably right. The old ones at least get display: none.

I'm using this for an input field where you enter a time series database query and then the results get shown below. If I evaluate the same database query again and again to see how the time series evolve, I get plenty of these divs, but probably still nothing to really worry about for now.

[silviuaavram]

we can probably work around with a setTimeout if we really need to. it will probably be ugly. but if you want you can still create an issue maybe someone will pick it up and come up with a desirable fix. :)