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

Accessibility: working with ARIA labelledby and describedby properties #1116

Closed
comp615 opened this issue Sep 20, 2018 · 14 comments
Closed

Accessibility: working with ARIA labelledby and describedby properties #1116

comp615 opened this issue Sep 20, 2018 · 14 comments

Comments

@comp615
Copy link
Contributor

comp615 commented Sep 20, 2018

Similar to #1006

Is your feature request related to a problem? Please describe.
Since RNW 0.8, many of our accessibility areas are broken as we relied on id references to throw certain labels and references across components.

Describe a solution you'd like
There are many instances when dealing with accessibility that we are required to reference dom ids. A few examples:

  • aria-describedby has no alternative to using dom id (i.e. there is no aria-description)
  • Typeahead or "combobox" elements need to be able to reference the id of the currently active object
  • Often labels may need to span distances across components in certain layouts where there can be a complexity to trying to use aria-label. Consider a general Modal Routing component. It does not care about what's rendered within the modal, but it needs a label that's based on what is rendered inside itself. One option is upwards props (remote-control), but another is to simply have a static domId ("modal-label") that both components agree to use.

Due to this, we'd like to bring back one of id, domId or accessibilityId as options for setting the dom id. These are a necessary side-effect of web development for accessibility. Naming it accessiblityId may help align it with testId and clarify that it's ONLY intended for use as an accessibility prop and not to ruin good component design.

Describe alternatives you've considered
As mentioned above, there are sometimes workarounds. A good example is #1112 which aims to implement the aria describedby prop (dom id only), but to avoid using domIds (and elements) appends it to the label instead. Labels themselves can be specified directly via remote props or different organization of code. Many times though, this requires duplicating logic: whereas we could reference the various pieces of a tweet for a screen reader via describedby to reorder the spoken output, if only labels are employed, it becomes difficult to internationalize and properly mix all the output into a single field.

For many aria tasks, there is no viable alternative: a combobox (typeahead) cannot be created without domIds.

Additional context
Combobox example:
https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html

Thanks! Happy to chat more about this or author a PR.

@necolas
Copy link
Owner

necolas commented Sep 20, 2018

I guess relying on those props is something very recent and you're having problems because of your long upgrade cycles and must have started this while using v0.5. You can use createElement or setNativeProps for now

@necolas
Copy link
Owner

necolas commented Sep 20, 2018

I think most of these cases can be done with better compatibility by using hidden headings, labels, document flow, and presentation role while also avoiding global ID management. Global IDs are fundamentally fragile in a component model. And for cases you absolutely need to do otherwise, you can currently use one of the approaches previously mentioned. I might look at mapping nativeID to DOM but will check with the RN team if this makes sense first.

@comp615
Copy link
Contributor Author

comp615 commented Sep 20, 2018

