Fix: Edit composer moves focus to main composer after press LHN menu item #24481
Conversation
|
Thanks! I've updated. |
src/components/Composer/index.js
Outdated
| @@ -354,6 +358,11 @@ function Composer({ | |||
| updateNumberOfLines(); | |||
| }, [updateNumberOfLines]); | |||
|
|
|||
| useEffect(() => () => { | |||
| ReportActionComposeFocusManager.clear(); | |||
| ReportActionComposeFocusManager.focus(); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- Use the
shouldShowComposeInputprop from Onyx. However, there're more reasons to hide the main composer than just switching to edit mode.shouldShowComposeInputdoesn'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.
App/src/pages/home/report/ReportFooter.js
Lines 79 to 82 in bfda5ed
- Another solution is to introduce a prop called
isMainComposertoComposercomponent. 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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Edited: Imma put the second alternative solution to main solution since it's short and not prone to regressions.
There was a problem hiding this comment.
Yeah that does sound like a simpler solution.
There was a problem hiding this comment.
Please take a look.
src/components/Composer/index.js
Outdated
| ReportActionComposeFocusManager.clear(); | ||
| ReportActionComposeFocusManager.focus(); | ||
| }, []); | ||
| useEffect( |
src/components/Composer/index.js
Outdated
| @@ -159,6 +161,8 @@ function Composer({ | |||
| }) { | |||
| const textRef = useRef(null); | |||
| const textInput = useRef(null); | |||
| const willBlurTextInputOnTapOutsideRef = useRef(); | |||
There was a problem hiding this comment.
Just out of curiosity, why did you change this to a ref?
There was a problem hiding this comment.
I think it's better to just directly call the function instead of creating the ref, right? I've updated that.
There was a problem hiding this comment.
Yeah we do it that way in reportActionCompose.
There was a problem hiding this comment.
But do we even need it allall seeing as we are only adding this to composer on web, and willBlurTextInputOnTapOutside only returns true on web. Correct me if I'm wrong but won't it return true 100% of the time here?
There was a problem hiding this comment.
Agree! I was trying to replicate the conditions from ReportActionCompose without noticing that.
| @@ -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 | |||
There was a problem hiding this comment.
| // 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. |
|
I have found one problem, although it is quite strange and I'm not sure it's in scope for this issue. VIDEOScreen.Recording.2023-08-21.at.23.23.26.mov |
Reviewer Checklist
Screenshots/VideosWebMacOS_Chrome.movMobile Web - ChromeAndroid_Chrome.movMobile Web - SafariiOS_Safari.movDesktopMacOS_Desktop.moviOSiOS_Native.movAndroidAndroid_Native.mov |
There was a problem hiding this comment.
Aside from #24481 (comment) which I think is beyond the scope of this PR, changes LGTM.
|
Sorry, I've been investigating that but had no clue why it got blurred. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.57-0 🚀
|
| @@ -448,6 +457,18 @@ function Composer({ | |||
| numberOfLines={numberOfLines} | |||
| disabled={isDisabled} | |||
| onKeyPress={handleKeyPress} | |||
| onFocus={(e) => { | |||
| ReportActionComposeFocusManager.onComposerFocus(() => { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes. Do you think we should do this @Ollyws? if not, why not.
There was a problem hiding this comment.
I don't see why not, but we should make sure to check willBlurTextInputOnTapOutside() so it's not run on native.
There was a problem hiding this comment.
So I thought we should handle the refactor in #25892, right? I'll update my proposal to handle that.
| if (!_.isFunction(focusCallback)) { | ||
| if (!_.isFunction(mainComposerFocusCallback)) { | ||
| return; | ||
| } | ||
|
|
||
| mainComposerFocusCallback(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
if (!_.isFunction(focusCallback) && !_.isFunction(mainComposerFocusCallback)) {
return;
}
if (_.isFunction(mainComposerFocusCallback)) {
mainComposerFocusCallback();
return;
}
focusCallback();This seems more readable to me.
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.58-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.57-6 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.58-5 🚀
|
Details
App loses focus on edit composer and switches to focus main composer after press LHN menu item. This PR fixes that.
Fixed Issues
$ #23898
PROPOSAL: #23898 (comment)
Tests
Offline tests
NA
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-08-12.at.18.33.51.mov
Mobile Web - Chrome
Screen.Recording.2023-08-12.at.23.10.56.mov
Mobile Web - Safari
Screen.Recording.2023-08-12.at.23.08.51.mov
Desktop
Screen.Recording.2023-08-12.at.22.39.54.mov
iOS
Screen.Recording.2023-08-12.at.22.48.26.mov
Android
Screen.Recording.2023-08-12.at.23.10.10.mov