[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$500] Web - Chat - Edit message box blink on escape from keyboard shortcut popup #26527

izarutskaya · 2023-09-01T20:45:39Z

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

Action Performed:

Open the app
Open any report
Edit any message
Click CTRL+J to open keyboard shortcut
Click ESC to close popup and observe that edit message box blinks before changing focus to main compose box

Expected Result:

Edit message box should not glitch out on change of focus to main compose box as it works currently on ESC from CTRL+K or CTRL+SHIFT+K shortcuts

Actual Result:

Edit message box blinks on change of focus to main compose box when keyboard shortcut is open using CTRL+J and closed by ESC

Workaround:

Unknown

Platforms:

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

Version Number: v1.3.61-3

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): Y

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

edit.box.blink.on.ESC.mp4

Bandicam.2023-09-01.23-37-36-100.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692944783305439

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01ad8834baac53effd
Upwork Job ID: 1698788846930915328
Last Price Increase: 2023-09-18
Automatic offers:

Dhanashree-Sawant | Reporter | 27049742

melvin-bot · 2023-09-01T20:45:44Z

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

melvin-bot · 2023-09-01T20:45:46Z

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

anmurali · 2023-09-04T20:03:08Z

Can reproduce and is not a dupe

melvin-bot · 2023-09-04T20:03:19Z

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

melvin-bot · 2023-09-04T20:03:23Z

Current assignee @anmurali is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-09-04T20:03:29Z

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

joh42 · 2023-09-05T13:26:58Z

What is the expected behavior here @anmurali? Should the edit message box be focused?

narefyev91 · 2023-09-06T10:57:42Z

Proposal

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

After selecting any chat from the sidebar and editing any comment within it, the focus shifts to the main compose input once you close the modal.

What is the root cause of that problem?

Problem exists in code where we are programmatically setting focus to the main composer. It happens in following code File is ComposerWithSuggestions

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

commenting focus() in Line 433 solves the problem without breaking the code.

What alternative solutions did you explore? (Optional)

Rewrite focus handler for all inputs in order to manage it within a hook

Hey @ayazalavi ! Removing focus will fully break this logic:
// We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused.

ayazalavi · 2023-09-06T12:16:02Z

Hey @ayazalavi ! Removing focus will fully break this logic: // We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused.

@narefyev91 Working video:

480.Screen.Recording.2023-09-06.at.5.13.08.PM.mov

It is not not breaking this "We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused." Which modal is this referring to?

narefyev91 · 2023-09-06T12:27:00Z

@ayazalavi your fix will break all modal closing re-focus input logic:
With your fix:

Screen.Recording.2023-09-06.at.15.23.36.mov

Without your fix:

Screen.Recording.2023-09-06.at.15.24.23.mov

ayazalavi · 2023-09-06T13:35:29Z

Proposal

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

After selecting any chat from the sidebar and editing any comment within it, the focus shifts to the main compose input once you close the modal with the glitch.

What is the root cause of that problem?

Root cause is the modal we are using in our codebase. It shifts focus back to input field as soon as it closes. reactjs/react-modal#680

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

Use ONYX to tap edit field focus. In

App/src/ONYXKEYS.ts

Line 9 in ea5f461

