Make aria-describedby able to reference both the placeholder and the description

[afercia]

Summary

I've read a6af3e1 and the related issue #1214

I'd like to propose an improvement to the way the placeholder and ariaDescribedBy props work. For more details, please see the related issue #1739.

This PR introduces a new _getAriaDescribedByIDs() function to calculate a proper value for the aria-describedby attribute.

• it returns null when placeholder and ariaDescribedBy aren't set, to avoid to render an aria-describedby attribute that points to nothing
• when either placeholder or ariaDescribedBy is set, it just uses its value
• when both placeholder and ariaDescribedBy are set, it uses both values, separated by a space

The third point would allow to replicate the native behavior: when both a placeholder and an aria-describedby pointing to some description are set in a textarea, assistive technologies use both.

If you want, you can verify this on a pen I've prepared: https://codepen.io/afercia/full/GdZOmX/

Screen readers announce first the placeholder, then the description referenced by aria-describedby. This is the expected behavior, since the placeholder and aria-describedby have different purposes and aria-describedby is not an "override" of the placeholder as mentioned in #1214 (comment)
Recommended combos to test:

• Safari+VoiceOver
• Firefox+NVDA
• IE11+JAWS

Specifically, the placeholder is meant to help users with data entry, for example providing a sample value or brief description of the expected format while aria-describedby is meant for a longer description.

Seems to me allowing both is essential to replicate the native behavior.

Worth noting that currently, when the prop ariaDescribedBy is set, the placeholder is not announced at all. As per backwards compatibility concerns, this change shouldn't change so much: everything should work as before, with the added bonus that both props will work.

Test Plan

I've tested using the "plaintext" example:

• remove the placeholder prop and verify no aria-describedby attribute is rendered
• restore the placeholder prop and verify the aria-describedby attribute value is the same of the placeholder ID
• add an ariaDescribedBy with a test ID (you can also add a paragraph with that ID and some content) and verify the aria-describedby attribute value takes both IDs
• play with the two props, remove one of them, re-add, etc. and verify the aria-describedby attribute value is the expected one

Worth noting npm run lint fails for me (also on master, see error below) and npm run flow shows a few errors, also on master. Seems to me they're unrelated errors, unless I'm missing something. In that case I'd appreciate any feedback and guidance 🙂

npm run lint error:

Cannot read property 'type' of undefined
TypeError: Cannot read property 'type' of undefined
    at hasTypeOfOperator (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/rules/no-undef.js:19:19)
    at Program:exit.globalScope.through.forEach.ref (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/rules/no-undef.js:59:44)
    at Array.forEach (<anonymous>)
    at Program:exit (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/rules/no-undef.js:56:37)
    at listeners.(anonymous function).forEach.listener (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/util/safe-emitter.js:47:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/util/safe-emitter.js:47:38)
    at NodeEventGenerator.applySelector (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/util/node-event-generator.js:280:22)
    at NodeEventGenerator.leaveNode (/Users/andrea/Projects/draft-js/node_modules/eslint/lib/util/node-event-generator.js:303:14)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! draft-js@0.10.5 lint: `eslint .`
npm ERR! Exit status 1

[afercia]

Seems to me npm-run-flow fails because of a change made a few days ago, see 8000486 and now entityKey may be of an unexpected type.

Instead, npm run lint fails for me for some problem related to the babel-eslint version. When I upgrade from "babel-eslint": "^7.2.3" to "babel-eslint": "^8.1.1", the error goes away. Maybe something similar to eslint/eslint#9767 (comment) . Not sure why it works on your end. /Cc @flarnie (hello!)

[jessebeach]

Without type safety, I'm concerned that folks might pass in either a string or an Array for this prop value. Is there a way to guarantee a string? Or, if not, you should run this prop through Array.isArray and spread it (or join it) before joining with the placeholder id.

[afercia]

I understand the concern. On the other hand, should this apply also to other props, for example ariaLabelledBy? I couldn't find in the codebase other examples of checking for a prop value this way, and it would look a bit overkill to me. If you strongly feel it's necessary, please do let me know.

[jessebeach]

Rather than making the value nullable, make the property optional. If the property is defined, it must be a string.

[afercia]

Looking better at this, I think it should be nullable. When the placeholder prop is not set, it's necessary to avoid to render the HTML attribute aria-describedby that would point to a non-existing element.

[afercia]

@jessebeach thanks for your review! I'll try to address your points as soon as I have some spare time 😄

On a side note: should the files in meta/bundle-size-stats/ be committed? I've seen they go stale pretty soon 🙂

[niveditc]

On a side note: should the files in meta/bundle-size-stats/ be committed? I've seen they go stale pretty soon

I think we should automate the generation of those files and/or add them to .gitignore. Feel free to exclude them in your commit for now and I'll look into fixing this process.

[niveditc]

@afercia - just wanted to ping this! We'd love to merge this PR when you get a chance to make the changes :)

[afercia]

@niveditc thanks for the ping. About those changes, there are a couple of question I've asked on Sep. 30 that would need some feedback first 🙂Anyways, I'm afraid I won't have much time to dedicate in the next months. Please do feel free to take over this PR, when you have a chance.