facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.85k stars 46.51k forks source link

Bug: ARIA Attribute Reflection #18821

Closed jonathantneal closed 4 years ago

jonathantneal commented 4 years ago

React version: 16.13.1

Steps To Reproduce

  1. Implement the gov.uk "breadcrumbs" component in React.
  2. Use the ARIA 1.2 ariaCurrent property, as available in Edge 81, Chrome 81, and Safari 13.
  3. See warning:
    Warning: Invalid ARIA attribute `ariaCurrent`. Did you mean `aria-current`?

Link to code example: https://codesandbox.io/s/bold-glitter-lpfpq

function Breadcrumbs() {
  return (
    <ol>
      <li>
        <a href="/">
          Home
        </a>
      </li>
      <li>
        <a href="/passports">
          Passports, travel and living abroad
        </a>
      </li>
      <li ariaCurrent="page">
        Travel abroad
      </li>
    </ol>
  );
}

These properties are helpful reflections that will also be followed by implementations of aria*Elements properties which would eliminate the need for an id by every reference. This is not a bug report on the support of those properties, but they are mentioned to present the higher value these properties will have over dash-less shorthands. I would (naively) expect these (more simple) reflections to worthy of support on their own merits of being implemented web standards.

The current behavior

The ariaCurrent property is not applied, an ariacurrent attribute is applied, and a warning is displayed in development.

 Warning: Invalid ARIA attribute `ariaCurrent`. Did you mean `aria-current`?

The expected behavior

The ariaCurrent property is applied.

TrySound commented 4 years ago

All is correct. Aria attributes should use only kebab case. https://reactjs.org/docs/accessibility.html#wai-aria

jonathantneal commented 4 years ago

Hey, @TrySound! First — off topic — I’m always happy to catch you on GitHub. I hope you’re doing awesome. Okay, back on topic:

Check out some of the links in my PR. In ARIA 1.2, there are now these "reflection" properties, which are not attributes in plain HTML. These properties are in camelCase and are supported in the production releases of Edge, Chrome, and Safari. _(Of course, it’s kind of cheating to list both Edge and Chrome, since they’ll typically maintain parity these days under a shared engine, but I listed them both to remove any hint of this being a proprietary feature.)_ Also, as it differs from the kebab attributes, you should note the casing changes in the ariaHasPopup and ariaReadOnly properties, among others.

While I linked to the appropriate section of the specification, this sub-section may be more immediately helpful — https://www.w3.org/TR/wai-aria-1.2/#reflection.

In the future, these reflection attributes may also possess abilities not possible with HTML attributes — https://w3c.github.io/aria/#reflection — but I’m not asking about those just yet.

Does that help clear up the difference between what I’m asking for and what you’re referring to?

aweary commented 4 years ago

@jonathantneal can you clarify what you're hoping to change in React here? By the definition of reflection the ariaCurrent property is automatically set by the user agent when the aria-current attribute is set. You can see this in the example that you provided.

eps1lon commented 4 years ago

In general having aria attributes as camelCase would help ergonomics regarding destructuring. Now that these are standardized and implemented there's a better justification for using them this way.

Without actually being able to <section ariaLabelledBy={headingRef} /> so that you don't have to deal with ids I think this change makes the way React handles attributes even more confusing.

Disallowing <div aria-labelledby /> would be quite a noisy breaking change with little gain.

jonathantneal commented 4 years ago

@aweary, your link to the reflection definition is spot on to my issue. I’m pretty thankful for that, because it shows me that you know this or you’ve looked into this. Thank you. Thank you.

The spirit of my request is best summarized in this project’s documentation:

React has always provided a JavaScript-centric API to the DOM. Since React components often take both custom and DOM-related props, React uses the camelCase convention just like the DOM APIs: — https://reactjs.org/docs/dom-elements.html#all-supported-html-attributes

Below the attribution on that page, you’ll see a list of DOM attributes supported by React. I’m hoping React will not warn when using the shipped DOM APIs of ARIA.

I would like to illustrate something. @aweary, I hope it’s cool to redirect your question at some other DOM APIs with similar semantics that are currently supported. Instead of ARIA props, what if I were asking about DOM props like controlsList or charSet? Would you clarify what you’d be hoping to change in React by supporting those properties? In a similar fashion, I can point out that the accessKey property is automatically set by the user agent when the accesskey attribute is set.

You may know that ARIA has an unfortunate history of being under-prioritized, so I hope it’s all right that I’m trying to justify my request from multiple angles. To that end, I’d also like to point that the link in that quote also mentions support for contextMenu and radioGroup — at that point doesn’t ARIA deserve a proverbial bone?

I’m super sorry if I was too pushy or rude in any way. It does mean a lot to me that you’ve read my issue. I’d be excited to see this accessibility feature make its way into React, as it has in browsers.

If my request would be more reasonable as a PR, I would also offer my time toward learning how DOM properties are whitelisted and putting effort toward my desired outcome in code.

aweary commented 4 years ago

I’m hoping React will not warn when using the shipped DOM APIs of ARIA.

I don't think we want to support both aria-controls and ariaControls. It's confusing for users when there's more than one correct way to do things. React uses aria-* mostly for historical reasons, and there are good arguments in favor of changing React so that it uses the camel case variants. The big downside is that this would be breaking change. I think it's something the core team wants to do eventually, but there are no plans at the moment that I know of.

Using aria-controls should be functionally equivalent, so hopefully you can just use that for now until the team revisits this!

stale[bot] commented 4 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale[bot] commented 4 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!