-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix keyboard flashing while clicking "Add attachment" #23994
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
Changes from all commits
11717c5
e076446
4ebbd77
7ac72b5
87699e3
98a5c4a
47c05fe
4d0afeb
887427a
a021e0f
bd2b6ad
3dfcf11
767b239
4d165ce
09baf7b
e272a1c
67d3228
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| diff --git a/node_modules/react-native/React/Views/RCTModalHostViewManager.m b/node_modules/react-native/React/Views/RCTModalHostViewManager.m | ||
| index 4b9f9ad..b72984c 100644 | ||
| --- a/node_modules/react-native/React/Views/RCTModalHostViewManager.m | ||
| +++ b/node_modules/react-native/React/Views/RCTModalHostViewManager.m | ||
| @@ -79,6 +79,13 @@ RCT_EXPORT_MODULE() | ||
| if (self->_presentationBlock) { | ||
| self->_presentationBlock([modalHostView reactViewController], viewController, animated, completionBlock); | ||
| } else { | ||
| + // In our App, If an input is blurred and a modal is opened, the rootView will become the firstResponder, which | ||
| + // will cause system to retain a wrong keyboard state, and then the keyboard to flicker when the modal is closed. | ||
| + // We first resign the rootView to avoid this problem. | ||
| + UIWindow *window = RCTKeyWindow(); | ||
| + if (window && window.rootViewController && [window.rootViewController.view isFirstResponder]) { | ||
| + [window.rootViewController.view resignFirstResponder]; | ||
| + } | ||
| [[modalHostView reactViewController] presentViewController:viewController | ||
| animated:animated | ||
| completion:completionBlock]; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import {propTypes as modalPropTypes, defaultProps as modalDefaultProps} from './ | |
| import * as Modal from '../../libs/actions/Modal'; | ||
| import getModalStyles from '../../styles/getModalStyles'; | ||
| import variables from '../../styles/variables'; | ||
| import ComposerFocusManager from '../../libs/ComposerFocusManager'; | ||
|
|
||
| const propTypes = { | ||
| ...modalPropTypes, | ||
|
|
@@ -73,6 +74,9 @@ class BaseModal extends PureComponent { | |
| this.props.onModalHide(); | ||
| } | ||
| Modal.onModalDidClose(); | ||
| if (!this.props.fullscreen) { | ||
|
Contributor
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. what's this condition for?
Contributor
Author
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.
Contributor
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. Hey @ntdiary, I know this thread is kinda old, but would appreciate your input:
Contributor
Author
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. @paultsimura, when using the
Contributor
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. Thanks @ntdiary. Do you have any particular
Contributor
Author
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. @paultsimura, I'm not exactly sure about the new code in the main branch, maybe you can verify the emoji picker in mobile chrome. Additionally, I'm refactoring the modal's refocusing behavior (#29199), so there might be a chance to review the related code again tomorrow. :)
Contributor
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. Cool, would your change cover the modal being closed on clicking the browser's "Back" button as well?
Contributor
Author
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. Yeah, I added a new |
||
| ComposerFocusManager.setReadyToFocus(); | ||
| } | ||
| } | ||
|
|
||
| render() { | ||
|
|
@@ -109,6 +113,9 @@ class BaseModal extends PureComponent { | |
| // Note: Escape key on web/desktop will trigger onBackButtonPress callback | ||
| // eslint-disable-next-line react/jsx-props-no-multi-spaces | ||
| onBackButtonPress={this.props.onClose} | ||
| onModalWillShow={() => { | ||
| ComposerFocusManager.resetReadyToFocus(); | ||
| }} | ||
| onModalShow={() => { | ||
| if (this.props.shouldSetModalVisibility) { | ||
| Modal.setModalVisibility(true); | ||
|
|
@@ -117,6 +124,7 @@ class BaseModal extends PureComponent { | |
| }} | ||
| propagateSwipe={this.props.propagateSwipe} | ||
| onModalHide={this.hideModal} | ||
| onDismiss={() => ComposerFocusManager.setReadyToFocus()} | ||
| onSwipeComplete={this.props.onClose} | ||
| swipeDirection={swipeDirection} | ||
| isVisible={this.props.isVisible} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| let isReadyToFocusPromise = Promise.resolve(); | ||
|
Contributor
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. Do we need this logic on web at all?
Contributor
Author
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. Yeah, the |
||
| let resolveIsReadyToFocus; | ||
|
|
||
| function resetReadyToFocus() { | ||
| isReadyToFocusPromise = new Promise((resolve) => { | ||
| resolveIsReadyToFocus = resolve; | ||
| }); | ||
| } | ||
| function setReadyToFocus() { | ||
| if (!resolveIsReadyToFocus) { | ||
| return; | ||
| } | ||
| resolveIsReadyToFocus(); | ||
| } | ||
| function isReadyToFocus() { | ||
| return isReadyToFocusPromise; | ||
| } | ||
|
|
||
| export default { | ||
| resetReadyToFocus, | ||
| setReadyToFocus, | ||
| isReadyToFocus, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import {InteractionManager} from 'react-native'; | ||
| import ComposerFocusManager from './ComposerFocusManager'; | ||
|
|
||
| /** | ||
| * Create a function that focuses a text input. | ||
| * @param {Object} textInput the text input to focus | ||
| * @returns {Function} a function that focuses the text input with a configurable delay | ||
| */ | ||
| function focusWithDelay(textInput) { | ||
| /** | ||
| * Focus the text input | ||
| * @param {Boolean} [shouldDelay=false] Impose delay before focusing the text input | ||
| */ | ||
| return (shouldDelay = false) => { | ||
| // There could be other animations running while we trigger manual focus. | ||
| // This prevents focus from making those animations janky. | ||
| InteractionManager.runAfterInteractions(() => { | ||
| if (!textInput) { | ||
| return; | ||
| } | ||
| if (!shouldDelay) { | ||
| textInput.focus(); | ||
| return; | ||
| } | ||
| ComposerFocusManager.isReadyToFocus().then(() => { | ||
| if (!textInput) { | ||
| return; | ||
| } | ||
| textInput.focus(); | ||
| }); | ||
| }); | ||
| }; | ||
| } | ||
|
|
||
| export default focusWithDelay; |
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -372,6 +372,17 @@ function ReportActionCompose({ | |
| focusWithDelay(textInputRef.current)(shouldDelay); | ||
| }, []); | ||
|
|
||
| const isNextModalWillOpenRef = useRef(false); | ||
| const isKeyboardVisibleWhenShowingModalRef = useRef(false); | ||
|
|
||
| const restoreKeyboardState = useCallback(() => { | ||
| if (!isKeyboardVisibleWhenShowingModalRef.current) { | ||
| return; | ||
| } | ||
| focus(true); | ||
| isKeyboardVisibleWhenShowingModalRef.current = false; | ||
| }, [focus]); | ||
|
|
||
| /** | ||
| * Update the value of the comment in Onyx | ||
| * | ||
|
|
@@ -943,7 +954,8 @@ function ReportActionCompose({ | |
| shouldBlockEmojiCalc.current = false; | ||
| shouldBlockMentionCalc.current = false; | ||
| setIsAttachmentPreviewActive(false); | ||
| }, []); | ||
| restoreKeyboardState(); | ||
| }, [restoreKeyboardState]); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribeNavigationBlur = navigation.addListener('blur', () => KeyDownListener.removeKeyDownPressListner(focusComposerOnKeyPress)); | ||
|
|
@@ -982,10 +994,13 @@ function ReportActionCompose({ | |
| const prevIsModalVisible = usePrevious(modal.isVisible); | ||
| const prevIsFocused = usePrevious(isFocusedProp); | ||
| useEffect(() => { | ||
| if (modal.isVisible && !prevIsModalVisible) { | ||
| isNextModalWillOpenRef.current = false; | ||
| } | ||
| // We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused. | ||
| // We avoid doing this on native platforms since the software keyboard popping | ||
| // open creates a jarring and broken UX. | ||
| if (!(willBlurTextInputOnTapOutside && !modal.isVisible && isFocusedProp && (prevIsModalVisible || !prevIsFocused))) { | ||
| if (!(willBlurTextInputOnTapOutside && !isNextModalWillOpenRef.current && !modal.isVisible && isFocusedProp && (prevIsModalVisible || !prevIsFocused))) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -1063,8 +1078,10 @@ function ReportActionCompose({ | |
| shouldBlockEmojiCalc.current = true; | ||
| shouldBlockMentionCalc.current = true; | ||
| } | ||
| isNextModalWillOpenRef.current = true; | ||
| openPicker({ | ||
| onPicked: displayFileInModal, | ||
| onCanceled: restoreKeyboardState, | ||
| }); | ||
| }; | ||
| const menuItems = [ | ||
|
|
@@ -1133,6 +1150,10 @@ function ReportActionCompose({ | |
| ref={actionButtonRef} | ||
| onPress={(e) => { | ||
| e.preventDefault(); | ||
| if (!willBlurTextInputOnTapOutside) { | ||
| isKeyboardVisibleWhenShowingModalRef.current = textInputRef.current.isFocused(); | ||
| } | ||
| textInputRef.current.blur(); | ||
|
|
||
| // Drop focus to avoid blue focus ring. | ||
| actionButtonRef.current.blur(); | ||
|
|
@@ -1150,7 +1171,10 @@ function ReportActionCompose({ | |
| <PopoverMenu | ||
| animationInTiming={CONST.ANIMATION_IN_TIMING} | ||
| isVisible={isMenuVisible} | ||
| onClose={() => setMenuVisibility(false)} | ||
| onClose={() => { | ||
| setMenuVisibility(false); | ||
| restoreKeyboardState(); | ||
| }} | ||
| onItemSelected={(item, index) => { | ||
| setMenuVisibility(false); | ||
|
|
||
|
|
@@ -1185,9 +1209,12 @@ function ReportActionCompose({ | |
| style={[styles.textInputCompose, isComposerFullSize ? styles.textInputFullCompose : styles.flex4]} | ||
| maxLines={maxComposerLines} | ||
| onFocus={() => setIsFocused(true)} | ||
| onBlur={() => { | ||
| onBlur={(e) => { | ||
| setIsFocused(false); | ||
| resetSuggestions(); | ||
| if (e.relatedTarget && e.relatedTarget === actionButtonRef.current) { | ||
| isKeyboardVisibleWhenShowingModalRef.current = true; | ||
|
Member
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. This caused a regression #26489. After setting this value to true we had to reset it to false as soon as a menu item is selected otherwise we will focus the composer for a brief period while opening the RHP. |
||
| } | ||
| }} | ||
| onClick={() => { | ||
| shouldBlockEmojiCalc.current = false; | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.