ariaDescribedBy prop should render to aria-describedby attribute

[basham]

BUG REPORT (AND POTENTIALLY A FEATURE REQUEST)

The ariaDescribedBy prop should render the provided id string to the aria-describedby attribute.

<Editor
  ariaDescribedBy="editorDescription"
  ariaLabel="Usernames or IDs"/>
<div id="editorDescription">Separate values by a comma or space.</div>

However, as of Draft v0.10.1, it will instead render the placeholder id as the value for aria-describedby and only if the placeholder prop is used.

If retaining the current behavior is desired, then at least the ariaDescribedBy string should be merged with the this._placeholderAccessibilityID string, since the attribute supports multiple id references.

Unfortunately, if the Editor merges these ids on behalf of the developer, the developer has no way to control the ordering of these ids. The order matters, because that's the order in which text is announced by a screen reader. And the order will be different in different contexts.

Bug Fix Recommendation: Merge ariaDescribedBy prop string before the placeholder id, because that's what the developer is explicitly providing.

Feature Recommendation: Consider a future feature in which ariaDescribedBy could be a function that provides the placeholder id parameter and returns a string. Such as:

<Editor
  ariaDescribedBy={placeholderId => `${placeholderId} editorDescription`}
  ariaLabel="Usernames or IDs"
  placeholder="Try copying and pasting values from a spreadsheet."/>
<div id="editorDescription">Separate values by a comma or space.</div>

If ariaDescribedBy is a string, then it renders it as-is, regardless of the presence of the placeholder prop.

<Editor
  ariaDescribedBy="editorDescription"
  ariaLabel="Usernames or IDs"
  placeholder="Try copying and pasting values from a spreadsheet."/>
<div id="editorDescription">Separate values by a comma or space.</div>

[flarnie]

Thanks so much for reporting this. The "Bug Fix Recommendation" would be good to have asap, since it won't affect existing uses of Draft that may be relying on the current behavior.

Would you like to take a try at fixing this?

I'd also be happy to see the feature implemented as you described, and I'm thinking about how to avoid making it a breaking change in behavior. If the default behavior remained the same when no ariaDescribedBy prop was passed, that would be the easiest. It's also kind of a complicated, unexpected API, so I might check with other folks about why it may have been written like this in the first place.

cc @jessebeach who might have more context/advice

[jessebeach]

@basham great catch here.

Merge ariaDescribedBy prop string before the placeholder id, because that's what the developer is explicitly providing.

What about using the ariaDescribedBy prop in lieu of the placeholder id? e.g. (pseudo)

ariaDescribedBy={ariaDescribedBy || this._placeholderId}

I agree that the order matters with the list of IDs. ariaDescribedBy feels like an override of the placeholder, which is a decent fallback.

[basham]

My original feature recommendation was done as a way to give the developer flexibility, fix the bug, and maintain strict backwards compatibility with the current implementation. But in doing so, it creates a clumsy API, and I'm not convinced it would be used or used well.

I'm 100% supportive of the simpler solution @jessebeach proposed:

What about using the ariaDescribedBy prop in lieu of the placeholder id? e.g. (pseudo)

ariaDescribedBy={ariaDescribedBy || this._placeholderId}

That would be a good compromise between the two solutions I proposed. It solves the issue, while still giving backwards compatibility if ariaDescribedBy is not used. I assume this solution would be considered a bug fix, not a new feature?

@flarnie: I'm unsure when I may have the time to create a PR for this. As in, don't wait on me, if someone else wants to take the lead.

[flarnie]

I think I have some folks at FB interested in working on this soon. :)