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.

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 flashing while clicking "Add attachment" #23994

Merged
merged 17 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions patches/react-native+0.72.3+004+ModalKeyboardFlashing.patch
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];
+ }
ntdiary marked this conversation as resolved.
Show resolved Hide resolved
[[modalHostView reactViewController] presentViewController:viewController
animated:animated
completion:completionBlock];
13 changes: 11 additions & 2 deletions src/components/AttachmentPicker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ function getAcceptableFileTypes(type) {
function AttachmentPicker(props) {
const fileInput = useRef();
const onPicked = useRef();
const onCanceled = useRef(() => {});

return (
<>
<input
Expand All @@ -46,13 +48,20 @@ function AttachmentPicker(props) {
}}
// We are stopping the event propagation because triggering the `click()` on the hidden input
// causes the event to unexpectedly bubble up to anything wrapping this component e.g. Pressable
onClick={(e) => e.stopPropagation()}
onClick={(e) => {
e.stopPropagation();
if (!fileInput.current) {
return;
}
fileInput.current.addEventListener('cancel', () => onCanceled.current(), {once: true});
}}
accept={getAcceptableFileTypes(props.type)}
/>
{props.children({
openPicker: ({onPicked: newOnPicked}) => {
openPicker: ({onPicked: newOnPicked, onCanceled: newOnCanceled = () => {}}) => {
onPicked.current = newOnPicked;
fileInput.current.click();
onCanceled.current = newOnCanceled;
},
})}
</>
Expand Down
13 changes: 10 additions & 3 deletions src/components/AttachmentPicker/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ function AttachmentPicker({type, children}) {

const completeAttachmentSelection = useRef();
const onModalHide = useRef();
const onCanceled = useRef();

const {translate} = useLocalize();
const {isSmallScreenWidth} = useWindowDimensions();
Expand Down Expand Up @@ -216,9 +217,11 @@ function AttachmentPicker({type, children}) {
* Opens the attachment modal
*
* @param {function} onPickedHandler A callback that will be called with the selected attachment
* @param {function} onCanceledHandler A callback that will be called without a selected attachment
*/
const open = (onPickedHandler) => {
const open = (onPickedHandler, onCanceledHandler = () => {}) => {
completeAttachmentSelection.current = onPickedHandler;
onCanceled.current = onCanceledHandler;
setIsVisible(true);
};

Expand All @@ -239,6 +242,7 @@ function AttachmentPicker({type, children}) {
const pickAttachment = useCallback(
(attachments = []) => {
if (attachments.length === 0) {
onCanceled.current();
return Promise.resolve();
}

Expand Down Expand Up @@ -308,13 +312,16 @@ function AttachmentPicker({type, children}) {
*/
const renderChildren = () =>
children({
openPicker: ({onPicked}) => open(onPicked),
openPicker: ({onPicked, onCanceled: newOnCanceled}) => open(onPicked, newOnCanceled),
});

return (
<>
<Popover
onClose={close}
onClose={() => {
close();
onCanceled.current();
}}
isVisible={isVisible}
anchorPosition={styles.createMenuPosition}
onModalHide={onModalHide.current}
Expand Down
8 changes: 8 additions & 0 deletions src/components/Modal/BaseModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -73,6 +74,9 @@ class BaseModal extends PureComponent {
this.props.onModalHide();
}
Modal.onModalDidClose();
if (!this.props.fullscreen) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this condition for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure that the pending promise can also be settled when coverScreen is false, because onDismiss will not be emitted in this case. (It only exists in the RN Modal component.)
image

Copy link
Contributor

Choose a reason for hiding this comment

The 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:
Why was it necessary to put the ComposerFocusManager.setReadyToFocus() logic into both onDismiss and onModalHide (which calls hideModal)?
Was there any drawback to just calling it in hideModal regardless of the fullscreen condition? This would mean removing the ReactNativeModal.onDismiss prop below.
I'm asking because we've recently discovered another case when the onDismiss is not called, this time for a full-screen Attachments modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paultsimura, when using the Modal component, the invocation of onModalHide happens earlier than the destruction of the focus trap, which will cause the composer not gaining focus correctly after the modal is dismissed. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ntdiary. Do you have any particular Modal scenario in mind that might break?
I tried with only the hideModal: emoji picker, attachments modal, the side drawer – everything focuses the composer correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. :)

Copy link
Contributor

@paultsimura paultsimura Nov 20, 2023

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I added a new ModalContent, I feel it should fix that issue.

ComposerFocusManager.setReadyToFocus();
}
}

render() {
Expand Down Expand Up @@ -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);
Expand All @@ -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}
Expand Down
10 changes: 10 additions & 0 deletions src/components/Modal/index.android.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@
import React from 'react';
import {AppState} from 'react-native';
import withWindowDimensions from '../withWindowDimensions';
import BaseModal from './BaseModal';
import {propTypes, defaultProps} from './modalPropTypes';
import ComposerFocusManager from '../../libs/ComposerFocusManager';

AppState.addEventListener('focus', () => {
ComposerFocusManager.setReadyToFocus();
});

AppState.addEventListener('blur', () => {
ComposerFocusManager.resetReadyToFocus();
});

// Only want to use useNativeDriver on Android. It has strange flashes issue on IOS
// https://github.com/react-native-modal/react-native-modal#the-modal-flashes-in-a-weird-way-when-animating
Expand Down
23 changes: 23 additions & 0 deletions src/libs/ComposerFocusManager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
let isReadyToFocusPromise = Promise.resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this logic on web at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the Modal component in react-native-web has focus trap. This code, together with InteractionManager.runAfterInteractions, can also ensure that the focus is called safely.

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,
};
35 changes: 35 additions & 0 deletions src/libs/focusWithDelay.js
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;
40 changes: 0 additions & 40 deletions src/libs/focusWithDelay/focusWithDelay.js

This file was deleted.

7 changes: 0 additions & 7 deletions src/libs/focusWithDelay/index.js

This file was deleted.

6 changes: 0 additions & 6 deletions src/libs/focusWithDelay/index.native.js

This file was deleted.

35 changes: 31 additions & 4 deletions src/pages/home/report/ReportActionCompose.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -1063,8 +1078,10 @@ function ReportActionCompose({
shouldBlockEmojiCalc.current = true;
shouldBlockMentionCalc.current = true;
}
isNextModalWillOpenRef.current = true;
openPicker({
onPicked: displayFileInModal,
onCanceled: restoreKeyboardState,
});
};
const menuItems = [
Expand Down Expand Up @@ -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();
Expand All @@ -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);

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
Loading