[$500] Edit message, save/cancel and visiting different report removes focus from composer #29919

m-natarajan · 2023-10-18T21:51:10Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Version Number: 1.3.86-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697650111645879

Action Performed:

Open the app
Open any report
Send any message
Edit the message
Save or cancel the edit message
Visit another report and observe that app does not focus on composer

Expected Result:

App should focus on composer whenever we visit any report

Actual Result:

App does not focus on composer when we edit any message, save/cancel the mesage and visit any other report

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Android: Native

Android.chrome.no.focus.on.composer.mp4

Android: mWeb Chrome

Android.app.no.focus.composer.mp4

iOS: Native

ios.native.no.focus.on.composer.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

mac.chrome.no.focus.on.composer.mov

Recording.124.mp4

MacOS: Desktop

mac.desktop.no.focus.on.composer.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e2dccea0afb54c13
Upwork Job ID: 1714761073301872640
Last Price Increase: 2023-11-08

melvin-bot · 2023-10-18T21:51:15Z

Triggered auto assignment to @peterdbarkerUK (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot · 2023-10-18T21:51:16Z

Job added to Upwork: https://www.upwork.com/jobs/~01e2dccea0afb54c13

melvin-bot · 2023-10-18T21:51:17Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-10-18T21:51:21Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External)

zukilover · 2023-10-18T22:54:36Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Composer is not focused when user has edited a chat from different report

What is the root cause of that problem?

