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: Edit composer moves focus to main composer after press LHN menu item #24481

Merged
merged 16 commits into from
Aug 22, 2023
Merged
21 changes: 21 additions & 0 deletions src/components/Composer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import Text from '../Text';
import isEnterWhileComposition from '../../libs/KeyboardShortcut/isEnterWhileComposition';
import CONST from '../../CONST';
import withNavigation from '../withNavigation';
import ReportActionComposeFocusManager from "../../libs/ReportActionComposeFocusManager";
import willBlurTextInputOnTapOutside from "../../libs/willBlurTextInputOnTapOutside";

const propTypes = {
/** Maximum number of lines in the text input */
Expand Down Expand Up @@ -159,6 +161,8 @@ function Composer({
}) {
const textRef = useRef(null);
const textInput = useRef(null);
const willBlurTextInputOnTapOutsideRef= useRef();
willBlurTextInputOnTapOutsideRef.current = willBlurTextInputOnTapOutside();
const initialValue = defaultValue ? `${defaultValue}` : `${value || ''}`;
const [numberOfLines, setNumberOfLines] = useState(numberOfLinesProp);
const [selection, setSelection] = useState({
Expand Down Expand Up @@ -354,6 +358,11 @@ function Composer({
updateNumberOfLines();
}, [updateNumberOfLines]);

useEffect(() => () => {
ReportActionComposeFocusManager.clear();
ReportActionComposeFocusManager.focus();
Copy link
Contributor

@Ollyws Ollyws Aug 19, 2023

Choose a reason for hiding this comment

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

Thanks for the update. Can you tell me, why is .focus() necessary here?
Also, wouldn't it be better to put this in our already existing useEffect return function?

Copy link
Contributor Author

@tienifr tienifr Aug 20, 2023

Choose a reason for hiding this comment

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

Why is .focus() necessary here?

After clearing the callback we need to re-focus on the main composer. It's the same logic just like within ReportActionItemMessageEdit's deleteDraft.

const deleteDraft = useCallback(() => {
    debouncedSaveDraft.cancel();
    Report.saveReportActionDraft(props.reportID, props.action, '');
    ComposerActions.setShouldShowComposeInput(true);
    ReportActionComposeFocusManager.clear();
    ReportActionComposeFocusManager.focus();
    ...
}

wouldn't it be better to put this in our already existing useEffect

Thanks, I've updated that!

Copy link
Contributor

Choose a reason for hiding this comment

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

@tienifr The problem with using .focus() here is that if you open two edit composers, and delete the unfocused one from another tab, the focus will be moved from the edit composer to the main composer.
The current behaviour on production is not to focus after deleting the comment from another tab so I think we can remove the .focus() here and keep the current production behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that's what we want to fix here? That the main composer should be focused when edit composer is closed/unmounted? Whichever approach we choose should be noted so that later issues wouldn't treat it as a regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it should be focused, just that the correct callback is used when we select something from the LHN context-menu and aren't left with any stale callbacks after a comment has been deleted. Simply clearing on unmount seems to fix this as far as I can see.

Copy link
Contributor Author

@tienifr tienifr Aug 21, 2023

Choose a reason for hiding this comment

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

The main point is to check whether the main composer is being unmounted.

Solution

Check if mainComposerFocusCallback has already been cleared before, this means that the main composer has been unmounted:

if (isMainComposer) {
    mainComposerFocusCallback = null;
} else if (mainComposerFocusCallback) {
    focusCallback = null;
}

This seems like a workaround and needs more testing but is the shortest solution.

Alternative solution

  1. Use the shouldShowComposeInput prop from Onyx. However, there're more reasons to hide the main composer than just switching to edit mode. shouldShowComposeInput doesn't cover all cases of un-mounting. For example: report with disabled write actions. Also, subscribing to Onyx may introduce unexpected re-rendering and performance decrease.

{!hideComposer && (props.shouldShowComposeInput || !props.isSmallScreenWidth) && (
<View style={[chatFooterStyles, props.isComposerFullSize && styles.chatFooterFullCompose]}>
<SwipeableView onSwipeDown={Keyboard.dismiss}>
<ReportActionCompose

  1. Another solution is to introduce a prop called isMainComposer to Composer component. We only clear if it's an edit composer.

I've already pushed the changes for the first solution. Please take a look and leave some feedback! @Ollyws

Copy link
Contributor

@Ollyws Ollyws Aug 21, 2023

Choose a reason for hiding this comment

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

@tienifr So we'd only be clearing the edit composer callback when the main composer is visible? Won't this cause a problem if we deleted an edit message from another tab on mWeb? (while the main composer is hidden)
Edit: Actually, seems to auto-focus the main composer when we do that on mWeb. Let me test this further.

Copy link
Contributor Author

@tienifr tienifr Aug 21, 2023

Choose a reason for hiding this comment

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

Edited: Imma put the second alternative solution to main solution since it's short and not prone to regressions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that does sound like a simpler solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a look.

}, []);

useEffect(() => {
// we need to handle listeners on navigation focus/blur as Composer is not unmounting
// when navigating away to different report
Expand Down Expand Up @@ -448,6 +457,18 @@ function Composer({
numberOfLines={numberOfLines}
disabled={isDisabled}
onKeyPress={handleKeyPress}
onFocus={(e) => {
ReportActionComposeFocusManager.onComposerFocus(() => {
Copy link
Member

@parasharrajat parasharrajat Aug 27, 2023

Choose a reason for hiding this comment

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

We shouldn't be using this onComposerFocus here. The composer component does seem related to the composer only but it is a standalone component. This logic adds a side-effect that will become hard to maintain over time.

There is no main or general composer inside composer/index.js. We handled this inside composer/index.js (web variant) but what about the native variant?

Can you optimize this solution keeping scalability in mind?

Can we handle this ReportActionCompose instead given that we allow onFocus prop as well as forward ref to inner textinput?

cc: @Ollyws

Copy link
Contributor

@Ollyws Ollyws Aug 30, 2023

Choose a reason for hiding this comment

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

Thanks for the feedback. Yes good point, we could pass the function to Composer from ReportActionCompose but we would also need to pass it in ReportActionItemMessageEdit.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Do you think we should do this @Ollyws? if not, why not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why not, but we should make sure to check willBlurTextInputOnTapOutside() so it's not run on native.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I thought we should handle the refactor in #25892, right? I'll update my proposal to handle that.

Copy link
Member

Choose a reason for hiding this comment

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

That works for me.

if (!willBlurTextInputOnTapOutsideRef.current || !textInput.current) {
return;
}

textInput.current.focus();
});
if (props.onFocus) {
props.onFocus(e);
}
}}
/>
{shouldCalculateCaretPosition && renderElementForCaretPosition}
</>
Expand Down
25 changes: 21 additions & 4 deletions src/libs/ReportActionComposeFocusManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,23 @@ import _ from 'underscore';
import React from 'react';

const composerRef = React.createRef();
// There are two types of composer: general composer (edit composer) and main composer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// There are two types of composer: general composer (edit composer) and main composer
// There are two types of composer: general composer (edit composer) and main composer.
// The general composer callback will take priority if it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

let focusCallback = null;
let mainComposerFocusCallback = null;

/**
* Register a callback to be called when focus is requested.
* Typical uses of this would be call the focus on the ReportActionComposer.
*
* @param {Function} callback callback to register
* @param {Boolean} isMainComposer
*/
function onComposerFocus(callback) {
focusCallback = callback;
function onComposerFocus(callback, isMainComposer = false) {
if (isMainComposer) {
mainComposerFocusCallback = callback;
} else {
focusCallback = callback;
}
}

/**
Expand All @@ -20,6 +27,11 @@ function onComposerFocus(callback) {
*/
function focus() {
if (!_.isFunction(focusCallback)) {
if (!_.isFunction(mainComposerFocusCallback)) {
return;
}

mainComposerFocusCallback();
return;
}
Comment on lines 30 to 37
Copy link
Member

@parasharrajat parasharrajat Aug 27, 2023

Choose a reason for hiding this comment

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

if (!_.isFunction(focusCallback) && !_.isFunction(mainComposerFocusCallback)) {
     return;
   }
if (_.isFunction(mainComposerFocusCallback)) {	
     mainComposerFocusCallback();
     return;
   }
   focusCallback();

This seems more readable to me.


Expand All @@ -29,9 +41,14 @@ function focus() {
/**
* Clear the registered focus callback
*
* @param {Boolean} isMainComposer
*/
function clear() {
focusCallback = null;
function clear(isMainComposer = false) {
robertjchen marked this conversation as resolved.
Show resolved Hide resolved
if (isMainComposer) {
mainComposerFocusCallback = null;
} else {
focusCallback = null;
}
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/pages/home/report/ReportActionCompose.js
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ function ReportActionCompose({
}

focus(false);
});
}, true);
}, [focus, isFocusedProp]);

/**
Expand Down Expand Up @@ -970,7 +970,7 @@ function ReportActionCompose({
}

return () => {
ReportActionComposeFocusManager.clear();
ReportActionComposeFocusManager.clear(true);

KeyDownListener.removeKeyDownPressListner(focusComposerOnKeyPress);
unsubscribeNavigationBlur();
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/report/ReportActionItemMessageEdit.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ function ReportActionItemMessageEdit(props) {
debouncedSaveDraft.cancel();
Report.saveReportActionDraft(props.reportID, props.action, '');
ComposerActions.setShouldShowComposeInput(true);
ReportActionComposeFocusManager.clear();
ReportActionComposeFocusManager.focus();

// Scroll to the last comment after editing to make sure the whole comment is clearly visible in the report.
Expand Down