Skip to content

ARIA: Fix mergeAriaAttributeValues not producing space delimited ID reference lists#8066

Closed
KevinTCoughlin wants to merge 2 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/fix-merge-aria-attrs
Closed

ARIA: Fix mergeAriaAttributeValues not producing space delimited ID reference lists#8066
KevinTCoughlin wants to merge 2 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/fix-merge-aria-attrs

Conversation

@KevinTCoughlin
Copy link
Copy Markdown
Member

@KevinTCoughlin KevinTCoughlin commented Feb 21, 2019

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

The Problem

Reviewing #8057 brought to my attention that mergeAriaAttributeValues is producing invalid values for aria-describedby when it receives multiple input values. That is because it is concatenating the values without a space which screen reader technology does not understand.

Repro

This Codepen illustrates the problem with Narrator enabled as you focus the two buttons. One of the buttons has its aria-describedby value space-delimited while the other does not. Narrator only reads the space-delimited values.

Narrator Output

Is read by narrator, button,
Description One Description Two, 
Is not read by Narrator, button,

MDN states that aria-describedby and similar attributes must have the value of:

a space-separated list of element IDs

This "space-separated list of element IDs" is referred to as a "ID reference list" in the W3 ARIA spec.

How to resolve

Currently, mergeAriaAttributeValues usage is confined to aria-describedby in a handful of call sites within OUFR. However, it is also publicly exported as part of the API surface.

@JasonGore, I'm happy to patch this, but need your opinion from an API standpoint before proceeding. The options I see are:

  1. We modify the default behavior of mergeAriaAttributeValues as done in this PR currently to space delimit the values. This makes sense to me if there aren't any valid ARIA use-cases for concatenated values without the space.

OR

  1. We offer the ability to provide the delimiter with a default to '' or ' '. And perhaps offer a wrapper function for the aria-describedby use-case.

Focus areas to test

I'm going to let the tests fail while we discuss correct behavior here.

Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

Speaking with @JasonGore, the existing tests need to continue to pass without modification aside from producing space-delimited values foo foo2 foo3 to solve the validation error.

We should also validate that padding the value with spaces is valid or if they should be trimmed.

The input undefined and '' should however return undefined as the aria-describedby attribute is not useful in those cases.

@JasonGore
Copy link
Copy Markdown
Contributor

As long as inputs appending spaces continue to work as expected, then I consider this patch to be functionally compatible and a robustness improvement.

However, the most important thing is that we never return empty strings or nulls, even when given empty strings and nulls as inputs. If there is no alphanumeric content, then undefined must be returned to conform to ARIA standards. The unit tests added as part of #5040 were written to verify this functionality.

*/
export function mergeAriaAttributeValues(...ariaAttributes: (string | undefined)[]): string | undefined {
const mergedAttribute = ariaAttributes.filter((arg: string | undefined) => arg !== undefined && arg !== null).join('');
const mergedAttribute = ariaAttributes.filter((arg: string | undefined) => arg !== undefined && arg !== null).join(' ');
Copy link
Copy Markdown
Contributor

@Dalimil Dalimil Feb 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have .join(' ').trim() maybe?

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/fix-merge-aria-attrs branch January 2, 2020 19:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants