-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 keyboard issues in add attachment flow #15337
Closed
priyeshshah11
wants to merge
25
commits into
Expensify:main
from
priyeshshah11:fix-keyboard-flash-on-add-attachment
Closed
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
44ff6f6
Fix keyboard issues in add attachment flow
priyeshshah11 17d37d7
fixed refocusing on first popover close bug & mweb refocus after dism…
priyeshshah11 7a9ee23
Added the blur to fix android keyboard not opening issue
priyeshshah11 7771fb0
fixed android keyboard not opening bug
priyeshshah11 f00881b
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 cdb9402
Revert "Merge branch 'main' of https://github.com/priyeshshah11/App i…
priyeshshah11 a76e4fb
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 31b0a4a
updated coment for onModalHide prop
priyeshshah11 8520ae5
added a potential fix for android keyboard not opening issue
priyeshshah11 48b0278
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 ba17fe7
code cleanup
priyeshshah11 17c6759
Resolved merge conflicts
priyeshshah11 6b156eb
fixed lint errors
priyeshshah11 2906e0f
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 77087b8
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 163bee8
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 ac3bdde
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 b2e4adc
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 bd9794a
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 997253f
removing the setTimeout hack that was added to reopen keyboard on mod…
priyeshshah11 892bec9
Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…
priyeshshah11 8d66be0
fixed merge issues
priyeshshah11 8da22e0
fixed merge issues
priyeshshah11 ed3a666
fixed lint issues
priyeshshah11 517e9a9
fixed prettier issues
priyeshshah11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,46 @@ | ||
import React from 'react'; | ||
import React, {useState, useEffect} from 'react'; | ||
import {Keyboard} from 'react-native'; | ||
import compose from '../../libs/compose'; | ||
import withKeyboardState from '../withKeyboardState'; | ||
import withWindowDimensions from '../withWindowDimensions'; | ||
import BaseModal from './BaseModal'; | ||
import {propTypes, defaultProps} from './modalPropTypes'; | ||
|
||
const Modal = props => ( | ||
<BaseModal | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
> | ||
{props.children} | ||
</BaseModal> | ||
); | ||
const Modal = (props) => { | ||
const [isVisible, setIsVisible] = useState(false); | ||
|
||
// This closes the keyboard before opening the modal to avoid the issue where iOS automatically reopens | ||
// the keyboard if it was already open before any modals were opened. We manage the keyboard behaviour | ||
// explicitly at other places in the app to be consistent across all platforms | ||
useEffect(() => { | ||
if (!props.isVisible) { | ||
setIsVisible(false); | ||
} else if (props.isKeyboardShown) { | ||
const keyboardListener = Keyboard.addListener('keyboardDidHide', () => { | ||
setIsVisible(true); | ||
keyboardListener.remove(); | ||
}); | ||
Keyboard.dismiss(); | ||
} else { | ||
setIsVisible(true); | ||
} | ||
}, [props.isVisible, props.isKeyboardShown]); | ||
|
||
return ( | ||
<BaseModal | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
isVisible={isVisible} | ||
> | ||
{props.children} | ||
</BaseModal> | ||
); | ||
}; | ||
|
||
Modal.propTypes = propTypes; | ||
Modal.defaultProps = defaultProps; | ||
Modal.displayName = 'Modal'; | ||
export default withWindowDimensions(Modal); | ||
export default compose( | ||
withWindowDimensions, | ||
withKeyboardState, | ||
)(Modal); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -132,13 +132,15 @@ class ReportActionCompose extends React.Component { | |
this.getInputPlaceholder = this.getInputPlaceholder.bind(this); | ||
this.getIOUOptions = this.getIOUOptions.bind(this); | ||
this.addAttachment = this.addAttachment.bind(this); | ||
this.finishAddAttachmentFlow = this.finishAddAttachmentFlow.bind(this); | ||
this.comment = props.comment; | ||
|
||
// React Native will retain focus on an input for native devices but web/mWeb behave differently so we have some focus management | ||
// code that will refocus the compose input after a user closes a modal or some other actions, see usage of ReportActionComposeFocusManager | ||
this.willBlurTextInputOnTapOutside = willBlurTextInputOnTapOutside(); | ||
|
||
this.state = { | ||
isInAddAttachmentFlow: false, | ||
isFocused: this.willBlurTextInputOnTapOutside && !this.props.modal.isVisible && !this.props.modal.willAlertModalBecomeVisible, | ||
isFullComposerAvailable: props.isComposerFullSize, | ||
textInputShouldClear: false, | ||
|
@@ -179,7 +181,7 @@ class ReportActionCompose extends React.Component { | |
// 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.willBlurTextInputOnTapOutside && this.props.isFocused | ||
if (this.willBlurTextInputOnTapOutside && this.props.isFocused && !this.state.isInAddAttachmentFlow | ||
&& prevProps.modal.isVisible && !this.props.modal.isVisible) { | ||
this.focus(); | ||
} | ||
|
@@ -302,6 +304,15 @@ class ReportActionCompose extends React.Component { | |
this.setState({maxLines}); | ||
} | ||
|
||
/** | ||
* Set isInAddAttachmentFlow state | ||
* | ||
* @param {Boolean} value | ||
*/ | ||
setIsInAddAttachmentFlow(value) { | ||
this.setState({isInAddAttachmentFlow: value}); | ||
} | ||
|
||
isEmptyChat() { | ||
return _.size(this.props.reportActions) === 1; | ||
} | ||
|
@@ -473,6 +484,17 @@ class ReportActionCompose extends React.Component { | |
this.setTextInputShouldClear(false); | ||
} | ||
|
||
/** | ||
* Set the focus back to the composer | ||
*/ | ||
finishAddAttachmentFlow() { | ||
this.setIsInAddAttachmentFlow(false); | ||
|
||
// Explicitly focus the composer from here as componentDidUpdate does not do that in native | ||
// because willBlurTextInputOnTapOutside is false on native which doesn't let it focus | ||
this.focus(true); | ||
} | ||
|
||
/** | ||
* Add a new comment to this chat | ||
* | ||
|
@@ -535,6 +557,7 @@ class ReportActionCompose extends React.Component { | |
]} | ||
> | ||
<AttachmentModal | ||
onModalHide={this.finishAddAttachmentFlow} | ||
headerTitle={this.props.translate('reportActionCompose.sendAttachment')} | ||
onConfirm={this.addAttachment} | ||
> | ||
|
@@ -592,6 +615,10 @@ class ReportActionCompose extends React.Component { | |
|
||
// Drop focus to avoid blue focus ring. | ||
this.actionButton.blur(); | ||
|
||
// we blur the input here to avoid an android specific issue where the keyboard | ||
// doesn't open when we re-focus the input due to it never losing focus | ||
this.textInput.blur(); | ||
this.setMenuVisibility(true); | ||
}} | ||
style={styles.composerSizeButton} | ||
|
@@ -605,16 +632,24 @@ class ReportActionCompose extends React.Component { | |
<PopoverMenu | ||
animationInTiming={CONST.ANIMATION_IN_TIMING} | ||
isVisible={this.state.isMenuVisible} | ||
onClose={() => this.setMenuVisibility(false)} | ||
onItemSelected={() => this.setMenuVisibility(false)} | ||
onClose={() => { | ||
this.setMenuVisibility(false); | ||
this.finishAddAttachmentFlow(); | ||
}} | ||
onItemSelected={() => { | ||
this.setMenuVisibility(false); | ||
this.setIsInAddAttachmentFlow(false); | ||
}} | ||
anchorPosition={styles.createMenuPositionReportActionCompose} | ||
menuItems={[...this.getIOUOptions(reportParticipants), | ||
{ | ||
icon: Expensicons.Paperclip, | ||
text: this.props.translate('reportActionCompose.addAttachment'), | ||
onSelected: () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @priyeshshah11 we need to add the |
||
this.setIsInAddAttachmentFlow(true); | ||
openPicker({ | ||
onPicked: displayFileInModal, | ||
onClose: this.finishAddAttachmentFlow, | ||
}); | ||
}, | ||
}, | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priyeshshah11 We're using
onClose
here to trigger the focus which will face the same issue on Android where it fails to open the keyboard. We might want to use useonModalHide
here instead.