Combo Box - Adding title and Improved aria-activedescendant for accessibility #3738
Conversation
| * dynamically populate input, and 'none' if auto-complete is not enabled as we can't give user inputs. | ||
| */ | ||
| private _getAriaAutoCompleteValue(): string { | ||
| let autoComplete = !this.props.disabled && this.props.autoComplete; |
There was a problem hiding this comment.
doesn't the autoComplete code need to compare against 'on' since it's not a boolean? The current code will be true if this.props.autoComplete === 'off'
| aria-disabled={ disabled } | ||
| aria-owns={ (id + '-list') } | ||
| spellCheck={ false } | ||
| defaultVisibleValue={ this._currentVisibleValue } | ||
| suggestedDisplayValue={ suggestedDisplayValue } | ||
| updateValueInWillReceiveProps={ this._onUpdateValueInAutoFillWillReceiveProps } | ||
| shouldSelectFullInputValueInComponentDidUpdate={ this._onShouldSelectFullInputValueInAutoFillComponentDidUpdate } | ||
| title={ (ariaLabel && !label) ? ariaLabel : label } |
There was a problem hiding this comment.
We shouldn't be setting the title with ariaLabel, actually we shouldn't be setting aria-label, aria-labelledby, or title at the same time. Why not use this.props.title and then check that there's no label or ariaLabel and a title before adding the title. You'll also need to tell getNativeProps to ignore the title attribute
There was a problem hiding this comment.
You're right, actually I don't think any of the labels are even needed. I think the correct approach here is to just add the title and exclude it so it's not added to the div.
| return 'both'; | ||
| } else { | ||
| return 'none'; | ||
| } |
There was a problem hiding this comment.
Consider changing this to a ternary opertation like: autoComplete ? allFreeform ? 'inline' : 'both' : 'none' or autoComplete ? (allFreeform ? 'inline' : 'both') : 'none' (if you want to be explicit about it). This will make the statements cleaner and remove a duplicate autoComplete check
There was a problem hiding this comment.
Will do. I was once told to avoid it, but if you think it's fine, then I'll make the change.
There was a problem hiding this comment.
Yeah if it's a complicated ternary statement or you're nesting more than two statements don't do it, is how I usually think about it
| } else if (this.props.ariaLabel) { | ||
| title = this.props.ariaLabel; | ||
| } | ||
| return title; |
There was a problem hiding this comment.
We should only be setting the title if one was passed in, so instead of having _getTitle you can just use the title if it's given (inside of render)
|
This fixes issue: #3063 |
…sibility (microsoft#3738) * added aria-autocomplete; added title; added activedescendant with focus item * added comments in combobox * removed autocomplete value for now * cleaned up code * added change files * fixed for id for label to point to the input element * updated changes with aria-autocomplete * fixed lint problem; updated test snap file * fixed aria autocomplete to have the correct logic in if check * simplified code and changed priority of title values * addressed lint problems and updated test * updated title to only use title attribute for combobox
Description of changes
Added a title element to the combo box to help different screen readers better narrate combo box elements. I set the value for our title to be ariaLabel or label whichever value is used in the combo box.
I also updated the aria-activedescendant to use the id of the focused combo box element when we're keyboarding through the elements, instead of always using the id of the previously selected element.
Focus areas to test
The Combo Box component page. Look to see if there's a title attribute and notice how if we were to set focus on the combo box items, we'll change our aria-activedescendant to be the id of the element we're selecting.