const ONYXKEYS = {

add EDITFOCUSED: 'editFocused',

Add

withOnyx({
        modal: {
            key: ONYXKEYS.MODAL,
        },
        editFocused: {
            key: ONYXKEYS.EDITFOCUSED
        }
    })

in

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

Lines 455 to 468 in ea5f461

    
           export default compose( 
        
               withLocalize, 
        
               withReportActionsDrafts({ 
        
                   propName: 'drafts', 
        
               }), 
        
           )( 
        
               React.forwardRef((props, ref) => ( 
        
                   <ReportActionItemMessageEdit 
        
                       // eslint-disable-next-line react/jsx-props-no-spreading 
        
                       {...props} 
        
                       forwardedRef={ref} 
        
                   /> 
        
               )), 
        
           );

and this

useEffect(() => {
        if (isFocused && !props.editFocused) {
            Onyx.set(ONYXKEYS.EDITFOCUSED, true);
            textInputRef.current.focus();
        }
        else if (!isFocused && props.editFocused && !props.modal.isVisible) {
            if (timeoutHandle) {
                clearTimeout(timeoutHandle)
                timeoutHandle = undefined
            }
            timeoutHandle = setTimeout(() => {
                if (!props.modal.isVisible && !props.modal.willAlertModalBecomeVisible) {
                    Onyx.set(ONYXKEYS.EDITFOCUSED, false);
                }
            }, 100)
        }
    }, [isFocused, props.editFocused, props.modal])

in

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

Line 91 in ea5f461

function ReportActionItemMessageEdit(props) {

The above useEffect ensures that if the edit input is focused, then ONYX EDITFOCUSED should be set to true. In the second condition, it checks if the input is not focused but was previously focused. In this case, it checks if the modal becomes visible using a timeout. If it doesn't become visible, then it sets ONYX EDITFOCUSED to false. This ensures that the input is only considered out of focus when it happens for reasons other than the modal.

The above code will work generically for all modals. It can be made more generic to ensure input field can maintain focus with modal opening and will prevent shifting focus to main composer input field. We can make it a hook as well to ensure field focus doesn't gets broken due to modal.

Here

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

Lines 532 to 540 in ea5f461

    
           export default compose( 
        
               withKeyboardState, 
        
               withOnyx({ 
        
                   numberOfLines: { 
        
                       key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT_NUMBER_OF_LINES}${reportID}`, 
        
                       // We might not have number of lines in onyx yet, for which the composer would be rendered as null 
        
                       // during the first render, which we want to avoid: 
        
                       initWithStoredValues: false, 
        
                   },

Add

editFocused: {
            key: ONYXKEYS.EDITFOCUSED
        },

Update this

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

Lines 434 to 435 in ea5f461

    
               focus(); 
        
           }, [focus, prevIsFocused, prevIsModalVisible, isFocused, modal.isVisible, isNextModalWillOpenRef]);

to

 if (editFocused) {
            Onyx.set(ONYXKEYS.EDITFOCUSED, false);
            return;
        }
        focus();
    }, [focus, prevIsFocused, editFocused, prevIsModalVisible, isFocused, modal.isVisible, isNextModalWillOpenRef]);

add editFocused in props

Result:

Untitled.11.mp4

What alternative solutions did you explore? (Optional)

before opening modal blur edit text field.

narefyev91 · 2023-09-06T20:30:44Z

@ayazalavi can you please explain why those lines of code will fix issues - and did you test your code? Will it introduce any regression? Just the same as i mentioned above with focusing composer - after closing right panel

ayazalavi · 2023-09-07T14:28:03Z

@narefyev91 please take a look at my new proposal here

#26527 (comment)

narefyev91 · 2023-09-11T09:10:07Z

@ayazalavi thank you for your updated proposal - but your code still introduce regressions
Example - how it should be:

Screen.Recording.2023-09-11.at.12.03.55.mov

How it's after your code:

Screen.Recording.2023-09-11.at.12.06.05.mov

narefyev91 · 2023-09-11T09:22:07Z

Also some issues with Android web as well
Before:

Screen.Recording.2023-09-11.at.12.16.12.mov

After:

Screen.Recording.2023-09-11.at.12.13.51.mov

We should be sure that any new changes regarding autofocus will not introduce any regression on each platform

ayazalavi · 2023-09-11T10:59:58Z

@narefyev91 thanks for such detailed review. Let me fix my proposal. Thanks again for the feedback as well.

melvin-bot · 2023-09-11T16:00:17Z

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

ayazalavi · 2023-09-12T08:08:36Z

@narefyev91 update to #26527 (comment) proposal:

Root cause:

As soon as modal gets closed then this react hook gets called forcing focus to compose field:

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

Lines 423 to 436 in 662af46

    
           useEffect(() => { 
        
               if (modal.isVisible && !prevIsModalVisible) { 
        
                   // eslint-disable-next-line no-param-reassign 
        
                   isNextModalWillOpenRef.current = false; 
        
               } 
        
               // We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused. 
        
               // We avoid doing this on native platforms since the software keyboard popping 
        
               // open creates a jarring and broken UX. 
        
               if (!(willBlurTextInputOnTapOutside && !isNextModalWillOpenRef.current && !modal.isVisible && isFocused && (prevIsModalVisible || !prevIsFocused))) { 
        
                   return; 
        
               } 
        
               focus(); 
        
           }, [focus, prevIsFocused, prevIsModalVisible, isFocused, modal.isVisible, isNextModalWillOpenRef]);

Solution

Remove modal.isVisible from dependency array so that callback does'nt gets triggered on every modal closing.

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

Line 436 in 662af46

    
           }, [focus, prevIsFocused, prevIsModalVisible, isFocused, modal.isVisible, isNextModalWillOpenRef]);

Here is how it looks:

Untitled.mp4

melvin-bot · 2023-10-13T13:32:55Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @ayazalavi got assigned: 2023-10-06 02:34:44 Z
when the PR got merged: 2023-10-13 13:32:55 UTC
days elapsed: 5

On to the next one 🚀

melvin-bot · 2023-10-16T21:29:21Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-10-16T21:29:25Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

added composer focus glitch fix #29073

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@ayazalavi requires payment
@narefyev91 does not require payment (Contractor)
@dhanashree-sawant requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-16T21:29:27Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@narefyev91] The PR that introduced the bug has been identified. Link to the PR:
[@narefyev91] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@narefyev91] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@narefyev91] Determine if we should create a regression test for this bug.
[@narefyev91] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

melvin-bot · 2023-10-16T21:54:27Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

added composer focus glitch fix #29073

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@ayazalavi requires payment
@narefyev91 does not require payment (Contractor)
@dhanashree-sawant requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-16T21:54:29Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@narefyev91] The PR that introduced the bug has been identified. Link to the PR:
[@narefyev91] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@narefyev91] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@narefyev91] Determine if we should create a regression test for this bug.
[@narefyev91] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

amyevans · 2023-10-25T13:27:51Z

@narefyev91 could you complete the checklist please?

narefyev91 · 2023-10-25T13:29:40Z

The PR that introduced the bug has been identified. Link to the PR - fully new implementation - not coming from any regression
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment - N/A
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion - N/A
Determine if we should create a regression test for this bug - N/A

ayazalavi · 2023-10-26T02:27:23Z

@flaviadefaria https://www.upwork.com/fl/~01a53213c1c66acb2d?s=996364627857502209

flaviadefaria · 2023-10-26T10:55:58Z

Payment Summary:

@ayazalavi requires payment
@narefyev91 does not require payment (Contractor)
@dhanashree-sawant requires payment offer (Reporter)

@dhanashree-sawant = $50
@ayazalavi = $500 no bonus = $500

flaviadefaria · 2023-10-26T10:58:33Z

@dhanashree-sawant = paid
@ayazalavi = offer sent

flaviadefaria · 2023-10-26T11:38:36Z

Everyone has been paid so closing this.

izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 1, 2023

melvin-bot bot assigned anmurali Sep 1, 2023

melvin-bot bot added the Overdue label Sep 4, 2023

melvin-bot bot removed the Overdue label Sep 4, 2023

anmurali added the External Added to denote the issue can be worked on by a contributor label Sep 4, 2023

melvin-bot bot changed the title ~~Web - Chat - Edit message box blink on escape from keyboard shortcut popup~~ [$500] Web - Chat - Edit message box blink on escape from keyboard shortcut popup Sep 4, 2023

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

melvin-bot bot assigned narefyev91 Sep 4, 2023

This comment was marked as outdated.

Sign in to view

melvin-bot bot added the Overdue label Sep 9, 2023

melvin-bot bot removed the Overdue label Sep 11, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 16, 2023

melvin-bot bot changed the title ~~[$500] Web - Chat - Edit message box blink on escape from keyboard shortcut popup~~ [HOLD for payment 2023-10-23] [$500] Web - Chat - Edit message box blink on escape from keyboard shortcut popup Oct 16, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 16, 2023

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 Daily KSv2 labels Oct 23, 2023

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

flaviadefaria assigned flaviadefaria and unassigned anmurali Oct 26, 2023

flaviadefaria closed this as completed Oct 26, 2023

hoangzinh mentioned this issue Oct 30, 2023

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

Closed

6 tasks

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$500] Web - Chat - Edit message box blink on escape from keyboard shortcut popup #26527

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$500] Web - Chat - Edit message box blink on escape from keyboard shortcut popup #26527

Comments

izarutskaya commented Sep 1, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 1, 2023

melvin-bot bot commented Sep 1, 2023

Bug0 Triage Checklist (Main S/O)

anmurali commented Sep 4, 2023

melvin-bot bot commented Sep 4, 2023

melvin-bot bot commented Sep 4, 2023

melvin-bot bot commented Sep 4, 2023

joh42 commented Sep 5, 2023

This comment was marked as outdated.

narefyev91 commented Sep 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?

What alternative solutions did you explore? (Optional)

ayazalavi commented Sep 6, 2023 • edited Loading

narefyev91 commented Sep 6, 2023

ayazalavi commented Sep 6, 2023 • edited Loading

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)

narefyev91 commented Sep 6, 2023

ayazalavi commented Sep 7, 2023 • edited Loading

narefyev91 commented Sep 11, 2023

narefyev91 commented Sep 11, 2023

ayazalavi commented Sep 11, 2023

melvin-bot bot commented Sep 11, 2023

ayazalavi commented Sep 12, 2023 • edited Loading

Root cause:

Solution

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023 • edited by flaviadefaria Loading

amyevans commented Oct 25, 2023

narefyev91 commented Oct 25, 2023

ayazalavi commented Oct 26, 2023

flaviadefaria commented Oct 26, 2023

flaviadefaria commented Oct 26, 2023

flaviadefaria commented Oct 26, 2023

izarutskaya commented Sep 1, 2023 •

edited by melvin-bot bot

Loading

ayazalavi commented Sep 6, 2023 •

edited

Loading

ayazalavi commented Sep 6, 2023 •

edited

Loading

ayazalavi commented Sep 7, 2023 •

edited

Loading

ayazalavi commented Sep 12, 2023 •

edited

Loading

melvin-bot bot commented Oct 16, 2023 •

edited by flaviadefaria

Loading