ChoiceGroup: Set data-is-focusable attribute so that FocusTrapZone can detect focusable elements#5842
ChoiceGroup: Set data-is-focusable attribute so that FocusTrapZone can detect focusable elements#5842JasonGore merged 7 commits intomicrosoft:masterfrom JasonGore:jg/5160-fix-choicegroup-in-ftz
Conversation
… detect tabbable elements.
JasonGore
requested review from
FalseLobster,
dzearing,
jspurlin and
natalieethell
There was a problem hiding this comment.
These changes look good to me, given that tabIndex is already an attribute on radio buttons in the following W3C standards example: https://www.w3.org/TR/wai-aria-practices/examples/radio/radio-1/radio-1.html. I would like someone else to look over this, though, in case I'm missing something.
|
Yep! I based this on a similar ARIA example. I think this is good for accessibility as well as fixing the bug. |
|
|
||
| // In cases where no option is checked, set tab index to first enabled option so that ChoiceGroup remains tabbable. | ||
| // If no options are enabled, ChoiceGroup is not tabbable. If any option is checked, do not set keyDefaultTab. | ||
| const firstEnabledOption = disabled || options === undefined ? undefined : options.find(option => !option.disabled); |
There was a problem hiding this comment.
I think you should avoid tabIndex here. Let me try to understand the issue a little better, but I think in general tabIndex=0 should not be required to make an input element focusable.
There was a problem hiding this comment.
I think I can pretty easily try data-is-focusable here and see if FTZ recognizes it since it's almost a 1:1 substitution. If you know of any other methods, let me know.
There was a problem hiding this comment.
This is the ARIA example I used as a reference: https://www.w3.org/TR/2016/WD-wai-aria-practices-1.1-20160317/examples/radio/radio.html
There was a problem hiding this comment.
I tested data-is-focusable and it works! I've switched the approach.
JasonGore
changed the title
| )} | ||
| <div className={classNames.flexContainer}> | ||
| {options!.map((option: IChoiceGroupOption) => { | ||
| const innerOptionProps: IChoiceGroupOptionProps = { |
There was a problem hiding this comment.
Is there any way to keep this typing without getting an error when using data-is-focusable attribute? Every usage I can find in OUFR has no typing.
…asonGore/office-ui-fabric-react into jg/5160-fix-choicegroup-in-ftz
jspurlin
left a comment
jspurlin
left a comment
There was a problem hiding this comment.
Why are we using just data-is-focusable and not additionally setting the tabIndex?
|
Per David's PR comments that we shouldn't use tabIndex simply to make an element focusable. |
|
I'm suggesting we do both use the data attribute AND set the tabIndex |
|
I know what you are suggesting, but David is wary of adding tabIndex. I'm hoping you two can agree on the approach. |
|
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 |
|
The example does it, but after looking at the ARIA role definition there is no requirement to manipulate tabIndex for accessibility reasons. |
|
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) |
5 participants
Pull request checklist
$ npm run changeDescription of changes
#5160 is caused by
FocusTrapZone'sinability to properly identify the last focusable element in aChoiceGroup. Tabbing from anyChoiceGroupOptionexcept the last would cause a break out ofFocusTrapZone.The fix is to set
data-is-focusableattribute onChoiceGroupOptionsto allowFocusTrapZoneto properly detect focusable elements.Adding
data-is-focusableintroduces some edge case scenarios where we need to ensure thatChoiceGroupis still focusable. For example,data-is-focusableneeds 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 introducingdata-is-focusabletoChoiceGroup, please let me know.Focus areas to test
Add tests to ensure
data-is-focusableis set properly based oncheckedanddisabledstates.Microsoft Reviewers: Open in CodeFlow