Polaris-react: [a11y] Spinner state button status not clearly conveyed to screen reader users

[dpersing]

Issue summary

_Updated April 11, 2019_

The spinner state variant of the Button component uses conflicting ARIA roles, resulting in the loading state not being conveyed clearly. ~Users hear "Loading group" (VO/macOS) or "Save Product" (NVDA, VO/iOS), with no clear role.~

In practice, this may vary a bit since typically focus will be on the button when it updates, as well.

When the loading prop is added to the button:

• While using VoiceOver with Safari on macOS, users hear "save product", then "loading" after a delay.
• While using VoiceOver with Chrome on macOS (as a control), users hear no updates, and cannot access the button content.
• While using VoiceOver with Safari on iOS, users hear no update, but can read the "save product" text manually.
• While using the newest version of NVDA (2019.1) with Firefox on Windows 10, users hear "alert unavailable busy save product".

Adding live regions to the page like we're currently doing (either through role="alert", role="status"or aria-live="true") dynamically doesn't work consistently. Screen readers will typically expect the live region to exist on the page on load. MDN has a good explanation of this expected behavior.

~Additionally, the <svg> element inside the button does not have a focusable attribute, which may cause focus issues for IE users if/when the button is not disabled. (This is currently implemented as expected in the Icon component.)~

Current code

Spinner state button (updated HTML output)

<button type="button" class="Polaris-Button Polaris-Button--disabled Polaris-Button--loading" disabled="" role="alert" aria-busy="true">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Spinner">
      <img src="data:image/svg+xml;base64,PHN2ZyB2aWV3Qm94PSIwIDAgMjAgMjAiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PHBhdGggZD0iTTcuMjI5IDEuMTczYTkuMjUgOS4yNSAwIDEgMCAxMS42NTUgMTEuNDEyIDEuMjUgMS4yNSAwIDEgMC0yLjQtLjY5OCA2Ljc1IDYuNzUgMCAxIDEtOC41MDYtOC4zMjkgMS4yNSAxLjI1IDAgMSAwLS43NS0yLjM4NXoiIGZpbGw9IiM5MTlFQUIiLz48L3N2Zz4K" alt="" class="Polaris-Spinner Polaris-Spinner--colorInkLightest Polaris-Spinner--sizeSmall" draggable="false" role="status" aria-label="Loading">
    </span>
    <span class="Polaris-Button__Text">Save product</span>
  </span>
</button>

Spinner state button text CSS

.Polaris-Button--loading, .Polaris-Button--loading.Polaris-Button--disabled, .Polaris-Button--loading:hover {
  color: transparent;
}

Steps to reproduce

1. Navigate to the button component page: https://polaris.shopify.com/components/actions/button#navigation
2. To mimic how the button dynamically updates when the user acts: With a screen reader running, select the Loading state variant of the component and hear how the screen reader conveys the update
3. Use the screen reader reading keys to manually read the button content to hear how it is read

Behavior (tl;dr)

Expected

The loading state will be clearly conveyed to users.

Actual

Screen readers read the previous button text, no update, or a complex status that does not clearly convey the purpose of the update. Users have to manually navigate to the button to hear the content, which is inconsistent.

Recommendations

In practice this is hairy for the aforementioned reasons around how live regions are parsed in the browser. I don't want to _always_ have a live region inside the button, since it shouldn't always be a live region and users who manually read the button content will discover the region, but adding a region dynamically to the page is inconsistent as well. It's not easy for a user to discover what's inside the button anyway at this point, since the button is also made when the loading prop is added.

Another approach would be to use the ARIA progressbar role, but this doesn't get announced automatically like a live region. The screen reader's virtual cursor has to be on the progress bar when it is present to read it.

• Implement the spinner using the ARIA progressbar role. This is semantically more accurate since we're conveying progress, not really updating content dynamically, anyway.
• Use aria-valuetext to convey the state of the spinner, since we don't have a numeric value, like we would for an actual progress bar.

There are also a couple of straightforward things we could do to tidy up some of the output:

• Refrain from using the aria-busy attribute, since it is conveyed inconsistently and may cause the button contents to get skipped entirely in some screen readers.
• Use display: none or visibility: hidden to hide the original button text ("Save product" in the example) when the loading prop is added, so it cannot be read by screen readers when the spinner is displayed. Alternately, remove it from the DOM when the spinner is added.

