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

[No QA] fixed keyboard appears on split bill page #2022

Merged
merged 8 commits into from
Apr 13, 2021
Merged

[No QA] fixed keyboard appears on split bill page #2022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
170f5f3
fix: keyboard appears on split bill page
parasharrajat Mar 23, 2021
6e40e39
typo fixed
parasharrajat Mar 23, 2021
780f0d2
Merge branch 'master' of github.com:Expensify/Expensify.cash into par…
parasharrajat Mar 25, 2021
c4f64e5
refactor
parasharrajat Mar 25, 2021
67f3823
Merge branch 'master' of github.com:Expensify/Expensify.cash into par…
parasharrajat Apr 12, 2021
1c343db
fix: Composer focus
parasharrajat Apr 13, 2021
e88cf38
Merge branch 'master' of github.com:Expensify/Expensify.cash into par…
parasharrajat Apr 13, 2021
80411e8
refactor the Composer focus logic.
parasharrajat Apr 13, 2021
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 package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"@react-native-firebase/app": "^8.4.5",
"@react-native-firebase/crashlytics": "^8.4.9",
"@react-native-picker/picker": "^1.9.11",
"@react-navigation/compat": "^5.3.15",
"@react-navigation/drawer": "5.12.5",
"@react-navigation/native": "5.9.2",
"@react-navigation/stack": "5.14.2",
Expand Down
3 changes: 3 additions & 0 deletions src/libs/canFocusInputOnScreenFocus/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import canUseTouchScreen from '../canUseTouchscreen';

export default () => !canUseTouchScreen();
1 change: 1 addition & 0 deletions src/libs/canFocusInputOnScreenFocus/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => false;
28 changes: 22 additions & 6 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import {View, TouchableOpacity} from 'react-native';
import {View, TouchableOpacity, InteractionManager} from 'react-native';
import {withNavigationFocus} from '@react-navigation/compat';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -18,6 +19,7 @@ import compose from '../../../libs/compose';
import CreateMenu from '../../../components/CreateMenu';
import withWindowDimensions from '../../../components/withWindowDimensions';
import withDrawerState from '../../../components/withDrawerState';
import canFocusInputOnScreenFocus from '../../../libs/canFocusInputOnScreenFocus';

const propTypes = {
// A method to call when the form is submitted
Expand Down Expand Up @@ -47,6 +49,10 @@ const propTypes = {

/* Is the window width narrow, like on a mobile device */
isSmallScreenWidth: PropTypes.bool.isRequired,

// Is composer screen focused
isFocused: PropTypes.bool.isRequired,

};

const defaultProps = {
Expand All @@ -66,18 +72,23 @@ class ReportActionCompose extends React.Component {
this.setIsFocused = this.setIsFocused.bind(this);
this.focus = this.focus.bind(this);
this.comment = props.comment;
this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();

this.state = {
isFocused: true,
isFocused: this.shouldFocusInputOnScreenFocus,
textInputShouldClear: false,
isCommentEmpty: props.comment.length === 0,
isMenuVisible: false,
};
}

componentDidUpdate(prevProps) {
// When any modal goes from visible to hidden, bring focus to the compose field
if (prevProps.modal.isVisible && !this.props.modal.isVisible) {
// We want to focus or refocus the input when a modal has been closed and the underlying screen is focused.
// We avoid doing this on native platforms since the software keyboard popping
// open creates a jarring and broken UX.
if (this.shouldFocusInputOnScreenFocus && this.props.isFocused
&& prevProps.modal.isVisible && !this.props.modal.isVisible) {
this.setIsFocused(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

setIsFocused(true) would be called when the field is focused anyway.
Though if there is a legit reason to call it here, the call can just be moved inside the focus method

this.setIsFocused(true);
this.focus();

vs

this.focus();

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice thoughts. Yeah Cancellation of Interaction is good to have.

But about autofocus, I think there are some PR on react native that are handling animation and focus issue. But I agree lets just move the interaction based focus to TextInputFocusable. I didn't do this here as that covers bigger scope then the current issue. So its better to first analyze the effect of these changes and consider them.

this.focus();
}
}
Expand Down Expand Up @@ -114,7 +125,11 @@ class ReportActionCompose extends React.Component {
*/
focus() {
if (this.textInput) {
this.textInput.focus();
// There could be other animations running while we trigger manual focus.
// This prevents focus from making those animations janky.
InteractionManager.runAfterInteractions(() => {
this.textInput.focus();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about what issue this is addressing. Can we add some comments to explain what interactions we are waiting to finish?

Copy link
Member Author

Choose a reason for hiding this comment

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

Animations do not run smoothly, as a side effect of the focus. So this case should suffice when the modal closes. It's a precautionary check that's all. Not particularly needed for these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool whatever it is, please add an explanation so we have some insights when looking at the code. Out of curiositym is there a particular modal closing that causes jank? Maybe we can add a test to verify it does not look janky?

cc @Julesssss We should also maybe update these test steps so that it is testable by QA?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, the Most effect of focus interfering with animation can be seen on a new chat or new group screen where we set the focus on Search input. Here, we may need this on the web when we close modals and then set focus back to Input but it's not apparent. Due to the similar effect happening on New Chat modals, I thought it's better to apply Interactionmanager on all the places where we are setting focus.

Copy link
Contributor

@kidroca kidroca Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, the Most effect of focus interfering with animation can be seen on a new chat or new group screen where we set the focus on Search input.

But these are due to the autoFocus. The logic here would be applied for manual/programmatic focuses only
If you want to make auto focus wait for interactions as well you'll need to remove the autoFocus prop and call this.focus() when the component mount

I thought it's better to apply Interactionmanager on all the places where we are setting focus.

This applies it only here on compose though right? Opening a new chat or search use the TextInputFocusable
Instead of adding this logic to all the places that set focus couldn't it perhaps be added to TextInputFocusable.
TextInputFocusable can wrap inputRef.focus with this behavior (before exposing it to parents)

Since focusing is done inside a callback, is it guaranteed you will always be inside a chat when this callback is invoked? Could it be possible for the user to tap/swipe too fast, open a chat and go back to LHN or somewhere else and then this callback runs and focuses the field?

  • if (this.textInput) - textInput - could be defined before setting this interaction, but could have changed inside the actual callback
  • you will want to abort this interaction in case the component is unmounted but the callback is still scheduled to run. Similar to how you would clean up timers when a component unmounts
focus() {
  this.scheduledIntercation = Interactionmanager.runAfterInteractions(() => {...})
}

componentWillUnmount() {
  if(this.scheduledIntercation) this.scheduledIntercation.cancel()
}

I think this works great, just that it should have been reusable/configurable for the other input fields as well

}
}

Expand Down Expand Up @@ -262,7 +277,7 @@ class ReportActionCompose extends React.Component {
)}
</AttachmentPicker>
<TextInputFocusable
autoFocus
autoFocus={this.shouldFocusInputOnScreenFocus}
multiline
ref={el => this.textInput = el}
textAlignVertical="top"
Expand Down Expand Up @@ -320,6 +335,7 @@ ReportActionCompose.defaultProps = defaultProps;
export default compose(
withWindowDimensions,
withDrawerState,
withNavigationFocus,
withOnyx({
comment: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
Expand Down