Skip to content

Invalid ARIA attributes: Fix empty values#5040

Merged
JasonGore merged 5 commits intomicrosoft:masterfrom
JasonGore:4985-invalid-aria-attributes
May 31, 2018
Merged

Invalid ARIA attributes: Fix empty values#5040
JasonGore merged 5 commits intomicrosoft:masterfrom
JasonGore:4985-invalid-aria-attributes

Conversation

@JasonGore
Copy link
Copy Markdown
Member

@JasonGore JasonGore commented May 30, 2018 •
edited
Loading

Pull request checklist

Description of changes

Prevent ARIA attributes from appearing with invalid empty values when there are no associated values to display.

Starting with #4985 highlighting an issue with invalid ARIA attributes in Checkbox, I found this to be a pattern affecting multiple components: BaseButton, Checkbox, ComboBox, ContextualMenuSplitButton, Dropdown, KeytipData. This PR fixes them. Also consistently default to undefined instead of null in some instances. Created a new utility helper to merge ARIA attributes.

Focus areas to test

Added unit tests for new utility function. Updated snapshots to reflect new behavior.

kenotron
kenotron approved these changes May 30, 2018
View reviewed changes
packages/utilities/src/aria.ts Outdated
* @param ariaAttributes ARIA attributes to merge
*/
export function mergeAriaAttributes(...ariaAttributes: (string | undefined)[]): string | undefined {
let mergedAttribute = '';
Copy link
Copy Markdown
Contributor

@kenotron kenotron May 30, 2018

Choose a reason for hiding this comment

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

return ariaAttributes.filter(arg => arg !== undefined && arg !== null).join(''); maybe?

Copy link
Copy Markdown
Member Author

@JasonGore JasonGore May 31, 2018

Choose a reason for hiding this comment

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

Sure. I like your idea of mergeAriaValues too, I think that would be more clear.

kelseyyoung
kelseyyoung reviewed May 31, 2018
View reviewed changes
packages/utilities/src/aria.ts
* ARIA helper to concatenate attributes, returning undefined if all attributes
* are undefined. (Empty strings are not a valid ARIA attribute value.)
*
* NOTE: This function will NOT insert whitespace between provided attributes.
Copy link
Copy Markdown
Contributor

@kelseyyoung kelseyyoung May 31, 2018

Choose a reason for hiding this comment

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

why not?

Copy link
Copy Markdown
Member Author

@JasonGore JasonGore May 31, 2018

Choose a reason for hiding this comment

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

Because all of the uses present in the codebase were already inserting spaces. This is a helper primarily to return undefined when all values are undefined, as opposed to 'undefined' string value or ''.

@JasonGore JasonGore merged commit 75bfc52 into microsoft:master May 31, 2018
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Jun 1, 2018
@Markionium
Merge branch 'master' into parallel-tsc
2c14a5c 
* master: (95 commits)
  Applying package updates.
  Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057)
  Add pointerup listener to exit keytip mode (microsoft#5051)
  Update PULL_REQUEST_TEMPLATE.md
  Update ISSUE_TEMPLATE.md
  Shimmer: resolve conflicts (microsoft#5034)
  Invalid ARIA attributes: Fix empty values (microsoft#5040)
  ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001)
  ComboBox: Fix submit pending value (microsoft#5048)
  FocusTrapZone - restore last focused descendant element (microsoft#4897)
  Applying package updates.
  Take 2 of the require.resolve change. This time using the "resolve" pkg (microsoft#5031)
  fixing webpack config to allow rush build to complete on a small VM (microsoft#5037)
  MessageBar: change color of X close button so that it is accessible (microsoft#5039)
  Theming: improve accessibility (microsoft#5038)
  Applying package updates.
  Added 'made with fabric' to readme (microsoft#5018)
  HoverCard: example accessibility fix. (microsoft#5027)
  Dropdown caret (microsoft#5016)
  Applying package updates.
  ...
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Jun 2, 2018
@Markionium
Merge branch 'master' into prettier-before-run
ff109ec 
* master: (274 commits)
  Demo Page Refactor Part 1 (microsoft#5055)
  SplitButton: Add aria-roledescription (microsoft#5062)
  Add addins sketch toolkit link (microsoft#5052)
  Dropdown title (microsoft#4983)
  Allow for more control over event handling for keytips (microsoft#5064)
  Build time speed improvements (microsoft#4965)
  Coachmark: Positioning fixes (microsoft#4995)
  Applying package updates.
  Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057)
  Add pointerup listener to exit keytip mode (microsoft#5051)
  Update PULL_REQUEST_TEMPLATE.md
  Update ISSUE_TEMPLATE.md
  Shimmer: resolve conflicts (microsoft#5034)
  Invalid ARIA attributes: Fix empty values (microsoft#5040)
  ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001)
  ComboBox: Fix submit pending value (microsoft#5048)
  FocusTrapZone - restore last focused descendant element (microsoft#4897)
  Applying package updates.
  Take 2 of the require.resolve change. This time using the "resolve" pkg (microsoft#5031)
  fixing webpack config to allow rush build to complete on a small VM (microsoft#5037)
  ...
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Jun 3, 2018
@Markionium
Merge branch 'master' into mapol/allow-overriding-root-element-on-link
3ad7870 
* master: (111 commits)
  Moving a variable to be defined sooner so that it is there when failures occur.
  Fix icon imports (microsoft#5069)
  MessageBar: More visible HC color for dismiss and expand buttons (microsoft#5061)
  Fix DetailsList accessibility and add more ARIA hooks (microsoft#5066)
  Update jest (microsoft#5068)
  Demo Page Refactor Part 1 (microsoft#5055)
  SplitButton: Add aria-roledescription (microsoft#5062)
  Add addins sketch toolkit link (microsoft#5052)
  Dropdown title (microsoft#4983)
  Allow for more control over event handling for keytips (microsoft#5064)
  Build time speed improvements (microsoft#4965)
  Coachmark: Positioning fixes (microsoft#4995)
  Applying package updates.
  Experiments/Nav component: display "show more" link only if there is atleast one hidden link (microsoft#5057)
  Add pointerup listener to exit keytip mode (microsoft#5051)
  Update PULL_REQUEST_TEMPLATE.md
  Update ISSUE_TEMPLATE.md
  Shimmer: resolve conflicts (microsoft#5034)
  Invalid ARIA attributes: Fix empty values (microsoft#5040)
  ComboBox: Correct invalid ARIA attributes. (microsoft#4873) (microsoft#5001)
  ...
@JasonGore JasonGore deleted the 4985-invalid-aria-attributes branch June 19, 2018 20:43
@JasonGore JasonGore mentioned this pull request Aug 6, 2018
Closed
@JasonGore JasonGore mentioned this pull request Sep 11, 2018
Closed
@JasonGore JasonGore mentioned this pull request Feb 21, 2019
Closed
2 tasks
@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.

Reviewers

@dzearing dzearing Awaiting requested review from dzearing

@kkjeer kkjeer Awaiting requested review from kkjeer

@micahgodbolt micahgodbolt Awaiting requested review from micahgodbolt

3 more reviewers

@kelseyyoung kelseyyoung kelseyyoung left review comments

@kenotron kenotron kenotron approved these changes

@joschect joschect joschect approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Checkbox accepting null ariaDescribedBy and creating an attribute without value

4 participants

@JasonGore @kenotron @kelseyyoung @joschect