Honestly, I only found out because a screen-reader users mentioned it to me today, not the type of thing we catch very easily sadly :(

Using aria-label and direct aria props is definitely preferred. However in large applications and the use cases mentioned it's just not always going to be realistic to do it with that alone. For certain aria-tasks it's not just hard, but impossible.

Totally agree with minimizing id usage, that's why I was thinking accessibilityId or something, nativeId sounds appropriate too. Thanks for checking in with them and if you can keep me in the loop on the response and prognosis, that'd be super helpful so I can weigh the options and timing of doing createElement.

@necolas
Copy link
Owner

necolas commented Sep 20, 2018

What did the screen reader users say?

Can you also share an example bit of code. Like, do you have implicit dependencies where one component just happens to know that an ID was set somewhere else in the codebase? That's probably not a pattern that would be accommodated. But if refs were passed around that might make more sense

@comp615
Copy link
Contributor Author

comp615 commented Sep 21, 2018

They mentioned that several areas I believed to be labeled were not reading as labeled which is what started me down the road to researching what happened. Here's a few examples I made (greatly simplified and not real code but should convey the need / idea):

Typeahead / Combobox
Based on: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html and Facebook's implementation.

https://gist.github.com/comp615/3b26330044d8afbee4bf6fec6e69ddc4

Modals (Distant / upward labeling)
This case can be done with just getting labels to the right place, but using certain other packages (react-router) or patterns, it requires upward props to have knowledge of contained props which can be clunky.

You can imagine this as a general wrapper where perhaps the inner workings are a flow where the heading changes at each step...this point is that the modal functionality and container itself does not care or know what's inside of it, so it doesn't really know how to label itself. But...the page itself can then label it's header as well.

https://gist.github.com/comp615/f99cb75d95739056755e583d80ec46f9#file-modalswitch-js-L54

Reorganizing Tweets for screen-readers (also upward labeling)
The weakest example from our code, but a nod to another aria related issue and a use-case where it can help keep code DRY.

https://gist.github.com/comp615/64df1b199fc6746d55ccbfe5c1b01820

@necolas
Copy link
Owner

necolas commented Sep 21, 2018

Is this the kind of thing you're looking to do? LMK if it's suitable for your needs.

import { modalLabelID } from 'web/accessibilityIds';

class ModalLabel extends Component {
  componentDidMount() {
    if (Platform.OS === 'web') {
      this.ref.setNativeProps({ id: modalLabelID });
    }
  }

  render() {
    return (
      <Text ref={(c) => { this.ref = c; }}></Text>
    );
  }
}
import { modalLabelID } from 'web/accessibilityIds';

class Modal extends Component {
  componentDidMount() {
    if (Platform.OS === 'web') {
      this.ref.setNativeProps({ 'aria-describedby': modalLabelID });
    }
  }

  render() {
    return (
      <View ref={(c) => { this.ref = c; }}>
      ...
      </View>
    );
  }
}

I imagine the combobox would need to update IDs in componentDidUpdate for the dynamic changes. I think this works and is a bit simpler than using lifecycles:

import { typeaheadLabelID } from 'web/accessibilityIds';

class TypeaheadDropdown extends Component {
  setListboxAccessibility = (ref) => {
    if (ref && Platform.OS === 'web') {
      ref.setNativeProps({
        'aria-multiselectable': false,
        id: typeaheadLabelID
      });
    }
  }

  setOptionAccessibility = (ref) => {
    if (ref && Platform.OS === 'web') {
      ref.setNativeProps({
        id: this.props.focusedItemDomId
      });
    }
  }

  render() {
    <View
      accessibilityRole="listbox"
      ref={this.setListboxAccessibility}
    >
      {this.props.items.map((item, i) => (
        <View
          accessibilityRole="option"
          children={item.value}
          ref={i === this.props.focusedItemIdx ? this.setOptionAccessibility : undefined}
        />
      )}
    </View>
  }
}

@comp615
Copy link
Contributor Author

comp615 commented Sep 21, 2018

Yeah that will work. CreateElement also works (though differently).

It's not that we can't do it at all right now (as you point out, it's definitely doable since there's still backdoors to set an id) but more that it becomes a matter of doing the ref management or wrapping every time one needs to reference an id. In the third example of the tweet, it means that we need to pass and handle ref callbacks for each element. In my mind that creates a lot of cruft.

At the same time, I understand the desire to have a strict adherence to the react native spec and not allow passing a field that wouldn't apply on other platforms. It's just shame it adds developer and code complexity in some cases. My thought on the balance is that if we want to encourage good accessibility (not just on Twitter but on the web in general), we've got to keep it as easy as possible to accomplish. You can sort of see the nod to this in that all aria-* props are whitelisted for View; there's just some web specific things we can deal with easily that way. The issue is that id is overloaded and can have some nefarious uses too.

Idk, just a hard balance to walk with the whitelisted props!

@necolas
Copy link
Owner

necolas commented Sep 21, 2018

You can also set instance values from the component class and access them in parents using refs this.ref.accessibilityId to avoid importing common modules and avoid doing the ID generation in the parent if that makes more sense.

My thought on the balance is that if we want to encourage good accessibility (not just on Twitter but on the web in general), we've got to keep it as easy as possible to accomplish.

Do need to reorder every part of the Tweet though? Facebook only seems to use one or two elements to label/describe News Feed items.

My thought on the balance is that if we want to encourage good accessibility (not just on Twitter but on the web in general), we've got to keep it as easy as possible to accomplish.

And I think RN/RNW makes it easier to do that by default. Consider how much of the focus details, modality accessibility, and ARIA work is already done for you in the framework and its components. The more intricate ARIA dances are going to be significantly less common and usually end up encapsulated in a component. Given how fragile and error-prone the aria-labelledby and aria-describedby design is (I noticed even Facebook has ARIA pointers to non-existent IDs in their app), it's probably not something you'd want to see being used all over the place.

I'll have a better idea of if / how this can fit into React Native in a few months. For now I think you can get there with setNativeProps in the few places you require this ARIA feature, or roll the dice and maintain a fork.

Thanks for bringing this up and sharing some clear examples of what you're trying to do :)

@necolas necolas changed the title Re-add some method for setting the dom "id" prop for accessiblity Accessibility: working with ARIA labelledby and described by properties Sep 21, 2018
@necolas necolas changed the title Accessibility: working with ARIA labelledby and described by properties Accessibility: working with ARIA labelledby and describedby properties Sep 21, 2018
@comp615
Copy link
Contributor Author

comp615 commented Sep 21, 2018

Yeah some of that example was contrived, but when labelling, you really do need to include everything you want read. It's not even about the ordering so much. (e.g. Facebook doesn't include action counts).

One of the screen reader users taught me something interesting today, Safari/Voiceover will magically read any content in a focused element (great for j/k on Twitter), but it's the only one that does that. Other screen readers require you to manually compose the label from text or elements (Todd did this in old Twitter). That was a big ohhh man more work moment :-P something I'd never known!

100% agree that the existing aria in RNW makes it easier for the average user. It's great! And I know we're the outlier here, it's really complex stuff the average site hopefully never needs to get into. But my hope is the extra effort will help make a world-class experience for screen reader users.

👍 Thanks for looking into this and chat again in a bit! 👋

@necolas
Copy link
Owner

necolas commented Sep 21, 2018

I couldn't find where this is done in old Twitter. Once upon a time, Twitter Lite was using ARIA grid as a hack to achieve this (I guess it worked at some point before browsers updated their code). But the Simply Accessible audit found that it completely broke screenreader access to the timeline content, so we removed it and created the existing experience following their recommendation.

@necolas
Copy link
Owner

necolas commented Oct 10, 2018

Got confirmation from the RN team than nativeID is equivalent to Web's id, so I'll do that mapping and put out another patch release. After that, you should be able to do everything you want without using the imperative approach.

I'll try to get more of the ARIA features baked into the official RN props API after talking with the web and native accessibility teams here in the coming months.

necolas added a commit that referenced this issue Oct 10, 2018
Maps the View prop 'nativeID' to DOM 'id' as these are equivalent.
Enables declarative use of various 'aria-*' properties that require ID
references.

Ref #1116
@comp615
Copy link
Contributor Author

comp615 commented Oct 10, 2018

Awesome. Thanks again for following up on this!

One other thought I had if you wanted to internalize some ARIA/id stuff is to pass ariaControls={this._siblingRef} or a direct element reference, and then internally have it generate ids on sibling and use the id on the current element to hide it. May not work as easily as just the id, but a thought.

necolas added a commit that referenced this issue Oct 12, 2018
Maps the View prop 'nativeID' to DOM 'id' as these are equivalent.
Enables declarative use of various 'aria-*' properties that require ID
references.

Ref #1116
necolas added a commit that referenced this issue Oct 12, 2018
Maps the View and Text prop 'nativeID' to DOM 'id' as these are
equivalent.  Enables declarative use of various 'aria-*' properties that
require ID references.

Ref #1116
Close #1130
@necolas
Copy link
Owner

necolas commented Oct 12, 2018

(Forgot to add nativeID to the supported props filter for View)

@necolas
Copy link
Owner

necolas commented Nov 16, 2018

@comp615 You might be interested in a related RN proposal. Let's move discussion there as it will be where the final APIs are determined and I'd like to make sure they are suitable for web too react-native-community/discussions-and-proposals#56

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

No branches or pull requests

2 participants