Skip to content

Fix invalid mergeAriaAttributeValues output when multiple ID references supplied#8113

Merged
msft-github-bot merged 4 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/space-delimit-aria-attrs
Feb 26, 2019
Merged

Fix invalid mergeAriaAttributeValues output when multiple ID references supplied#8113
msft-github-bot merged 4 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/space-delimit-aria-attrs

Conversation

@KevinTCoughlin
Copy link
Copy Markdown
Member

@KevinTCoughlin KevinTCoughlin commented Feb 25, 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 unless the consumer knows to supply the space padding themselves.

Currently, the helper is concatenating the values without a space which screen reader technology does not understand.

Example: The output elementId1elementId2 should be elementId1 elementId2 where each string refers to the ID attribute of an element in the DOM.

I believe that the helper should space-delimit the output by default to minimize the risk of introducing ARIA bugs and/or regressions. Currently, the helper is only used for aria-describedby values within OUFR.

I'm unaware of an a11y scenario where we would want the output values concatenated without a space.

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.

Focus areas to test

  • Tests should pass
  • Components which use this helper and supply multiple values such as with KeyTip should now be correctly read by the screen-reader.
Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link
Copy Markdown

size-auditor bot commented Feb 25, 2019

Bundle test Size (minified) Diff from master
Utilities 66.948 kB ExceedsBaseline     1 bytes
SearchBox 184.111 kB ExceedsBaseline     1 bytes
ContextualMenu 161.079 kB ExceedsBaseline     1 bytes
Dialog 195.853 kB ExceedsBaseline     1 bytes
Pivot 185.556 kB ExceedsBaseline     1 bytes
Pickers 273.554 kB ExceedsBaseline     1 bytes
DocumentCard 210.772 kB ExceedsBaseline     1 bytes
Dropdown 222.859 kB ExceedsBaseline     1 bytes
Panel 194.376 kB ExceedsBaseline     1 bytes
Facepile 207.668 kB ExceedsBaseline     1 bytes
Nav 186.871 kB ExceedsBaseline     1 bytes
FloatingPicker 234.683 kB ExceedsBaseline     1 bytes
MessageBar 187.367 kB ExceedsBaseline     1 bytes
Link 71.211 kB ExceedsBaseline     1 bytes
Grid 180.837 kB ExceedsBaseline     1 bytes
GroupedList 141.477 kB ExceedsBaseline     1 bytes
KeytipData 29.294 kB ExceedsBaseline     1 bytes
CommandBar 198.864 kB ExceedsBaseline     1 bytes
DetailsList 227.727 kB ExceedsBaseline     1 bytes
Toggle 76.648 kB ExceedsBaseline     1 bytes
Breadcrumb 197.216 kB ExceedsBaseline     1 bytes
ShimmeredDetailsList 239.265 kB ExceedsBaseline     1 bytes
Button 190.662 kB ExceedsBaseline     1 bytes
Checkbox 84.012 kB ExceedsBaseline     1 bytes
TeachingBubble 192.493 kB ExceedsBaseline     1 bytes
SelectedItemsList 223.917 kB ExceedsBaseline     1 bytes
SwatchColorPicker 195.314 kB ExceedsBaseline     1 bytes
ComboBox 236.252 kB BelowBaseline     -3 bytes
SpinButton 191.472 kB BelowBaseline     -3 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

Copy link
Copy Markdown
Contributor

@JasonGore JasonGore left a comment

Choose a reason for hiding this comment

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

The concern I had in making this change is that we continue to return undefined when given empty strings as input. Can you add test cases like this and verify the continue to work as expected?

          {
            args: ['', ''],
            expected: undefined
          },
          {
            args: [' ', ' '],
            expected: undefined
          },

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

The concern I had in making this change is that we continue to return undefined when given empty strings as input. Can you add test cases like this and verify the continue to work as expected?

          {
            args: ['', ''],
            expected: undefined
          },
          {
            args: [' ', ' '],
            expected: undefined
          },

Great suggestion @JasonGore! Done in 0c5bede.

Copy link
Copy Markdown
Contributor

@natalieethell natalieethell left a comment

Choose a reason for hiding this comment

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

Looks good!

@msft-github-bot
Copy link
Copy Markdown
Contributor

Hello @KevinTCoughlin!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

@msft-github-bot msft-github-bot merged commit 6381336 into microsoft:master Feb 26, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/space-delimit-aria-attrs branch February 26, 2019 19:29
@msft-github-bot
Copy link
Copy Markdown
Contributor

🎉@uifabric/utilities@v6.29.4 has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot
Copy link
Copy Markdown
Contributor

🎉office-ui-fabric-react@v6.145.0 has been released which incorporates this pull request.:tada:

Handy links:

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
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.

5 participants