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.

Sign up for GitHub

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: prevent refocusing in options-selector and selection-list #29084

Merged
merged 7 commits into from
Oct 10, 2023
Merged

fix: prevent refocusing in options-selector and selection-list #29084

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e41454b
fix: prevent refocusing in selection-list
sangar-1028 Oct 6, 2023
5142adc
fix: prevent refocusing in option-selector
sangar-1028 Oct 6, 2023
1f59c8f
Merge branch 'main' into fix/issue-22812
sangar-1028 Oct 9, 2023
5a74195
fix: change props name
sangar-1028 Oct 9, 2023
90c6039
fix: prettier issue
sangar-1028 Oct 9, 2023
2b2d137
fix: focus in workspace invite page
sangar-1028 Oct 9, 2023
1b94af4
fix: use unique props name
sangar-1028 Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/components/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ const propTypes = {
/** Whether to remove the lateral padding and align the content with the margins */
shouldDisableRowInnerPadding: PropTypes.bool,

/** Whether to prevent default focusing on select */
shouldPreventDefaultFocusOnSelect: PropTypes.bool,

/** Whether to wrap large text up to 2 lines */
isMultilineSupported: PropTypes.bool,

Expand All @@ -95,6 +98,7 @@ const defaultProps = {
style: null,
shouldHaveOptionSeparator: false,
shouldDisableRowInnerPadding: false,
shouldPreventDefaultFocusOnSelect: false,
};

class OptionRow extends Component {
Expand Down Expand Up @@ -213,6 +217,7 @@ class OptionRow extends Component {
hoverDimmingValue={1}
hoverStyle={this.props.hoverStyle}
needsOffscreenAlphaCompositing={lodashGet(this.props.option, 'icons.length', 0) >= 2}
onMouseDown={this.props.shouldPreventDefaultFocusOnSelect ? (e) => e.preventDefault() : undefined}
>
<View style={sidebarInnerRowStyle}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
Expand Down
2 changes: 2 additions & 0 deletions src/components/OptionsList/BaseOptionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ function BaseOptionsList({
showScrollIndicator,
listContainerStyles,
shouldDisableRowInnerPadding,
shouldPreventDefaultFocusOnSelect,
disableFocusOptions,
canSelectMultipleOptions,
shouldShowMultipleOptionSelectorAsButton,
Expand Down Expand Up @@ -206,6 +207,7 @@ function BaseOptionsList({
isDisabled={isItemDisabled}
shouldHaveOptionSeparator={index > 0 && shouldHaveOptionSeparator}
shouldDisableRowInnerPadding={shouldDisableRowInnerPadding}
shouldPreventDefaultFocusOnSelect={shouldPreventDefaultFocusOnSelect}
isMultilineSupported={isRowMultilineSupported}
/>
);
Expand Down
4 changes: 4 additions & 0 deletions src/components/OptionsList/optionsListPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ const propTypes = {
/** Whether to disable the inner padding in rows */
shouldDisableRowInnerPadding: PropTypes.bool,

/** Whether to prevent default focusing when selecting a row */
shouldPreventDefaultFocusOnSelect: PropTypes.bool,

/** Whether to show the scroll bar */
showScrollIndicator: PropTypes.bool,

Expand Down Expand Up @@ -107,6 +110,7 @@ const defaultProps = {
onLayout: undefined,
shouldHaveOptionSeparator: false,
shouldDisableRowInnerPadding: false,
shouldPreventDefaultFocusOnSelect: false,
showScrollIndicator: false,
isRowMultilineSupported: false,
};
Expand Down
1 change: 1 addition & 0 deletions src/components/OptionsSelector/BaseOptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ class BaseOptionsSelector extends Component {
isLoading={!this.props.shouldShowOptions}
showScrollIndicator={this.props.showScrollIndicator}
isRowMultilineSupported={this.props.isRowMultilineSupported}
shouldPreventDefaultFocusOnSelect={this.props.shouldFocusOnSelectRow}
/>
);
return (
Expand Down
12 changes: 11 additions & 1 deletion src/components/SelectionList/BaseListItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,16 @@ import RadioListItem from './RadioListItem';
import OfflineWithFeedback from '../OfflineWithFeedback';
import CONST from '../../CONST';

function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip, canSelectMultiple = false, onSelectRow, onDismissError = () => {}}) {
function BaseListItem({
item,
isFocused = false,
isDisabled = false,
showTooltip,
shouldPreventDefaultFocusOnSelect = false,
canSelectMultiple = false,
onSelectRow,
onDismissError = () => {},
}) {
const isUserItem = lodashGet(item, 'icons.length', 0) > 0;
const ListItem = isUserItem ? UserListItem : RadioListItem;

Expand All @@ -32,6 +41,7 @@ function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip,
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldPreventDefaultFocusOnSelect ? (e) => e.preventDefault() : undefined}
>
<View
style={[
Expand Down
2 changes: 2 additions & 0 deletions src/components/SelectionList/BaseSelectionList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ function BaseSelectionList({
canSelectMultiple={canSelectMultiple}
onSelectRow={() => selectRow(item, true)}
onDismissError={onDismissError}
shouldPreventDefaultFocusOnSelect={shouldFocusOnSelectRow}
/>
);
};
Expand Down Expand Up @@ -401,6 +402,7 @@ function BaseSelectionList({
accessibilityState={{checked: flattenedSections.allSelected}}
disabled={flattenedSections.allOptions.length === flattenedSections.disabledOptionsIndexes.length}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldFocusOnSelectRow ? (e) => e.preventDefault() : undefined}
>
<Checkbox
accessibilityLabel={translate('workspace.people.selectAll')}
Expand Down
1 change: 1 addition & 0 deletions src/components/SelectionList/selectionListPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const radioListItemPropTypes = {
const baseListItemPropTypes = {
...commonListItemPropTypes,
item: PropTypes.oneOfType([PropTypes.shape(userListItemPropTypes.item), PropTypes.shape(radioListItemPropTypes.item)]),
shouldPreventDefaultFocusOnSelect: PropTypes.bool,
};

const propTypes = {
Expand Down
2 changes: 2 additions & 0 deletions src/pages/workspace/WorkspaceInvitePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import withPolicy, {policyDefaultProps, policyPropTypes} from './withPolicy';
import FullPageNotFoundView from '../../components/BlockingViews/FullPageNotFoundView';
import ROUTES from '../../ROUTES';
import * as PolicyUtils from '../../libs/PolicyUtils';
import * as Browser from '../../libs/Browser';
import useNetwork from '../../hooks/useNetwork';
import useLocalize from '../../hooks/useLocalize';
import SelectionList from '../../components/SelectionList';
Expand Down Expand Up @@ -231,6 +232,7 @@ function WorkspaceInvitePage(props) {
onConfirm={inviteUser}
showScrollIndicator
showLoadingPlaceholder={!didScreenTransitionEnd || !OptionsListUtils.isPersonalDetailsReady(props.personalDetails)}
shouldFocusOnSelectRow={!Browser.isMobile()}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two props - shouldFocusOnSelectRow and shouldPreventDefaultFocusOnSelect. Can we make it one to make it easy to understand?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MonilBhavsar Can you please check this discussion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Commented there

/>
<View style={[styles.flexShrink0]}>
<FormAlertWithSubmitButton
Expand Down