Within deleteDraft which is called in the triggerSaveOrCancel method, composer focused is set to false thus loses its focus state:

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 275 to 276 in 1ed55e0

    
           const deleteDraft = useCallback(() => { 
        
               InputFocus.callback(() => setIsFocused(false));

isFocused triggers composerFocusKeepFocusOn on an effect that then read from the state value to remove the focus ref.

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 136 to 138 in 1ed55e0

    
           useEffect(() => { 
        
               InputFocus.composerFocusKeepFocusOn(textInputRef.current, isFocused, modal, onyxFocused); 
        
           }, [isFocused, modal, onyxFocused]);

What changes do you think we should make in order to solve the problem?

Remove the offending line that triggers the focus state change:

const deleteDraft = useCallback(() => {
        // InputFocus.callback(() => setIsFocused(false)); << remove this line
        InputFocus.inputFocusChange(false);

What alternative solutions did you explore? (Optional)

N/A

abzokhattab · 2023-10-18T22:59:22Z

is it a regression from #29073 ?

ayazalavi · 2023-10-20T13:28:57Z

@narefyev91 please take a look

hoangzinh · 2023-10-21T02:27:43Z

@ayazalavi can you confirm that it's a regression from your PR #29073?

ayazalavi · 2023-10-21T06:52:09Z

I am not sure, let me take a look.

hoangzinh · 2023-10-24T01:31:00Z

@ayazalavi is there any luck so far? If I comment out the line as mentioned in @zukilover's proposal, this issue will no longer be reproducible.

peterdbarkerUK · 2023-10-24T13:35:34Z

Is this intended behaviour? From here:

On small devices the auto focus will bring up the keyboard which may be annoying. Reports are optimized for reading. Ref: https://expensify.slack.com/archives/C01GTK53T8Q/p1617201995245400

hoangzinh · 2023-10-24T16:19:34Z

@peterdbarkerUK Yes, it's true on small-screen devices. But we still maintain auto-focus on Web/Desktop.

melvin-bot · 2023-10-25T16:00:07Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot · 2023-10-30T12:54:26Z

@hoangzinh, @peterdbarkerUK Eep! 4 days overdue now. Issues have feelings too...

hoangzinh · 2023-10-30T15:01:32Z

@peterdbarkerUK the Prod regression period is over on issue #26527. Do you agree to accept new proposals to fix this issue here?

peterdbarkerUK · 2023-10-31T14:54:35Z

Yes, please. If @zukilover's proposal still works to your liking? Looks like an extremely easy one.

ayazalavi · 2023-10-31T15:23:16Z

Let me please write down the proposal as well. Since apparently my PR is connected with this bug.

melvin-bot · 2023-11-01T13:20:36Z

@hoangzinh @peterdbarkerUK this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2023-11-01T16:00:28Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

hoangzinh · 2023-11-02T08:54:57Z

@zukilover Thanks for your proposal #29919 (comment). I think you pointed out the correct place that causes this issue. But you haven't explained why we need that line from the beginning and if we remove this line, will it cause any regression bug?

zukilover · 2023-11-02T09:07:52Z

That specific line was added from #26527 to fix a glitch of the focus states (race condition).

It should be fixed with the new inputFocusChange, and thus the focus callback is a redundant line here that should be removed.

ayazalavi · 2023-11-02T10:11:11Z

it will cause regression if we remove this line and the bug mentioned in the #26527 will reappear.

…

On Thu, Nov 2, 2023 at 1:55 PM Vinh Hoang ***@***.***> wrote: @zukilover <https://github.com/zukilover> Thanks for your proposal #29919 (comment) <#29919 (comment)>. I think you pointed out the correct place that causes this issue. But you haven't explained why we need that line from the beginning and if we remove this line, will it cause any regression bug? — Reply to this email directly, view it on GitHub <#29919 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAF3LTZFSHJJMCEQIYGBPGTYCNNW3AVCNFSM6AAAAAA6GGGOZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGMYTGNBWGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ayazalavi · 2023-11-02T10:12:06Z

basically thats why I requested to put proposal but I am still not able to find fix for it so far.

…

On Thu, Nov 2, 2023 at 3:10 PM ayaz alavi ***@***.***> wrote: it will cause regression if we remove this line and the bug mentioned in the #26527 will reappear. On Thu, Nov 2, 2023 at 1:55 PM Vinh Hoang ***@***.***> wrote: > @zukilover <https://github.com/zukilover> Thanks for your proposal #29919 > (comment) > <#29919 (comment)>. > I think you pointed out the correct place that causes this issue. But you > haven't explained why we need that line from the beginning and if we remove > this line, will it cause any regression bug? > > — > Reply to this email directly, view it on GitHub > <#29919 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAF3LTZFSHJJMCEQIYGBPGTYCNNW3AVCNFSM6AAAAAA6GGGOZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGMYTGNBWGE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

hoangzinh · 2023-11-03T14:33:37Z

It should be fixed with the new inputFocusChange, and thus the focus callback is a redundant line here that should be removed.

@zukilover, because both InputFocus.callback(() => setIsFocused(false)) and inputFocusChange are added in the same PR #29073. Could you elaborate on why you think it's a redundant line? Thanks

zukilover · 2023-11-03T14:47:56Z

If we take a look at how composerFocusKeepFocusOn is implemented, especially these lines:

App/src/libs/actions/InputFocus/index.website.ts

Lines 16 to 26 in 17ce0c2

    
               if (isFocused) { 
        
                   refSave = ref; 
        
               } 
        
               if (!isFocused && !onyxFocused && !modal.willAlertModalBecomeVisible && !modal.isVisible && refSave) { 
        
                   if (!ReportActionComposeFocusManager.isFocused()) { 
        
                       refSave.focus(); 
        
                   } else { 
        
                       refSave = undefined; 
        
                   } 
        
               } 
        
           }

It tries to focus on refSave but refSave will be defined if isFocused is truthy, but the focusing is executed only when isFocused is falsy. This seems to be counterintuitive.

ayazalavi · 2023-11-03T15:47:13Z

Proposal

Restating the Problem

The problem we are addressing is that when a user edits a message in a chat and then switches to another chat, the main composer does not receive focus.

Identifying the Root Cause

The root cause of this issue is that the focus on the compose field is triggered programmatically in ComposerWithSuggestions at this location:

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

Line 550 in bc9cb8f

focus();

.

The issue arises when the edit compose box loses focus after closing the edit input field. As a result, the main compose box in the same chat where the message was edited regains focus. When we switch to another chat, this line

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

Line 542 in bc9cb8f

    
           if (!(willBlurTextInputOnTapOutside && !isNextModalWillOpenRef.current && !modal.isVisible && isFocused && (prevIsModalVisible || !prevIsFocused))) {

evaluates prevIsFocused===true, causing it to skip the focus() method call at

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

Line 550 in bc9cb8f

focus();

.

Consequently, the main compose box loses focus in the new chat.

Proposed Solution

To solve this problem, we suggest calling the focus method at

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

Lines 614 to 620 in bc9cb8f

    
           onLayout={(e) => { 
        
               const composerLayoutHeight = e.nativeEvent.layout.height; 
        
               if (composerHeight === composerLayoutHeight) { 
        
                   return; 
        
               } 
        
               setComposerHeight(composerLayoutHeight); 
        
           }}

.

This will ensure that the main compose box receives focus as soon as it's layout'd, preventing the issue from occurring.

Result:

91588771-ee1c-49e9-8e01-db48b37709cd.mp4

Alternative Solutions

No alternative solutions were explored or mentioned in the proposal.

bernhardoj · 2023-11-06T16:16:20Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Visiting another report after saving/canceling an edit message composer does not auto-focus the main composer.

What is the root cause of that problem?

We have a condition here to decide whether we should auto-focus the composer or not.

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

Line 129 in d75acaf

    
           const shouldAutoFocus = !modal.isVisible && (shouldFocusInputOnScreenFocus || (isEmptyChat && !ReportActionsUtils.isTransactionThread(parentAction))) && shouldShowComposeInput;

The problem is with the shouldShowComposeInput. One of the purposes of shouldShowComposeInput is to hide the main composer when an edit composer is focused, on a small screen

When we start editing a message, the edit composer will be focused, and shouldShowComposeInput is set to false.

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 407 to 410 in d75acaf

    
           onFocus={() => { 
        
               setIsFocused(true); 
        
               reportScrollManager.scrollToIndex(props.index, true); 
        
               setShouldShowComposeInputKeyboardAware(false);

When we save/cancel it, it will be unmounted, and it is supposed to reset shouldShowComposeInput back to true.

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 186 to 204 in d75acaf

    
               return () => { 
        
                   // Skip if the current report action is not active 
        
                   if (!isActive()) { 
        
                       return; 
        
                   } 
        
                   if (EmojiPickerAction.isActive(props.action.reportActionID)) { 
        
                       EmojiPickerAction.clearActive(); 
        
                   } 
        
                   if (ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) { 
        
                       ReportActionContextMenu.clearActiveReportAction(); 
        
                   } 
        
                   // Show the main composer when the focused message is deleted from another client 
        
                   // to prevent the main composer stays hidden until we swtich to another chat. 
        
                   setShouldShowComposeInputKeyboardAware(true); 
        
               }; 
        
               // eslint-disable-next-line react-hooks/exhaustive-deps -- this cleanup needs to be called only on unmount 
        
           }, [props.action.reportActionID]);

The problem is, isActive() is always false because isFocusedRef is false
https://github.com/Expensify/App/blob/d75acafec3e24cfa6bc48a20822f9d36763b71a0/src/pages/home/report/ReportActionItemMessageEdit.js#L166-L168R

and that is because we set it to false every time we call deleteDraft (both saving/canceling draft will call this) which is added by this PR

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 280 to 281 in d75acaf

    
           const deleteDraft = useCallback(() => { 
        
               InputFocus.callback(() => setIsFocused(false));

So, shouldShowComposeInput is still false even after we close the edit composer. When we switch to another report, autoFocus is now false because shouldShowComposeInput is false.

We actually have a logic to always reset shouldShowComposeInput to true here

App/src/pages/home/ReportScreen.js

Lines 276 to 278 in d75acaf

    
           useEffect(() => { 
        
               fetchReportIfNeeded(); 
        
               ComposerActions.setShouldShowComposeInput(true);

but as you know, it takes time to update the Onyx.

What changes do you think we should make in order to solve the problem?

IMO, the offending PR (#29073) solution that is trying to solve this issue is a bit hard to read. The behavior that the PR wants to achieve is to refocus back to the last focused composer, in that issue case is the edit composer.

We already have a utility for that called ReportActionComposeFocusManager which we can reuse in our case. So, my suggestion is:

"Revert" the code from added composer focus glitch fix #29073
When a modal closes or the screen regains focused, call ReportActionComposeFocusManager.focus(); instead of the local focus() function.

if (!(willBlurTextInputOnTapOutside && !isNextModalWillOpenRef.current && !modal.isVisible && isFocused && (prevIsModalVisible || !prevIsFocused))) {
    return;
}

-focus();
+ReportActionComposeFocusManager.focus();

Super simple!

melvin-bot · 2023-11-07T13:52:48Z

@hoangzinh, @peterdbarkerUK Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

peterdbarkerUK · 2023-11-07T16:48:42Z

Not overdue, just waiting on proposal review

melvin-bot · 2023-11-08T13:52:09Z

@hoangzinh @peterdbarkerUK this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot · 2023-11-08T16:00:39Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

hoangzinh · 2023-11-09T16:03:20Z

On my list to review today

hoangzinh · 2023-11-13T07:24:08Z

Update: still reading the previous issue #26527 to understand their changes.

ayazalavi · 2023-11-13T07:27:42Z

@Vinh hoang Simply calling focus on main composer layout callback fixes the issue. The issue is due to bug in main composer component as I said in my proposal. Focus doesn’t gets called in a condition when it was already focused without popover getting rendered. If popover gets rendered then to us gets called correctly and this bug doesn’t happen.

…

On Nov 13, 2023 at 12:24 PM, <Vinh Hoang ***@***.***)> wrote: Update: still reading the previous issue #26527 (#26527) to understand their changes. — Reply to this email directly, view it on GitHub (#29919 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAF3LT6XKF372C75I6UB7YLYEHDKFAVCNFSM6AAAAAA6GGGOZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGU4TCMBYGQ). You are receiving this because you were mentioned.Message ID: ***@***.***>

hoangzinh · 2023-11-13T07:43:42Z

Somehow I couldn't reproduce the issue in dev/staging. Is there anyone able to reproduce the issue now?

Screen.Recording.2023-11-13.at.14.39.49.mov

bernhardoj · 2023-11-13T09:12:03Z

Looks like it's no longer reproducible after the rn-web upgrade.

peterdbarkerUK · 2023-11-13T14:19:46Z

+1 I can no longer reproduce. I'm checking how compensation works in this scenario (personal paper trial)

ayazalavi · 2023-11-13T14:32:42Z

My ticket was marked with regression. Should I be paid for compensating that?

…

On Nov 13, 2023 at 7:19 PM, <peterdbarkerUK ***@***.***)> wrote: +1 I can no longer reproduce. I'm checking how compensation works in this scenario (personal paper trial (https://expensify.slack.com/archives/C01SKUP7QR0/p1699885147013969)) — Reply to this email directly, view it on GitHub (#29919 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAF3LTYAGBIPRNE3XKIR7PLYEIUA5AVCNFSM6AAAAAA6GGGOZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYGI2TEOBXGY). You are receiving this because you were mentioned.Message ID: ***@***.***>

peterdbarkerUK · 2023-11-14T08:56:46Z

@ayazalavi I'm not sure I understand your question, could you clarify? Thanks!

peterdbarkerUK · 2023-11-15T10:26:58Z

Hey folks - first up, I really appreciate all your work making and reviewing proposals for this.

As this has been resolved by the rn-web update, I'm going to close it out without paying bounties. The reason being that this issue was solved "in the course of things" unrelated to this issue, without work needing to be done. I understand this can be frustrating, though with the issues that are quick and easy to resolve I hope it is, on balance, not too painful.

Please let me know if you have any questions or disagree!

m-natarajan added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 18, 2023

melvin-bot bot assigned peterdbarkerUK Oct 18, 2023

melvin-bot bot changed the title ~~Edit message, save/cancel and visiting different report removes focus from composer~~ [$500] Edit message, save/cancel and visiting different report removes focus from composer Oct 18, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 18, 2023

melvin-bot bot assigned hoangzinh Oct 18, 2023

melvin-bot bot added the Overdue label Oct 23, 2023

melvin-bot bot removed the Overdue label Oct 24, 2023

melvin-bot bot added the Overdue label Oct 26, 2023

melvin-bot bot removed the Overdue label Oct 30, 2023

melvin-bot bot added the Overdue label Nov 6, 2023

melvin-bot bot removed the Overdue label Nov 7, 2023

melvin-bot bot added the Engineering label Nov 8, 2023

peterdbarkerUK closed this as completed Nov 15, 2023

[$500] Edit message, save/cancel and visiting different report removes focus from composer #29919

[$500] Edit message, save/cancel and visiting different report removes focus from composer #29919

Comments

m-natarajan commented Oct 18, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Oct 18, 2023

melvin-bot bot commented Oct 18, 2023

melvin-bot bot commented Oct 18, 2023 • edited by peterdbarkerUK Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Oct 18, 2023

zukilover commented Oct 18, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

abzokhattab commented Oct 18, 2023

ayazalavi commented Oct 20, 2023

hoangzinh commented Oct 21, 2023

ayazalavi commented Oct 21, 2023

hoangzinh commented Oct 24, 2023

peterdbarkerUK commented Oct 24, 2023

hoangzinh commented Oct 24, 2023

melvin-bot bot commented Oct 25, 2023

melvin-bot bot commented Oct 30, 2023

hoangzinh commented Oct 30, 2023 • edited Loading

peterdbarkerUK commented Oct 31, 2023

ayazalavi commented Oct 31, 2023

melvin-bot bot commented Nov 1, 2023

melvin-bot bot commented Nov 1, 2023

hoangzinh commented Nov 2, 2023

zukilover commented Nov 2, 2023

ayazalavi commented Nov 2, 2023 via email

ayazalavi commented Nov 2, 2023 via email

hoangzinh commented Nov 3, 2023

zukilover commented Nov 3, 2023

ayazalavi commented Nov 3, 2023

Proposal

Restating the Problem

Identifying the Root Cause

Proposed Solution

Alternative Solutions

bernhardoj commented Nov 6, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

melvin-bot bot commented Nov 7, 2023

peterdbarkerUK commented Nov 7, 2023

melvin-bot bot commented Nov 8, 2023

melvin-bot bot commented Nov 8, 2023

hoangzinh commented Nov 9, 2023

hoangzinh commented Nov 13, 2023

ayazalavi commented Nov 13, 2023 via email

hoangzinh commented Nov 13, 2023

bernhardoj commented Nov 13, 2023

peterdbarkerUK commented Nov 13, 2023

ayazalavi commented Nov 13, 2023 via email

peterdbarkerUK commented Nov 14, 2023

peterdbarkerUK commented Nov 15, 2023

m-natarajan commented Oct 18, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Oct 18, 2023 •

edited by peterdbarkerUK

Loading

hoangzinh commented Oct 30, 2023 •

edited

Loading