Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(selection-list): highlight on lists with 1 section #27246

Merged
1 change: 1 addition & 0 deletions src/components/Button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ class Button extends Component {
]}
nativeID={this.props.nativeID}
accessibilityLabel={this.props.accessibilityLabel}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
hoverDimmingValue={1}
>
{this.renderContent()}
Expand Down
39 changes: 27 additions & 12 deletions src/components/SelectionList/BaseSelectionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ function BaseSelectionList({
};
}, [canSelectMultiple, sections]);

// Disable `Enter` hotkey if the active element is a button or checkbox
const shouldDisableHotkeys = activeElement && [CONST.ACCESSIBILITY_ROLE.BUTTON, CONST.ACCESSIBILITY_ROLE.CHECKBOX].includes(activeElement.role);

// If `initiallyFocusedOptionKey` is not passed, we fall back to `-1`, to avoid showing the highlight on the first member
const [focusedIndex, setFocusedIndex] = useState(() => _.findIndex(flattenedSections.allOptions, (option) => option.keyForList === initiallyFocusedOptionKey));

Expand Down Expand Up @@ -168,23 +171,35 @@ function BaseSelectionList({
listRef.current.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated, viewOffset: variables.contentHeaderHeight});
};

const selectRow = (item, index) => {
/**
* Logic to run when a row is selected, either with click/press or keyboard hotkeys.
*
* @param {Object} item - the list item
* @param {Boolean} shouldUnfocusRow - flag to decide if we should unfocus all rows. True when selecting a row with click or press (not keyboard)
*/
const selectRow = (item, shouldUnfocusRow = false) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why defaulting shouldUnfocusRow to false when the callers always have true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allroundexperts The callers that pass true are the press handlers. Please search for the keyboard subscriber - there selectRow is used without the second parameter, which defaults to false

// In single-selection lists we don't care about updating the focused index, because the list is closed after selecting an item
if (canSelectMultiple) {
if (sections.length === 1) {
// If the list has only 1 section (e.g. Workspace Members list), we always focus the next available item
const nextAvailableIndex = _.findIndex(flattenedSections.allOptions, (option, i) => i > index && !option.isDisabled);
setFocusedIndex(nextAvailableIndex);
} else {
// If the list has multiple sections (e.g. Workspace Invite list), we focus the first one after all the selected (selected items are always at the top)
if (sections.length > 1) {
// If the list has only 1 section (e.g. Workspace Members list), we do nothing.
// If the list has multiple sections (e.g. Workspace Invite list), and `shouldUnfocusRow` is false,
// we focus the first one after all the selected (selected items are always at the top).
const selectedOptionsCount = item.isSelected ? flattenedSections.selectedOptions.length - 1 : flattenedSections.selectedOptions.length + 1;
setFocusedIndex(selectedOptionsCount);

if (!shouldUnfocusRow) {
setFocusedIndex(selectedOptionsCount);
}

if (!item.isSelected) {
// If we're selecting an item, scroll to it's position at the top, so we can see it
scrollToIndex(Math.max(selectedOptionsCount - 1, 0), true);
}
}

if (shouldUnfocusRow) {
// Unfocus all rows when selecting row with click/press
setFocusedIndex(-1);
}
}

onSelectRow(item);
Expand All @@ -197,7 +212,7 @@ function BaseSelectionList({
return;
}

selectRow(focusedOption, focusedIndex);
selectRow(focusedOption);
};

/**
Expand Down Expand Up @@ -254,7 +269,7 @@ function BaseSelectionList({
<UserListItem
item={item}
isFocused={isItemFocused}
onSelectRow={() => selectRow(item, index)}
onSelectRow={() => selectRow(item, true)}
onDismissError={onDismissError}
showTooltip={showTooltip}
/>
Expand All @@ -266,7 +281,7 @@ function BaseSelectionList({
item={item}
isFocused={isItemFocused}
isDisabled={isDisabled}
onSelectRow={() => selectRow(item, index)}
onSelectRow={() => selectRow(item, true)}
/>
);
};
Expand All @@ -290,7 +305,7 @@ function BaseSelectionList({
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
captureOnInputs: true,
shouldBubble: () => !flattenedSections.allOptions[focusedIndex],
isActive: !activeElement && isFocused,
isActive: !shouldDisableHotkeys && isFocused,
});

/** Calls confirm action when pressing CTRL (CMD) + Enter */
Expand Down
1 change: 0 additions & 1 deletion src/components/SelectionList/RadioListItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ function RadioListItem({item, isFocused = false, isDisabled = false, onSelectRow
accessibilityRole="button"
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
focusStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
>
<View style={[styles.flex1, styles.justifyContentBetween, styles.sidebarLinkInner, styles.optionRow, styles.userSelectNone, isFocused && styles.sidebarLinkActive]}>
Expand Down
1 change: 0 additions & 1 deletion src/components/SelectionList/UserListItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ function UserListItem({item, isFocused = false, showTooltip, onSelectRow, onDism
accessibilityState={{checked: item.isSelected}}
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
focusStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
>
<View style={styles.checkboxPressable}>
Expand Down
6 changes: 6 additions & 0 deletions src/hooks/useActiveElement/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import {useEffect, useState} from 'react';

/**
* Listens for the focusin and focusout events and sets the DOM activeElement to the state.
* On native, we just return null.
*
* @return {Element} the active element in the DOM
*/
export default function useActiveElement() {
const [active, setActive] = useState(document.activeElement);

Expand Down
5 changes: 5 additions & 0 deletions src/hooks/useActiveElement/index.native.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* Native doesn't have the DOM, so we just return null.
*
* @return {null}
*/
export default function useActiveElement() {
return null;
}
Loading