opened
JasonGore
username_0
#### Pull request checklist

- [X] Addresses an existing issue: Fixes #5160
- [X] Include a change request file using `$ npm run change`

#### Description of changes

#5160 is caused by `FocusTrapZone's` inability to properly identify the last tabbable element in a `ChoiceGroup`. Tabbing from any `ChoiceGroupOption` except the last would cause a break out of `FocusTrapZone`.

The fix is to set `tabIndex` on `ChoiceGroupOptions` to allow `FocusTrapZone` to properly detect tabbable elements.

Adding `tabIndex` introduces some edge case scenarios where we need to ensure that `ChoiceGroup` is still tabbable. For example, `tabIndex` needs to be set on the first non-disabled element when no options are checked. If you can think of any other possible regressions caused by introducing `tabIndex` to `ChoiceGroup`, please let me know.

#### Focus areas to test

Add tests to ensure `tabIndex` is set properly based on `checked` and `disabled` states.

###### Microsoft Reviewers: [Open in CodeFlow](http://wpcp.azurewebsites.net/CodeFlowProtocolProxy2.php?pullrequest=https://github.com/OfficeDev/office-ui-fabric-react/pull/5842)
ChoiceGroup: Set tabIndex so that FocusTrapZone can detect tabbable elements
issue
created
JasonGore
username_0
Yep! I based this on a similar ARIA example. I think this is good accessibility as well as fixing the bug.
comment
created
JasonGore
username_0
Per David's PR comments that we shouldn't use tabIndex simply to make an element focusable.
comment
created
jspurlin
username_1
I'm suggesting we do both use the data attribute AND set the tabIndex
comment
created
JasonGore
username_0
I know what you are suggesting, but David is wary of adding tabIndex. I'm hoping you two can agree on the approach.
comment
created
jspurlin
username_1
considering that the W3C example does that and we do that with other components (like FocusZone) it doesn't seem unreasonable to do it as well and be consistent... Although if there's a ChoiceGroup in a FocusZone there may be some tabIndex inconsistencies, maybe the ChoiceGroup could be wrapped in a FocusZone to isolate it
comment
created
JasonGore
username_0
The example does it, but after looking at the ARIA role definition there is no requirement to manipulate tabIndex for accessibility reasons.
comment
created
jspurlin
username_1
true, but it's listed as a way to manager focus within components: https://www.w3.org/TR/wai-aria-practices/#kbd_roving_tabindex (and we already use this concept in FocusZone)
comment