Skip to content

Fixes invalid aria-owns/ activedescendant attributes on BasePicker#4552

Merged
manishgarg1 merged 6 commits intomicrosoft:masterfrom
rebeccaballantyne:rebeba/aria-owns-invalid
Apr 18, 2018
Merged

Fixes invalid aria-owns/ activedescendant attributes on BasePicker#4552
manishgarg1 merged 6 commits intomicrosoft:masterfrom
rebeccaballantyne:rebeba/aria-owns-invalid

Conversation

@rebeccaballantyne
Copy link
Copy Markdown

@rebeccaballantyne rebeccaballantyne commented Apr 13, 2018

Pull request checklist

Description of changes

Fixes aria-owns issue on BasePicker. Aria-owns should only be set to "suggestion-list" when suggestions are rendered.

Focus areas to test

N/A

suggestedDisplayValue={ suggestedDisplayValue }
aria-activedescendant={ activeDescendant }
aria-owns='suggestion-list'
aria-owns={ !!this.state.suggestionsVisible ? 'suggestion-list' : '' }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this.state.suggestionsVisible [](start = 30, length = 29)

I don;t think you really need to use the !! prefix

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

True, because it's a ternary. Removing.

Copy link
Copy Markdown
Collaborator

@manishgarg1 manishgarg1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@manishgarg1
Copy link
Copy Markdown
Collaborator

@joschect, are you good with this?

@KevinTCoughlin
Copy link
Copy Markdown
Member

Change overall LGTM, but one minor nit given the spec https://www.w3.org/WAI/PF/aria-1.1/states_and_properties#aria-owns

The value of the aria-owns attribute is a space-separated list of IDREFS that reference one or more elements in the document by ID. The reason for adding aria-owns is to expose a parent/child contextual relationship to assistive technologies that is otherwise impossible to infer from the DOM.

So an empty string for the value when the suggestion list is not shown may be invalid. Though my WAVE extension doesn't seem to flag it when testing.

Might be better to return null or undefined? Thoughts?

suggestedDisplayValue={ suggestedDisplayValue }
aria-activedescendant={ activeDescendant }
aria-owns='suggestion-list'
aria-owns={ this.state.suggestionsVisible ? 'suggestion-list' : '' }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we use undefined rather than blank string?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would also prefer undefined if that will work.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks. Will update!

@dzearing
Copy link
Copy Markdown
Member

Yes undefined. The null value is not liked by the latest react typings.

@manishgarg1 manishgarg1 self-assigned this Apr 16, 2018
@rebeccaballantyne rebeccaballantyne changed the title Fixes invalid aria-owns attribute on BasePicker Fixes invalid aria-owns/ activedescendant attributes on BasePicker Apr 17, 2018
@manishgarg1 manishgarg1 merged commit 9686b87 into microsoft:master Apr 18, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accessibility- BasePicker aria-owns and activedescendant field should not be set unless suggestions are rendered

6 participants