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 29405 cursor displayed before emoji on ios native #30835

Merged
merged 15 commits into from
Dec 28, 2023
Merged

Fix 29405 cursor displayed before emoji on ios native #30835

Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e8a054a
Fix 29405 cursor displayed before emoji on ios native
roksanaz Nov 3, 2023
15e4590
Merge branch 'main' into 29405-ios-emoji-cursor
roksanaz Nov 3, 2023
2c6b1f1
Merge branch 'main' into 29405-ios-emoji-cursor
roksanaz Nov 6, 2023
bd9ab69
Merge branch 'main' into 29405-ios-emoji-cursor
roksanaz Nov 6, 2023
5359e09
Merge branch 'main' into 29405-ios-emoji-cursor
artus9033 Nov 15, 2023
b506758
Merge branch 'main' of github.com:Expensify/App into proposal/29405
artus9033 Nov 23, 2023
0f098ad
Merge branch 'main' of github.com:Expensify/App into proposal/29405
artus9033 Nov 29, 2023
b107b9c
Fix Composer not positioning caret properly on iOS after substituting…
artus9033 Dec 5, 2023
6d899c3
Merge branch 'main' of github.com:Expensify/App into proposal/29405
artus9033 Dec 5, 2023
e2480e5
Added comments, additional conditions, refactored fix in ComposerWith…
artus9033 Dec 6, 2023
a0b31b5
Fixed typo in ComposerWithSuggestions fix
artus9033 Dec 6, 2023
fc88d76
Merge branch 'main' of github.com:Expensify/App into proposal/29405
artus9033 Dec 7, 2023
062265e
Refactored ComposerWithSuggestions
artus9033 Dec 7, 2023
b2ff88c
Stripped obsolete code from ComposerWithSuggestions.js
artus9033 Dec 13, 2023
98436b7
Added explanatory commit to ComposerWithSuggestions.js
artus9033 Dec 20, 2023
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
32 changes: 30 additions & 2 deletions src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {useIsFocused, useNavigation} from '@react-navigation/native';
import lodashGet from 'lodash/get';
import React, {useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react';
import {findNodeHandle, NativeModules, View} from 'react-native';
import {findNodeHandle, InteractionManager, NativeModules, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import Composer from '@components/Composer';
Expand All @@ -17,6 +17,7 @@ import getDraftComment from '@libs/ComposerUtils/getDraftComment';
import convertToLTRForComposer from '@libs/convertToLTRForComposer';
import * as EmojiUtils from '@libs/EmojiUtils';
import focusComposerWithDelay from '@libs/focusComposerWithDelay';
import getPlatform from '@libs/getPlatform';
import * as KeyDownListener from '@libs/KeyboardShortcut/KeyDownPressListener';
import ReportActionComposeFocusManager from '@libs/ReportActionComposeFocusManager';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
Expand All @@ -39,6 +40,8 @@ import {defaultProps, propTypes} from './composerWithSuggestionsProps';

const {RNTextInputReset} = NativeModules;

const isIOSNative = getPlatform() === CONST.PLATFORM.IOS;

/**
* Broadcast that the user is typing. Debounced to limit how often we publish client events.
* @param {String} reportID
Expand Down Expand Up @@ -133,6 +136,8 @@ function ComposerWithSuggestions({
const textInputRef = useRef(null);
const insertedEmojisRef = useRef([]);

const syncSelectionWithOnChangeTextRef = useRef(null);

// A flag to indicate whether the onScroll callback is likely triggered by a layout change (caused by text change) or not
const isScrollLikelyLayoutTriggered = useRef(false);
const suggestions = lodashGet(suggestionsRef, 'current.getSuggestions', () => [])();
Expand Down Expand Up @@ -232,6 +237,11 @@ function ComposerWithSuggestions({
setValue(newCommentConverted);
if (commentValue !== newComment) {
const position = Math.max(selection.end + (newComment.length - commentRef.current.length), cursorPosition || 0);

if (isIOSNative) {
roksanaz marked this conversation as resolved.
Show resolved Hide resolved
syncSelectionWithOnChangeTextRef.current = {position, value: newComment};
}

setSelection({
start: position,
end: position,
Expand Down Expand Up @@ -364,6 +374,24 @@ function ComposerWithSuggestions({
[isKeyboardShown, isSmallScreenWidth, parentReportActions, report, reportActions, reportID, handleSendMessage, suggestionsRef, valueRef],
);

const onChangeText = useCallback(
(commentValue) => {
updateComment(commentValue, true);

if (isIOSNative && syncSelectionWithOnChangeTextRef.current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isIOSNative && may be redundant as syncSelectionWithOnChangeTextRef.current will never be set in all other platforms

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely, this is true - however, since this has negligent impact on performance, I wanted it to stay there as a defensive condition for the code to be clean & stable. Please consider a situation when something that sets this ref is introduced in the future, in which case this condition would still prevent the code from being executed and also it explicitly shows that this is only to be set on iOS native. Would you agree to leave this here, then?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok leave it then

const positionSnapshot = syncSelectionWithOnChangeTextRef.current.position;
syncSelectionWithOnChangeTextRef.current = null;

InteractionManager.runAfterInteractions(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comment why InteractionManager.runAfterInteractions is needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean in code? It is required to ensure that the selection is set imperatively after all state changes are effective; without it, the problem would still persist since setSelection would run too eager.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean in code?

yes, please as InteractionManager.runAfterInteractions is workaround solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing. Resolved in 98436b7

// note: this implementation is only available on non-web RN, thus the wrapping
// 'if' block contains a redundant (since the ref is only used on iOS) platform check
textInputRef.current.setSelection(positionSnapshot, positionSnapshot);
});
}
},
[updateComment],
);

const onSelectionChange = useCallback(
(e) => {
if (textInputRef.current && textInputRef.current.isFocused() && suggestionsRef.current.onSelectionChange(e)) {
Expand Down Expand Up @@ -524,7 +552,7 @@ function ComposerWithSuggestions({
ref={setTextInputRef}
placeholder={inputPlaceholder}
placeholderTextColor={theme.placeholderText}
onChangeText={(commentValue) => updateComment(commentValue, true)}
onChangeText={onChangeText}
onKeyPress={triggerHotkeyActions}
textAlignVertical="top"
style={[styles.textInputCompose, isComposerFullSize ? styles.textInputFullCompose : styles.textInputCollapseCompose]}
Expand Down
Loading