<button type="button" class="Polaris-Button Polaris-Button--disabled Polaris-Button--loading" disabled="">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Spinner">
      <img src="data:image/svg..." alt="" class="Polaris-Spinner Polaris-Spinner--colorInkLightest Polaris-Spinner--sizeSmall" draggable="false" role="progressbar" aria-valuetext="Loading">
    </span>
    <span class="Polaris-Button__Text">Save product</span>
  </span>
</button>

Specifications

• Component: Button
• WCAG SCs: 4.1.3 Status Messages (Level A)
• Testing done with:
  
  
  
  • VoiceOver/Safari and Chrome/macOS
  
  
  • VoiceOver/Safari/iOS
  
  
  • NVDA/Firefox/Windows 10
  
  

[dpersing]

Migrated from #1570 in polaris-react.

[dpersing]

Moved from polaris-ux for consideration with list filters.

[ry5n]

I like the main recommendation (adjusting ARIA attributes to avoid issues) over the alternative (moving the spinner out of the button). I think the next step here is to implement the main recommendation.

[yourpalsonja]

Hey @ry5n, the Spinner actually uses an svg loaded through the Image component. I'm wondering if you know the reasoning behind this?

It would be nice to replace the Image with an svg, so that we would be able to make it focusable=false. I just don't want to do this w/o understanding if it was built like this for a reason.

[yourpalsonja]

@BPScott What can I do here so this doesn't end up as an Icon, but is still an svg? Is it possible? Can it be done? They say it can't, but I won't believe it until I hear it from you!

[BPScott]

Context for "hey that's not an svg anymore": #1042 changed Spinner to use an <Image> instead of an inline <svg>, because all icons that were in polaris-react now live in polaris-icons, as the spinner svgs aren't actually icons (they're the wrong shape and color) they got moved into the 'images' folder and thus now use our normal image loading style of being an <Image> which uses <img> under the hood. At the same time we removed our icon loaders in polaris-react as we had no more icons left so it's not just a simple case of moving that file back into and 'icons' folder.

At risk of not answering your question: It sounds like the original issue here is "there was an svg that could gain focus because it wasn't set to be focusable='false". Because we now use an <img> which I don't believe is inherently focusable have we sidestepped the original issue around incorrect focus? Why does it need to be an inline <svg> and not an <img>?

