fix(Dropdown): make clearIndicator tabbable in non-search #12684
Conversation
silviuaavram
requested review from
dvdzkwsk,
dzearing,
jurokapsiar,
layershifter,
levithomason,
miroslavstastny,
mnajdova and
pompomon
as code owners
msft-github-bot
added
the
Fluent UI react-northstar (v0)
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 11e1db9534bdfc9e0758cca2ee1edba6cacf2ec7 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
| @@ -506,6 +506,7 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo | |||
| className: Dropdown.slotClassNames.clearIndicator, | |||
| styles: styles.clearIndicator, | |||
| accessibility: indicatorBehavior, | |||
| ...(!search && { tabIndex: 0 }), | |||
There was a problem hiding this comment.
Should we move this logic into indicatorBehavior to be more consistent with other components? Right now we work with tabbIndex mostly in Behaviors only (apart from Dropdown and RadioGroup components)
There was a problem hiding this comment.
I think we should leave indicatorBehavior as it is. Based on this comment and the other one maybe we should create a new behavior, clearIndicatorBehavior or something, that will be tabbable, not have any aria-hidden and instead have aria-label.
@jurokapsiar @kolaps33 what do you think?
There was a problem hiding this comment.
If I understand it correctly with this change there will be element which will have:
role: img,
aria-hidden: true
tabIndex: 0
What doesn't looks reasonable from my point of view.
I think would be better if clear indicator icon element has:
role: button
tabIndex: 0
And in this case it would make sense to put in into new behavior.
There was a problem hiding this comment.
or even wouldn't be there more clear way to use icon only button?
<Button icon="icon-close" text iconOnly title="Close" />
just asking as I don't know the code well if it is possible or not... :)
| @@ -506,6 +506,7 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo | |||
| className: Dropdown.slotClassNames.clearIndicator, | |||
| styles: styles.clearIndicator, | |||
| accessibility: indicatorBehavior, | |||
| ...(!search && { tabIndex: 0 }), | |||
There was a problem hiding this comment.
I think we should add some user-defined label, since the clearIndicator would be tabbable, but it won't have any aria-label to indicate it's purpose to screen-reader users. And also here there might be a question whether or not we should keep 'aria-hidden=true' and 'role=img' from IndicatorBehavior, when we make clearIndicator tabbable
There was a problem hiding this comment.
I'd suggest to create a separate behavior for clearIndicator
| /** | ||
| * @description | ||
| * Indicator is usually only visual representation and therefore is hidden from screen readers, unless 'aria-label' property is provided. | ||
| * Adds attribute 'tabIndex=-1' to 'root' slot if 'search' property is false. Does not set the attribute otherwise. |
There was a problem hiding this comment.
| * Adds attribute 'tabIndex=-1' to 'root' slot if 'search' property is false. Does not set the attribute otherwise. | |
| * Adds attribute 'tabIndex=0' to 'root' slot if 'search' property is false. Does not set the attribute otherwise. |
There was a problem hiding this comment.
And shouldn't it be in the @specification part?
| const dropdownClearIndicatorBehavior: Accessibility<DropdownClearIndicatorBehaviorProps> = props => ({ | ||
| attributes: { | ||
| root: { | ||
| role: 'button', |
There was a problem hiding this comment.
what do you think about the adding condition(!props.search) for the role as well?
I would propose it, because even element has no tabIndex, but if it is button for example JAWS can navigate there with Virtual cursor
I did testing with this example:
https://codesandbox.io/s/fluent-ui-example-2plsz?file=/example.js
…12684) * make clearIndicator tabbable in non search * also add role button * fix unit test * changelog
Pull request checklist
$ yarn changeDescription of changes
To give users the ability to clear the selection by keyboard in a
selectdropdown we are making the indicator tabbable.We are keeping it only clickable in a
comboboxcontext since clearing can be achievable by Escape.Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow