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

Merged
merged 16 commits into from
Aug 22, 2023
Merged

Fix: Edit composer moves focus to main composer after press LHN menu item #24481

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
994fd6f
fix: 23898
tienifr Aug 8, 2023
f5b834f
Merge branch 'main' of https://github.com/tienifr/App into fix/23898
tienifr Aug 12, 2023
9480a6c
add comment and remove logs
tienifr Aug 12, 2023
39b3902
add null check
tienifr Aug 12, 2023
d3ec5e4
clear callback when exit draft mode
tienifr Aug 12, 2023
c7fb2ff
Merge branch 'main' into fix/23898
tienifr Aug 15, 2023
76c3710
clear callback on unmount
tienifr Aug 18, 2023
bc9b0d6
Merge branch 'main' into fix/23898
tienifr Aug 19, 2023
3a64c6e
move to existing cleanup func
tienifr Aug 20, 2023
56ceb26
remove redundant cleanup
tienifr Aug 20, 2023
0f744e4
do not re-focus on unmount
tienifr Aug 20, 2023
fafeeaf
remove ref
tienifr Aug 20, 2023
9b8f563
remove redundant check
tienifr Aug 20, 2023
a623c7d
modify comment
tienifr Aug 21, 2023
9add8d1
clear focus callback only in case of edit compose
tienifr Aug 21, 2023
9005a8a
only clear in case of edit composer
tienifr Aug 21, 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
14 changes: 14 additions & 0 deletions src/components/Composer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Text from '../Text';
import isEnterWhileComposition from '../../libs/KeyboardShortcut/isEnterWhileComposition';
import CONST from '../../CONST';
import withNavigation from '../withNavigation';
import ReportActionComposeFocusManager from '../../libs/ReportActionComposeFocusManager';

const propTypes = {
/** Maximum number of lines in the text input */
Expand Down Expand Up @@ -370,6 +371,7 @@ function Composer({
}

return () => {
ReportActionComposeFocusManager.clear();
unsubscribeFocus();
unsubscribeBlur();
document.removeEventListener('paste', handlePaste);
Expand Down Expand Up @@ -448,6 +450,18 @@ function Composer({
numberOfLines={numberOfLines}
disabled={isDisabled}
onKeyPress={handleKeyPress}
onFocus={(e) => {
ReportActionComposeFocusManager.onComposerFocus(() => {
Copy link
Member

@parasharrajat parasharrajat Aug 27, 2023 •
edited
Loading

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 •
edited
Loading

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 (!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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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
View file
Open in desktop
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