Transforming the contents of spinnerLarge and spinnerSmall - which are currently base64 encoded dataURIs - back in to something you can use with dangerouslySetInnerHTML is going to be pretty sucky due to a lack of a common way of doing base64 decoding on client and server. If this absolutely needs to be an inline svg element then our best bet is probably implementing an svgo loader (which is what we were originally planning in #1042 before we realized "hey we have no more icons left, there's not much value in a loader if we have no icons to load").

[dpersing]

I've retested this on macOS, Windows, and iOS in light of the changes in the component, as well as updates to browsers and screen readers, and made some changes to the findings and recommendations.

TL;DR: Because of how ARIA live regions get parsed, putting one inside a button is not straightforward, regardless of what element it's attached to. I've proposed looking into using the progressbar role instead, but that has its own challenges.

[yourpalsonja]

@dpersing what are the follow-up steps I should take here?

[dpersing]

@yourpalsonja I don't have a clear recommendation here at present, unfortunately. There's not a clear solution based on how the button is currently implemented. I've added Needs exploration. Something to pair on?

[dpersing]

Hi @yourpalsonja! Any updates on this issue? Is this something you'd be able to pair on next week?

[yourpalsonja]

Update: We came, we paired, we need to come back and look again. Assigning back to Devon for further research.

Things we did

• Removed aria-busy for now, since we weren't using it for the right reasons.
• Created a branch here

Sandbox code for future testing purposes

import React from 'react';
import {Page, Button} from '../src';


class CoolButton extends React.Component {
  state = {
    isLoading: false,
  };

  render() {
    const {isLoading} = this.state;

    return (
      <Button loading={isLoading} onClick={()=> this.setState({isLoading: true})}>Button?</Button>
    )

  }
}



export function Playground() {
  return (
    <Page title="Playground">
       <CoolButton/>
    </Page>
  );
}

[dpersing]

After a chat with @yourpalsonja before she went a-traveling, I'm on the hook to do some research into how other systems or examples are tackling this problem. Our main goal is to find a way to preserve the button role and disabled state while also conveying the spinner state.

[dpersing]

TL;DR: This is doable if we rework all the ARIA attributes in use, and toggle some states during the loading animation.

A working example

This article from 2017 goes into depth about the issues we've run into with nested statuses (statii?), with a working example: https://accessabilly.com/an-accessible-microinteractions-button/

There are a few critical differences between this example and ours:

• The button in the article solution stays active through part of the interaction, but extra activations are prevented with Javascript, so focus remains on the button through the middle state. Our button goes disabled immediately, which helps prevent multiple submissions more naturally. The button in the example goes disabled at the _end_ though, but the behavior doesn't seem better or worse one way or the other.
• Our version has a role="status" on the button, which overrides the button role. The example here uses aria-live="assertive" instead to do the announcing without changing the role, along with some nested role values on the text snippets. This allows the updates to be read but also maintains the button-ness of the element.
• The button in the article solution has every text string present, and swaps them out by updating attributes, instead of loading them dynamically. This isn't how we load content in Polaris React, which relies instead on dynamically adding content to the DOM.

I found the same results described here for TalkBack in macOS and iOS, where the dynamically updated text is double or triple read, so this solution as-is still isn't ideal.

Forking that solution

Removing the aria-atomic and aria-relevant attributes, along with the roles on the button content snippets seems to fix the duplicate reading.

I also experimented with having aria-live="off" by default and toggling it aria-live="assertive" when the button is activated.

I tried all these things in this fork here: https://codepen.io/dpersing/pen/f88e7ca65884f37c64353b27b02c3224

Test results

I tested the original and my forked solution using (mostly) most-recent versions of:

• VoiceOver on macOS: Original reads but repeats all state changes. Forked version sometimes skips the middle state, but always reads the final state.
• VoiceOver on iOS: Original reads but repeats all state changes. Forked version sometimes repeats the middle state, but always reads the final state as expected.
• TalkBack and Chrome on Android (7.1.1): No state change read for the original, but all states are read in my forked version; we should also test this with a newer version of Android
• NVDA and Firefox on Windows 10: Both work as expected.
• JAWS and Firefox on Windows 10: State updates aren't read (neither the original or my forked version).
• Narrator and Edge on Windows 10: Skips the middle state in both versions, but reads the end/disabled state in both.

The main problem with this implementation is that it requires having aria-live="off" on the button component by default. It feels like overkill, but we don't know when a consumer will end up making a button go into a loading state, and we don't want button content to be read on page load. 🤷‍♀

I think we should do some research into how the loading state _ends_ in the wild. I'm expecting that it usually goes back to the original state, but it might vary by implementation? I've included examples of how I think either would work.

Code examples

Current button

<!-- default button -->

<button type="button" class="Polaris-Button">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Text">Add product</span>
  </span>
</button>

<!-- current loading state -->

<button type="button" class="Polaris-Button Polaris-Button--disabled Polaris-Button--loading" disabled="" role="alert" aria-busy="true">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Spinner">
      <img src="..." alt="" class="Polaris-Spinner Polaris-Spinner--colorInkLightest Polaris-Spinner--sizeSmall" draggable="false" role="status" aria-label="Loading">
    </span>
    <span class="Polaris-Button__Text">Save product</span>
  </span>
</button>

Proposed button

<!--
proposed default button:

- addition of an aria-live="off" attribute
-->

<button type="button" class="Polaris-Button" aria-live="off">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Text">Add product</span>
  </span>
</button>

<!--
proposed loading/middle state, on activation: 

- would need additional JS to fake the disabled state by preventing keyboard/touch/mouse input
- changes aria-live value
- changes styles for loading style
- the image gets an `alt`! (I need to test this, but it should get treated the same way as actual text)
- no roles or additional aria-based labeling on the content
- button text is updated and visually hidden with CSS
-->

<button type="button" class="Polaris-Button Polaris-Button--disabled Polaris-Button--loading" aria-live="assertive">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Spinner">
      <img src="..." alt="" class="Polaris-Spinner Polaris-Spinner--colorInkLightest Polaris-Spinner--sizeSmall" draggable="false">
    </span>
    <span class="Polaris-Button__Text">Loading</span>
  </span>
</button>

<!--
proposed post-loading, disabled state:

- aria-live value stays on assertive
- disabled attribute is added to button
- text is un-visually hidden and swapped out for "saved" state text
- styles are changed for disabled state
-->

<button type="button" class="Polaris-Button Polaris-Button--disabled" disabled aria-live="assertive">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Text">Product saved</span>
  </span>
</button>

<!--
proposed post-loading, enabled state:

- aria-live value stays on assertive
- no disabled attribute is added to button
- text is un-hidden and returned to default
- styles are changed for default state
-->

<button type="button" class="Polaris-Button" aria-live="assertive">
  <span class="Polaris-Button__Content">
    <span class="Polaris-Button__Text">Save product</span>
  </span>
</button>

@yourpalsonja Some food for thought when you return!