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

Jump to bottom

fix: auto focus and scroll to bottom on large-text in composer #28790

Merged
merged 15 commits into from
Oct 17, 2023
Merged

fix: auto focus and scroll to bottom on large-text in composer #28790

Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
adc3aee
fix: auto focus and scroll to bottom on large-text in composer
jeet-dhandha Oct 4, 2023
72bb8a2
changed shouldAutoFocus to autoFocusCheck in Composer
jeet-dhandha Oct 4, 2023
3828277
updated name for flag and fixed callback dependencies
jeet-dhandha Oct 4, 2023
f070f8b
removed extra code
jeet-dhandha Oct 4, 2023
f189981
removed InteractionManager as it was creating a janky animation
jeet-dhandha Oct 5, 2023
23ff678
Updated comments and variable name for better understanding
jeet-dhandha Oct 6, 2023
2dafa3f
remove unnecessary code.
jeet-dhandha Oct 9, 2023
d79641a
removed `focusWithScrolledToBottom`
jeet-dhandha Oct 9, 2023
317d060
fix: remove extra code and handle web scroll to bottom
jeet-dhandha Oct 10, 2023
5b2d7b9
Merge branch 'main' into fix-27153
jeet-dhandha Oct 10, 2023
82778ee
removed extra useEffect and extra checks
jeet-dhandha Oct 11, 2023
8972e44
updated checks
jeet-dhandha Oct 12, 2023
fbb9dac
Merge branch 'main' into fix-27153
jeet-dhandha Oct 13, 2023
1fbbbb0
Update src/pages/home/report/ReportActionCompose/ComposerWithSuggesti…
jeet-dhandha Oct 13, 2023
ce44e6e
Merge branch 'main' into fix-27153
jeet-dhandha Oct 13, 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
29 changes: 27 additions & 2 deletions src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useEffect, useCallback, useState, useRef, useMemo, useImperativeHandle} from 'react';
import {View, NativeModules, findNodeHandle} from 'react-native';
import {View, NativeModules, findNodeHandle, InteractionManager} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import lodashGet from 'lodash/get';
Expand Down Expand Up @@ -33,6 +33,7 @@ import withKeyboardState from '../../../../components/withKeyboardState';
import {propTypes, defaultProps} from './composerWithSuggestionsProps';
import focusWithDelay from '../../../../libs/focusWithDelay';
import useDebounce from '../../../../hooks/useDebounce';
import updateMultilineInputRange from '../../../../libs/UpdateMultilineInputRange';

const {RNTextInputReset} = NativeModules;

Expand Down Expand Up @@ -111,6 +112,7 @@ function ComposerWithSuggestions({

const isEmptyChat = useMemo(() => _.size(reportActions) === 1, [reportActions]);
const shouldAutoFocus = !modal.isVisible && (shouldFocusInputOnScreenFocus || isEmptyChat) && shouldShowComposeInput;
const shouldAutoFocusWithLongText = shouldAutoFocus && !willBlurTextInputOnTapOutside;

const valueRef = useRef(value);
valueRef.current = value;
Expand All @@ -124,6 +126,7 @@ function ComposerWithSuggestions({

const textInputRef = useRef(null);
const insertedEmojisRef = useRef([]);
const initialFocusedRef = useRef(false);

// 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);
Expand Down Expand Up @@ -445,6 +448,28 @@ function ComposerWithSuggestions({
textInputRef.current.blur();
}, []);

const focusWithScrolledToBottom = useCallback(() => {
// Using `shouldAutoFocus` check to determine whether the component should be focused or not.
if (!textInputRef.current || !willBlurTextInputOnTapOutside || !shouldAutoFocus) {
return;
}

InteractionManager.runAfterInteractions(() => {
updateMultilineInputRange(textInputRef.current);
focus(); // This will focus the composer after its fully rendered.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure why we need to force focus here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

Line 535 in f070f8b

autoFocus={shouldAutoFocusWithLongText}

If you check above line of code we are disabling autoFocus as we wan't to focus manually after scrollHeight and selection are updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

So without that, the selection wouldn't be set to the end, is that what you mean? Or what side-effect is there if we don't add that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2023-10-06.at.3.10.04.PM.mov

Without that Composer won't get Focused. As you can see the Cursor isn't there in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry this still isn't really making sense to me. If I remove your change to autoFocus and just have a simple useEffect that runs updateMultilineInputRange, it works fine (on desktop at least). I'm struggling to understand the reason why all this extra logic is necessary here.

Copy link
Contributor Author

@jeet-dhandha jeet-dhandha Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did you check out my video @jjcoffee ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets have a quick chat in Slack here its harder to chat.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeet-dhandha Yes, is that with autoFocus set back to shouldAutoFocus? I don't get the same behaviour if so...

Copy link
Contributor

Choose a reason for hiding this comment

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

The original issue also doesn't show any problem with focusing on the composer. That's why I'm quite sceptical about adding extra focus calls here!

});
}, [focus, shouldAutoFocus]);

useEffect(() => {
// Added initial focus ref to remove unneccessary focus after first render.
if (initialFocusedRef.current) {
return;
}

initialFocusedRef.current = true;
focusWithScrolledToBottom();
}, [focusWithScrolledToBottom]);

useEffect(() => {
const unsubscribeNavigationBlur = navigation.addListener('blur', () => KeyDownListener.removeKeyDownPressListner(focusComposerOnKeyPress));
const unsubscribeNavigationFocus = navigation.addListener('focus', () => {
Expand Down Expand Up @@ -507,7 +532,7 @@ function ComposerWithSuggestions({
<View style={[containerComposeStyles, styles.textInputComposeBorder]}>
<Composer
checkComposerVisibility={checkComposerVisibility}
autoFocus={shouldAutoFocus}
autoFocus={shouldAutoFocusWithLongText}
multiline
ref={setTextInputRef}
textAlignVertical="top"
Expand Down
Loading