Skip to content

Commit

Permalink
Fix disabled, selected Tab in TabList from impeding keyboarding navig…
Browse files Browse the repository at this point in the history
…ation when trying to tab in (#3235)

* Fix disabled, selected tab impeding keyboard nav

If a selected tab manages to be disabled, the user cannot change
keyboard focus to the tablist or any element beyond that. This commit
fixes the issue.

* Change files

* Update snapshot tests

* A11y state disabled prop is reflected correctly

* Commit for testing narrator bug, PLEASE REVERT

* Revert "Commit for testing narrator bug, PLEASE REVERT"

This reverts commit 98a9c7e.

* Address nits

* Add comments to clairfy exhaustives-deps disabled warnings
  • Loading branch information
lawrencewin authored Nov 29, 2023
1 parent d6902bb commit bdfa459
Show file tree
Hide file tree
Showing 8 changed files with 118 additions and 26 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Fix disabled selected tab impeding keyboard navigation",
"packageName": "@fluentui-react-native/tablist",
"email": "[email protected]",
"dependentChangeType": "patch"
}
8 changes: 4 additions & 4 deletions packages/experimental/TabList/src/Tab/useTab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,17 @@ export const useTab = (props: TabProps): TabInfo => {
...rest
} = props;
// Grabs the context information from Tabs (currently selected Tab and client's onTabSelect callback).
const { addTabKey, invoked, onTabSelect, removeTabKey, setInvoked, setSelectedTabRef, selectedKey, tabKeys, ...tablist } =
const { addTabKey, invoked, onTabSelect, removeTabKey, setInvoked, setFocusedTabRef, selectedKey, tabKeys, ...tablist } =
React.useContext(TabListContext);

const isDisabled = disabled || tablist.disabled;

const changeSelection = React.useCallback(() => {
if (tabKey !== selectedKey) {
onTabSelect(tabKey);
componentRef && setSelectedTabRef(componentRef);
componentRef && setFocusedTabRef(componentRef);
}
}, [componentRef, setSelectedTabRef, onTabSelect, selectedKey, tabKey]);
}, [componentRef, setFocusedTabRef, onTabSelect, selectedKey, tabKey]);

const changeSelectionWithFocus = useOnPressWithFocus(componentRef, changeSelection);

Expand All @@ -65,7 +65,7 @@ export const useTab = (props: TabProps): TabInfo => {
addTabKey(tabKey);
// Set a defaultTabbableElement if we're the initial selectedKey.
if (selectedKey === tabKey) {
componentRef && setSelectedTabRef(componentRef);
componentRef && setFocusedTabRef(componentRef);
}
return () => removeTabKey(tabKey);
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
41 changes: 32 additions & 9 deletions packages/experimental/TabList/src/Tab/useTab.win32.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,27 @@ export const useTab = (props: TabProps): TabInfo => {
...rest
} = props;
// Grabs the context information from Tabs (currently selected Tab and client's onTabSelect callback).
const { addTabKey, invoked, onTabSelect, removeTabKey, setInvoked, setSelectedTabRef, selectedKey, tabKeys, vertical, ...tablist } =
React.useContext(TabListContext);
const {
addTabKey,
invoked,
onTabSelect,
removeTabKey,
setInvoked,
setFocusedTabRef,
selectedKey,
tabKeys,
vertical,
updateDisabledTabs,
updateTabRef,
...tablist
} = React.useContext(TabListContext);

const isDisabled = disabled || tablist.disabled;

const changeSelection = React.useCallback(() => {
if (tabKey !== selectedKey) {
onTabSelect(tabKey);
componentRef && setSelectedTabRef(componentRef);
}
}, [componentRef, setSelectedTabRef, onTabSelect, selectedKey, tabKey]);
onTabSelect(tabKey);
componentRef && setFocusedTabRef(componentRef);
}, [componentRef, setFocusedTabRef, onTabSelect, tabKey]);

