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 "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

Merged
merged 7 commits into from
Sep 9, 2022
Merged

fix "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a220e3
fix "Split bill- Own contact gets highlighted when you press down arr…
Aug 18, 2022
704d654
fix "highlight goes out of screen when press down arrow"
Aug 22, 2022
342481d
handle case of all options are disabled
Aug 23, 2022
d44c976
handle case of single option
Aug 25, 2022
4eae63c
add back "maxIndex" check
Aug 31, 2022
d1e764a
Merge branch 'main' of https://github.com/vladnobenladen/Expensify in…
Sep 6, 2022
d66417b
Merge branch 'main' of https://github.com/vladnobenladen/Expensify in…
Sep 6, 2022
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
22 changes: 14 additions & 8 deletions src/components/ArrowKeyFocusManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ const propTypes = {
PropTypes.node,
]).isRequired,

/** Array of disabled indexes. */
disabledIndexes: PropTypes.arrayOf(PropTypes.number),

/** The current focused index. */
focusedIndex: PropTypes.number.isRequired,

Expand All @@ -20,6 +23,10 @@ const propTypes = {
onFocusedIndexChanged: PropTypes.func.isRequired,
};

const defaultProps = {
disabledIndexes: [],
};

class ArrowKeyFocusManager extends Component {
componentDidMount() {
const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP;
Expand All @@ -30,11 +37,10 @@ class ArrowKeyFocusManager extends Component {
return;
}

let newFocusedIndex = this.props.focusedIndex - 1;
let newFocusedIndex = this.props.focusedIndex > 0 ? this.props.focusedIndex - 1 : this.props.maxIndex;

// Wrap around to the bottom of the list
if (newFocusedIndex < 0) {
newFocusedIndex = this.props.maxIndex;
while (this.props.disabledIndexes.includes(newFocusedIndex)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird edge case question: What happens when all the indexes are added to disabledIndexes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Infinite loop that case! I fixed it here

newFocusedIndex = newFocusedIndex > 0 ? newFocusedIndex - 1 : this.props.maxIndex;
}

this.props.onFocusedIndexChanged(newFocusedIndex);
Expand All @@ -45,11 +51,10 @@ class ArrowKeyFocusManager extends Component {
return;
}

let newFocusedIndex = this.props.focusedIndex + 1;
let newFocusedIndex = this.props.focusedIndex < this.props.maxIndex ? this.props.focusedIndex + 1 : 0;

// Wrap around to the top of the list
if (newFocusedIndex > this.props.maxIndex) {
newFocusedIndex = 0;
while (this.props.disabledIndexes.includes(newFocusedIndex)) {
newFocusedIndex = newFocusedIndex < this.props.maxIndex ? newFocusedIndex + 1 : 0;
}

this.props.onFocusedIndexChanged(newFocusedIndex);
Expand All @@ -72,5 +77,6 @@ class ArrowKeyFocusManager extends Component {
}

ArrowKeyFocusManager.propTypes = propTypes;
ArrowKeyFocusManager.defaultProps = defaultProps;

export default ArrowKeyFocusManager;
9 changes: 8 additions & 1 deletion src/components/OptionsSelector/BaseOptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,19 @@ class BaseOptionsSelector extends Component {
*/
flattenSections() {
const allOptions = [];
this.disabledOptionsIndexes = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

any specific reason we're defining it here, rather than the constructor?

Copy link
Contributor Author

@0xmiros 0xmiros Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The state value allOptions can change from componentDidUpdate

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 100 to 109 in dbfd0eb

componentDidUpdate(prevProps) {
if (_.isEqual(this.props.sections, prevProps.sections)) {
return;
}
const newOptions = this.flattenSections();
const newFocusedIndex = this.props.selectedOptions.length;
// eslint-disable-next-line react/no-did-update-set-state
this.setState({
allOptions: newOptions,

We need to sync this.disabledOptionsIndexes with this.state.allOptions always

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah we reinit there its fine. Not asking to remove from flattenSection, instead asking to define the member variable with default value in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be defined in constructor but need to empty array at the entry of flattenSections in the case when called from componentDidUpdate because items already added in constructor

let index = 0;
_.each(this.props.sections, (section, sectionIndex) => {
_.each(section.data, (option, optionIndex) => {
allOptions.push({
...option,
sectionIndex,
index: optionIndex,
});
if (section.isDisabled || option.isDisabled) {
this.disabledOptionsIndexes.push(index);
}
index += 1;
});
});
return allOptions;
Expand Down Expand Up @@ -265,8 +271,9 @@ class BaseOptionsSelector extends Component {
) : <FullScreenLoadingIndicator />;
return (
<ArrowKeyFocusManager
disabledIndexes={this.disabledOptionsIndexes}
focusedIndex={this.state.focusedIndex}
maxIndex={this.props.canSelectMultipleOptions ? this.state.allOptions.length : this.state.allOptions.length - 1}
maxIndex={this.state.allOptions.length - 1}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still not confident of the change, so we'll have to counter with the test so that nothing breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of original code was to have a state of no highlighted options when multiple options type, which is the 2nd issue of this GH (the highlight goes out of the screen)
So this change removes this state and doesn't break any others.

Only one possible regression will be:

App/src/components/ArrowKeyFocusManager.js

Lines 29 to 31 in dbfd0eb

if (this.props.maxIndex < 1) {
return;
}
(when there is only 1 option in list)
This case, that single option will not be highlighted by arrow keys in current codebase.
If it's fine, leave what it is now.
If it's issue, we can fix it by removing that code

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah lets fix it @vladnobenladen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d44c976

Copy link
Collaborator

Choose a reason for hiding this comment

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

Completely removing the code won't help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mananjadhav any issue with it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we could pass any integer and it can cause if the index is negative. But we can ignore this change as I am also fixing it here #7846 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, makes sense in case user can pass any bad values like -1, -100 to maxIndex props.
added and pushed @mananjadhav

onFocusedIndexChanged={this.props.disableArrowKeysActions ? () => {} : this.updateFocusedIndex}
>
<View style={[styles.flex1]}>
Expand Down