/**
* In the main useTab file, our onPress callback we pass to usePressableState below is generated by the useOnPressWithFocus hook.
Expand Down Expand Up @@ -74,22 +84,35 @@ export const useTab = (props: TabProps): TabInfo => {
addTabKey(tabKey);
// Set a defaultTabbableElement if we're the initial selectedKey.
if (selectedKey === tabKey) {
componentRef && setSelectedTabRef(componentRef);
componentRef && setFocusedTabRef(componentRef);
}
return () => removeTabKey(tabKey);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

React.useEffect(() => {
updateTabRef(tabKey, componentRef);
// Disable exhaustive-deps warning because the hook shouldn't run whenever the excluded dependency, updateTabRef, changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [tabKey, componentRef]);

React.useEffect(() => {
updateDisabledTabs(tabKey, disabled);
// Disable exhaustive-deps warning because the hook shouldn't run whenever the excluded dependency, updateDisabledTabs, change.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [tabKey, disabled]);

/**
* Continuing from `handlePressAndDeferFocus`, once we have updated the selection state, we can safely set focus to the correct tab so
* the narrator will only run once.
*/
React.useEffect(() => {
if (invoked && setInvoked && tabKey === selectedKey && !isDisabled) {
componentRef && setSelectedTabRef(componentRef);
componentRef && setFocusedTabRef(componentRef);
componentRef?.current?.focus();
setInvoked(false);
}
// Disable exhaustive-deps warning because hook should only run whenever 'invoked' and its setter are updated.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [invoked, setInvoked]);

Expand Down
4 changes: 2 additions & 2 deletions packages/experimental/TabList/src/TabList/TabList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ export const TabList = compose<TabListType>({

const { disabled, defaultTabbableElement, isCircularNavigation, vertical, ...mergedProps } = mergeProps(tablist.props, final);

const { animatedIndicatorStyles, canShowAnimatedIndicator, layout, selectedKey } = tablist.state;
const { animatedIndicatorStyles, canShowAnimatedIndicator, disabled: tablistDisabledState, layout, selectedKey } = tablist.state;

return (
<TabListContext.Provider
// Passes in the selected key and a hook function to update the newly selected tab and call the client's onTabsClick callback.
value={tablist.state}
>
<Slots.container
disabled={disabled}
disabled={disabled || tablistDisabledState}
defaultTabbableElement={defaultTabbableElement}
focusZoneDirection={vertical ? 'vertical' : 'horizontal'}
isCircularNavigation={isCircularNavigation}
Expand Down
14 changes: 12 additions & 2 deletions packages/experimental/TabList/src/TabList/TabList.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ export interface TabListState {
setInvoked?: (invoked: boolean) => void;

/**
* Setter for the selected Tab's ref to set as the default tabbable element in the FocusZone
* Setter for the focused Tab's ref to set as the default tabbable element in the FocusZone
*/
setSelectedTabRef: (ref: React.RefObject<any>) => void;
setFocusedTabRef: (ref: React.RefObject<any>) => void;

/**
* TabList's `size` prop.
Expand All @@ -106,6 +106,16 @@ export interface TabListState {
*/
updateAnimatedIndicatorStyles?: (updates: AnimatedIndicatorStylesUpdate) => void;

/**
* Updates internal map that keeps track of each of this tablist's tabs disabled state
*/
updateDisabledTabs: (tabKey: string, isDisabled: boolean) => void;

/**
* Updates internal map that keeps track of each of this tablist's tabs' refs.
*/
updateTabRef: (tabKey: string, ref: React.RefObject<View>) => void;

/**
* TabList's `vertical` prop.
*/
Expand Down
4 changes: 3 additions & 1 deletion packages/experimental/TabList/src/TabList/TabListContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ export const TabListContext = React.createContext<TabListState>({
onTabSelect: nullFunction,
removeTabKey: nullFunction,
selectedKey: '',
setSelectedTabRef: nullFunction,
setFocusedTabRef: nullFunction,
size: 'small',
tabKeys: [],
vertical: false,
updateDisabledTabs: nullFunction,
updateTabRef: nullFunction,
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

exports[`TabList component tests TabList appearance 1`] = `
<RCTFocusZone
disabled={false}
focusZoneDirection="horizontal"
navigateAtEnd="NavigateStopAtEnds"
>
Expand Down Expand Up @@ -432,6 +433,7 @@ exports[`TabList component tests TabList appearance 1`] = `

exports[`TabList component tests TabList default props 1`] = `
<RCTFocusZone
disabled={false}
focusZoneDirection="horizontal"
navigateAtEnd="NavigateStopAtEnds"
>
Expand Down Expand Up @@ -1293,6 +1295,7 @@ exports[`TabList component tests TabList disabled list 1`] = `

exports[`TabList component tests TabList orientation 1`] = `
<RCTFocusZone
disabled={false}
focusZoneDirection="vertical"
navigateAtEnd="NavigateStopAtEnds"
>
Expand Down Expand Up @@ -1720,6 +1723,7 @@ exports[`TabList component tests TabList orientation 1`] = `

exports[`TabList component tests TabList selected key 1`] = `
<RCTFocusZone
disabled={false}
focusZoneDirection="horizontal"
navigateAtEnd="NavigateStopAtEnds"
>
Expand Down Expand Up @@ -2151,6 +2155,7 @@ exports[`TabList component tests TabList selected key 1`] = `

exports[`TabList component tests TabList size 1`] = `
<RCTFocusZone
disabled={false}
focusZoneDirection="horizontal"
navigateAtEnd="NavigateStopAtEnds"
>
Expand Down
61 changes: 53 additions & 8 deletions packages/experimental/TabList/src/TabList/useTabList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,28 @@ export const useTabList = (props: TabListProps): TabListInfo => {
} = props;

const data = useSelectedKey(selectedKey || defaultSelectedKey || null, onTabSelect);
const selectedTabKey = selectedKey ?? data.selectedKey;

// selectedTabRef should be set to default tabbable element.
const [selectedTabRef, setSelectedTabRef] = React.useState(React.useRef<View>(null));
// focusedTabRef should be set to default tabbable element.
const [focusedTabRef, setFocusedTabRef] = React.useState(React.useRef<View>(null));
const [invoked, setInvoked] = React.useState(false);
const [tabKeys, setTabKeys] = React.useState<string[]>([]);
const [allTabsDisabled, setAllTabsDisabled] = React.useState(false);

// These maps are used to switch tab focus in the event the selected tab is disabled. React refs are used as storage because updating the maps shouldn't trigger a re-render.
const tabRefMap = React.useRef<{ [key: string]: React.RefObject<View> }>({}).current;
const disabledStateMap = React.useRef<{ [key: string]: boolean }>({}).current;

const updateTabRef = React.useCallback((key: string, ref: React.RefObject<View>) => (tabRefMap[key] = ref), [tabRefMap]);
const updateDisabledTabs = React.useCallback(
(key: string, isDisabled: boolean) => {
disabledStateMap[key] = isDisabled;
if (allTabsDisabled && !isDisabled) {
setAllTabsDisabled(false);
}
},
[allTabsDisabled, disabledStateMap],
);

const addTabKey = React.useCallback(
(tabKey: string) => {
Expand Down Expand Up @@ -94,15 +111,41 @@ export const useTabList = (props: TabListProps): TabListInfo => {
}
};

// If the current selected tab becomes disabled, the following useEffect sets the default focused element to the next non-disabled tab key.
// Without this, keyboard navigation gets stuck when attempting to tab towards the tablist and every following element after
const isSelectedTabDisabled = disabledStateMap[selectedTabKey];

React.useEffect(() => {
if (isSelectedTabDisabled) {
// switch focus to the next available tab key
let tabIndex = tabKeys.indexOf(selectedTabKey);
for (let i = 0; i < tabKeys.length; i++) {
tabIndex = (tabIndex + 1) % tabKeys.length;
if (!disabledStateMap[tabKeys[tabIndex]]) {
break;
}
}
if (tabKeys[tabIndex] === selectedTabKey) {
// In the very rare edge case of all tabs somehow being disabled, we need to set this tablist to become disabled to prevent users from keyboarding in
setAllTabsDisabled(true);
} else {
const ref = tabRefMap[tabKeys[tabIndex]];
setFocusedTabRef(ref);
}
}
// Disable exhaustive-deps warning because this hook should only run once 'isSelectedTabDisabled' dependency changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isSelectedTabDisabled]);

return {
props: {
...props,
accessible: accessible ?? true,
accessibilityState: getAccessibilityState(disabled, accessibilityState),
accessibilityState: getAccessibilityState(disabled || allTabsDisabled, accessibilityState),
accessibilityRole: 'tablist',
appearance: appearance,
componentRef: componentRef,
defaultTabbableElement: selectedTabRef,
defaultTabbableElement: focusedTabRef,
isCircularNavigation: isCircularNavigation ?? false,
onLayout: onTabListLayout,
size: size,
Expand All @@ -113,22 +156,24 @@ export const useTabList = (props: TabListProps): TabListInfo => {
addTabLayout: addTabLayout,
animatedIndicatorStyles: userDefinedAnimatedIndicatorStyles,
appearance: appearance,
canShowAnimatedIndicator: !!(userDefinedAnimatedIndicatorStyles && listLayoutMap && listLayoutMap[selectedKey ?? data.selectedKey]),
disabled: disabled,
canShowAnimatedIndicator: !!(userDefinedAnimatedIndicatorStyles && listLayoutMap && listLayoutMap[selectedTabKey]),
disabled: disabled || allTabsDisabled,
invoked: invoked,
layout: {
tablist: tabListLayout,
tabs: listLayoutMap,
},
onTabSelect: data.onKeySelect,
removeTabKey: removeTabKey,
selectedKey: selectedKey ?? data.selectedKey,
setSelectedTabRef: setSelectedTabRef,
selectedKey: selectedTabKey,
setFocusedTabRef: setFocusedTabRef,
setInvoked: setInvoked,
size: size,
tabKeys: tabKeys,
vertical: vertical,
updateAnimatedIndicatorStyles: updateStyles,
updateDisabledTabs,
updateTabRef,
},
};
};
Expand Down

0 comments on commit bdfa459

Please sign in to comment.