Try to refactor the logic regarding refocusing when the modal is closed #29199

ntdiary · 2023-10-10T15:15:56Z

So far, the Modal components under our control can be categorized into three types: BaseModal, PopoverWithoutOverlay, and RHP (Right Hand Panel).

This PR will simulate the iOS native Modal refocusing behavior: If there is a focused input field before opening the Modal, our app will attempt to restore that focus after the Modal is dismissed. If there is no focus before opening, no restore after the Modal is dismissed.

Additionally, this PR will introduce a restoreFocusType prop, here are possible values:

default, this will follow the default behavior mentioned above.
preserve, this will leave the focus in the manager and no longer restore focus, applicable for consecutive modal scenarios.
delete, this will delete the focus and no longer restore focus, the caller can decide in its own business logic whether to focus a specific element.

The specific business Modals that have been tested:

context menu (long press / right click a message to open this modal)
three dots menu (click the three dot button in the top right corner of the app to open this modal)
confirm modal (some delete actions will show this modal)
attachment preview modal (click an image message to open this modal)
emoji picker modal (click the emoji button in a composer)
the main composer attachment modal (click the plus button in the main composer to open this modal)
RHP (click the username in a message to open the profile page, or use shortcuts to open some RHP pages)

Once the POC is approved by most people, we'll refine the code, clean things up, and then repush it.

Details

Fixed Issues

$ #29011
$ #24452
PROPOSAL: #24452 (comment)

Tests

context menu modal

case 1 (cancel action)

Open a report.
Focus the main composer.
Send a regular text message.
Long press this message to open the context menu modal (or right click the message for web/desktop platforms).
Click the backdrop or blank area to close the context menu modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 4 & 5.
Verify the main composer won't gain focus again.

case 2 (add reaction + cancel action)

Open a report.
Focus the main composer.
Long press a text message to open the context menu modal.
Click the Add reactionto open the emoji modal.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop or blank area to close the emoji modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, and 6.
Verify the main composer won't gain focus again.

case 3 (edit coment)

Open a report.
Focus the main composer.
Long press a text message to open the context menu modal.
Select the Edit comment.
Verify the edit composer will gain focus.

case 4 (delete comment)

Open a report.
Focus the main composer.
Long press the text message to open the context menu modal.
Select the Delete comment to open the Confirm modal.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop / Delete button / Cancel button to close the Confirm modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, and 6.
Verify the main composer won't gain focus again.

emoji modal

case 1 (cancel action)

Open any report.
Focus the main composer.
Click the emoji button to open the emoji modal.
Click the backdrop or blank area to close the emoji modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3 & 4.
Verify the main composer won't gain focus again.

case 2 (cancel action)

Open any report.
Edit a text message.
Click the emoji button in the edit composer to open the emoji modal.
Click the backdrop or blank area to close the emoji modal.
Verify the edit composer will gain focus again.
Make the edit composer lose focus.
Repeat step 3 & 4.
Verify the edit composer won't gain focus again.

case 3 (cancel action)

Open any report.
Edit a text message.
Focus the main composer.
Click the emoji button in the edit composer to open the emoji modal.
Click the backdrop or blank area to close the emoji modal.
Verify the main composer will gain focus again.

Right Hand Panel

case 1 (test by mouse)

Open a report.
Focus the main composer.
Click the top avatar to open the detail page.
Click the backdrop to close the detail page.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3 & 4.
Verify the main composer won't gain focus again.

case 2 (test by keyboard shortcuts)

Open a 1:1 report.
Focus the main composer.
Press CMD + J to open the keyboard shortcuts page.
Click the backdrop to close the shortcuts page.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3 & 4.
Verify the main composer won't gain focus again.

attachment modal

case 1 (cancel action)

Open a report.
Focus the main composer.
Click the plus button in the main composer to open the first attachment modal.
Click the backdrop or blank area to close the first attachment modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3 & 4.
Verify the main composer won't gain focus again.

case 2 (first attachment modal + RHP)

Open a report.
Focus the main composer.
Click the plus button to open the first attachment modal.
Select the Request money / Send money / Assign task.
Verify that there is no focus/keyboard flickering during the transition.
Click the top-left back button.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, and 6.
Verify the main composer won't gain focus again.

case 3 (all mobile platforms: cancel action for the second modal)

Open a report.
Focus the main composer.
Click the plus button to open the first attachment modal.
Select the Add attachment.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop to close the second attachment modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, and 6.
Verify the main composer won't gain focus again.

case 4 (cancel file selection)

Open a report.
Focus the main composer.
Click the plus button in the main composer to open the first attachment modal.
Select the Add attachment.
1. For mobile native platforms: select the Choose from gallery / Choose document.
Verify that there is no focus/keyboard flickering during the transition.
Click Cancel to cancel the file selection.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, and 6.
Verify the main composer won't gain focus again.

case 5 (send an attachment)

Open a report.
Focus the main composer.
Click the plus button in the main composer to open the first attachment modal.
Select the Add attachment.
1. For mobile native platforms, select the Choose from gallery.
Select an attachment.
Verify that there is no focus/keyboard flickering during the transition.
Click the top-left back button or Send button.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, 6, and 7.
Verify the main composer won't gain focus again.

case 6 (send an invalid attachment, e.g. file size < 240B)

Open a 1:1 report.
Focus the main composer.
Click the plus button in the main composer to open the first attachment modal.
Select the Add attachment.
Select the Choose document.
Select a document with a file ize less than 240B.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop or Close button.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, 6, 7, and 8.
Verify the main composer won't gain focus again.

case 7 (attachment preview modal)

Open a report with an image message.
Focus the main composer.
Click the image to open the preview modal.
Click the back button or close button to close the modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3 and 4.
Verify the main composer won't gain focus again.

case 8 (receipt preview modal + ThreeDotsMenu modal)

Open a IOU report, which has a receipt image.
Focus the main composer.
Click the receipt image to open the preview modal.
Click the ThreeDotsMenu button to open the menu modal.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop or blank area to close the menu modal.
Verify that there is no focus/keyboard flickering during the transition.
Close the back button or close button to close the preview modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, 6, 7, and 8.
Verify the main composer won't gain focus again.

case 9 (receipt preview modal + ThreeDotsMenu modal + RHP)

Open a IOU report, which has a receipt image.
Focus the main composer.
Click the receipt image to open the preview modal.
Click the ThreeDotsMenu button to open the menu modal.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop or blank area to close the menu modal.
Verify that there is no focus/keyboard flickering during the transition.
Select the Replace option.
Verify that there is no focus/keyboard flickering during the transition.
Close the backdrop or back button to close the RHP.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 3, 4, 5, 6, 7, 8, 9, and 10.
Verify the main composer won't gain focus again.

ThreeDotsMenu modal

case 1 (cancel action)

Open a report.
Focus the main composer.
Click the top-right ThreeDotsMenu button to open the modal.
Click the backdrop or blank area to close the modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 4 & 5.
Verify the main composer won't gain focus again.

case 2 (delete action)

Open a IOU report.
Focus the main composer.
Click the top-right button to open the three dots menu modal.
Select the Delete request to open the Confirm modal.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop or Cancel button to close the Confirm modal.
Verify the main composer will gain focus again.
Make the main composer lose focus.
Repeat step 4, 5, and 6.
Verify the main composer won't gain focus again.

case 3 (waypoint editor, cancel action)

Click the global FAB and select the Distance tab.
Add three waypoints and then select one of them.
Make sure the address input is focused.
Click the top-right ThreeDotsMenu button to open the menu modal.
Click the backdrop or blank area to close the modal.
Verify the address input will gain focus again.
Make the address input lose focus.
Repeat step 4 & 5.
Verify the main composer won't gain focus again.

case 4 (waypoint editor, delete option + cancel action)

Click the global FAB and select the Distance tab.
Add three waypoints and then select one of them.
Make sure the address input is focused.
Click the top-right ThreeDotsMenu button to open the menu modal.
Selete the Delete option.
Verify that there is no focus/keyboard flickering during the transition.
Click the backdrop or blank area or Cancel button to close the Confirm modal.
Verify the address input will gain focus again.
Make the address input lose focus.
Repeat step 4, 5, 6, and 7.
Verify the main composer won't gain focus again.

case 5 (waypoint editor, delete option + confirm action)

Click the global FAB and select the Distance tab.
Add three waypoints and then select one of them.
Make sure the address input is focused.
Click the top-right ThreeDotsMenu button to open the menu modal.
Selete the Delete option.
Verify that there is no focus/keyboard flickering during the transition.
Click the Delete button.
Verify that there is no focus/keyboard flickering during the transition.
Add a new waypoint and then select it.
Make the address input lose focus.
Repeat step 4, 5, 6, and 7.
Verify that there is no focus/keyboard flickering during the transition.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2023-10-10T15:16:02Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

ntdiary · 2023-11-29T14:04:58Z

Hi @0xmiroslav, when you get a chance, could you take a quick look at the implementation of this PR? It has successfully passed tests in many scenarios. I'm eager to get your initial impressions on this PR, such as whether this implementation approach is acceptable and if there are any serious blockers I might have overlooked. :)
Additionally, considering the importance of controlling the size of the PR, I am also planning to address some non-urgent cleanup tasks in follow-up PRs, such as removing unnecessary focus logic, optimizing the Modal implementation , and other improvements.

src/CONST.ts

src/libs/ComposerFocusManager.js

parasharrajat · 2023-12-01T16:21:47Z

src/components/PopoverWithoutOverlay/index.js

-                        >
-                            {props.children}
-                        </View>
+                        <ModalContent onDismiss={handleDismissContent}>


What is the purpose of this?

App/src/components/PopoverWithoutOverlay/index.js

Lines 84 to 88 in e1fcb42

} else {

props.onModalHide();

close(props.anchorRef);

Modal.onModalDidClose();

}

It's somewhat similar to the above condition code, but has the following advantages:

it's only called when the modal disappears (the code above is called even on the initial load),

it's beneficial for extracting common logic in the future.

parasharrajat · 2023-12-01T16:24:16Z

src/pages/iou/WaypointEditor.js

@@ -153,6 +154,7 @@ function WaypointEditor({route: {params: {iouType = '', transactionID = '', wayp
    };

    const deleteStopAndHideModal = () => {
+        setRestoreFocusType(CONST.MODAL.RESTORE_TYPE.DELETE);


Why the focus type have to change dynamically?

For this case, when we remove a waypoint, the navigation will return to the distance page, the input is unmounted after refocusing is called, which can cause keyboard flickering. So setting the type to DELETE can avoid this issue. When clicking Cancel, we can set it back to DEFAULT to allow refocusing to continue working.

demo.mp4

parasharrajat · 2023-12-06T20:06:31Z

I will take a deeper dive into this PR starting tomorrow.

Julesssss · 2023-12-21T10:24:41Z

Hey both, just checking the progress here. It looks like there have been a decent amount of updates since the last review, but I see lint errors. @ntdiary is this ready for a second C+ review?

ntdiary · 2023-12-21T13:40:57Z

Hey both, just checking the progress here. It looks like there have been a decent amount of updates since the last review, but I see lint errors. @ntdiary is this ready for a second C+ review?

@Julesssss, so far, the current implementation has fixed many cases. My updates are more aimed at resolving conflicts.
I've also made adjustments based on the feedback from the previous review, so I think it's ready for a deeper review. :)

ntdiary · 2024-01-29T16:05:54Z

Just updated the test cases, so far, the count is 23. :)

Julesssss

Hey @ntdiary, I didn't realise this would require so many test cases. Lets talk in the issue about increasing the bounty 👍

parasharrajat · 2024-01-31T08:34:07Z

Yeah, I think the bounty should be increased for sure.

Meanwhile, we should create a project and put other focus-related issues on hold so that we don't create new challenges for this PR.

May be inform this on the C+ channel so that C+ can prevent that from happening.

Julesssss · 2024-02-21T14:34:09Z

Hey @parasharrajat, bumping the review here now your back

parasharrajat · 2024-03-02T16:04:57Z

I thought we are breaking this PR into parts. @ntdiary Could you please confirm?

First PR phase1: fix emoji picker refocus issue #35572

mallenexpensify · 2024-03-28T22:42:03Z

@ntdiary can you provide an update on where we're at here?
Asking cuz the below is held on this PR

[$500] Compose Box - Focus not lost from composer on LHN popup open #32970

ntdiary · 2024-03-29T13:20:23Z

@ntdiary can you provide an update on where we're at here? Asking cuz the below is held on this PR

[HOLD #29199] [$500] Compose Box - Focus not lost from composer on LHN popup open #32970

@mallenexpensify, yesterday, @getusha reported two bugs in the first small PR #35572, and I'm still investigating them. :)

Julesssss · 2024-04-29T13:37:19Z

Hey @ntdiary, just checking in here for the next steps as the linked PRs were merged.

ntdiary · 2024-04-29T15:55:11Z

Hey @ntdiary, just checking in here for the next steps as the linked PRs were merged.

@Julesssss, sorry for the delayed update. While I have reassigned all other new issues in the past few weeks, there are still two long-standing and relatively challenging issues that require my attention (#26239, #37596). Now that progress has been made on the other two issues, so expect to be able to raise a second small PR for this issue this week. 😂

ntdiary · 2024-04-30T16:08:39Z

It seems our app recently switched to the Fabric rendering system, which changed the execution order of some native & js code when displaying the modal. I'll investigate further this week to adapt to this new behavior. :)

Julesssss · 2024-04-30T16:42:28Z

Ah another new problem introduced by the new architecture 😅

Julesssss · 2024-06-17T08:24:17Z

Hey @ntdiary. Can we close this outdated PR?

ntdiary · 2024-06-17T12:11:58Z

Hey @ntdiary. Can we close this outdated PR?

@Julesssss, sure! BTW, I noticed that some issues have been put on hold because of this PR, so I plan to look into them this week to see if I can offer any suggestions, just like issue #31224. :)

ntdiary force-pushed the fix-24452 branch 3 times, most recently from 1130e78 to d5694f6 Compare November 20, 2023 18:29

This was referenced Nov 20, 2023

[Awaiting Payment 13th June] [$4000] Android - Edit message - keyboard is dismissed after selecting an emoji #29011

Closed

fix keyboard flashing while clicking "Add attachment" #23994

Merged

ntdiary force-pushed the fix-24452 branch 2 times, most recently from ec72ddf to 05c51f6 Compare November 21, 2023 09:12

try to refactor the modal's refocus logic

4952dce

ntdiary force-pushed the fix-24452 branch from 545c7e6 to 4952dce Compare November 29, 2023 12:58

fix map iterator

7655502

0xmiros mentioned this pull request Nov 30, 2023

[$500] mWeb - The keyboard opens after requesting money in 1:1 DM #31588

Closed

6 tasks

parasharrajat suggested changes Dec 1, 2023

View reviewed changes

parasharrajat reviewed Dec 1, 2023

View reviewed changes

ntdiary mentioned this pull request Dec 15, 2023

fix: 10731 Focus composer without showing keyboard when users go to chats #32711

Merged

56 tasks

ntdiary added 7 commits December 18, 2023 23:35

Merge branch 'main' into fix-24452

0f1ebe1

fix merge error

8a7ce9d

Merge branch 'main' into fix-24452

bda18dd

fix RHP refocus

44b57f5

fix ts check

5dfca1b

optimize based on the comments

b73e737

delete outdated file

2d85663

Julesssss assigned ntdiary Dec 21, 2023

ntdiary added 3 commits December 21, 2023 18:37

Merge branch 'main' into fix-24452

ed42722

fix lint error

7da38ae

Merge branch 'main' into fix-24452

a7bbc6c

Merge branch 'main' into fix-24452

b450dd6

situchan mentioned this pull request Jan 24, 2024

[HOLD for payment 2024-02-07] [$500] Chat - Keyboard displayed over compose box when user returned from Distance Request Money #34724

Closed

6 tasks

ntdiary added 10 commits January 24, 2024 22:06

Merge branch 'main' into fix-24452

5372ced

fix conflicts

8c131ed

Merge branch 'main' into fix-24452

4e5b69e

fix ts check

1ddaf05

fix lint error

b826eca

remove unused import

51d8928

Merge branch 'main' into fix-24452

8c73ac7

fix app crash

cc009cc

fix android preview modal

f853d13

fix android AddReaction button

40ff044

lint error

bd84c65

Julesssss reviewed Jan 30, 2024

View reviewed changes

ikevin127 mentioned this pull request Feb 5, 2024

[HOLD #29199] [$500] iOS - Composer field loses focus & keypad dismissed when creating thread conversation #35778

Closed

6 tasks

ikevin127 mentioned this pull request Feb 13, 2024

[HOLD #35572] [$500] Chat - Keyboard not dismissed after tapping flag as offensive in thread #36373

Closed

6 tasks

fedirjh mentioned this pull request Mar 14, 2024

[Hold] on #29199 [$500] Chat - The composer isn't focused if you choose "Manage my team's expenses" #37897

Closed

6 tasks

ntdiary closed this Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to refactor the logic regarding refocusing when the modal is closed #29199

Try to refactor the logic regarding refocusing when the modal is closed #29199

ntdiary commented Oct 10, 2023 •

edited

Loading

melvin-bot bot commented Oct 10, 2023

ntdiary commented Nov 29, 2023

parasharrajat Dec 1, 2023

ntdiary Dec 6, 2023

parasharrajat Dec 1, 2023

ntdiary Dec 6, 2023

parasharrajat commented Dec 6, 2023

Julesssss commented Dec 21, 2023

ntdiary commented Dec 21, 2023

ntdiary commented Jan 29, 2024

Julesssss left a comment

parasharrajat commented Jan 31, 2024 •

edited

Loading

Julesssss commented Feb 21, 2024

parasharrajat commented Mar 2, 2024 •

edited

Loading

mallenexpensify commented Mar 28, 2024

ntdiary commented Mar 29, 2024

Julesssss commented Apr 29, 2024

ntdiary commented Apr 29, 2024

ntdiary commented Apr 30, 2024

Julesssss commented Apr 30, 2024

Julesssss commented Jun 17, 2024

ntdiary commented Jun 17, 2024

	} else {
	props.onModalHide();
	close(props.anchorRef);
	Modal.onModalDidClose();
	}

Try to refactor the logic regarding refocusing when the modal is closed #29199

Try to refactor the logic regarding refocusing when the modal is closed #29199

Conversation

ntdiary commented Oct 10, 2023 • edited Loading

Details

Fixed Issues

Tests

context menu modal

emoji modal

Right Hand Panel

attachment modal

ThreeDotsMenu modal

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 10, 2023

ntdiary commented Nov 29, 2023

parasharrajat Dec 1, 2023

Choose a reason for hiding this comment

ntdiary Dec 6, 2023

Choose a reason for hiding this comment

parasharrajat Dec 1, 2023

Choose a reason for hiding this comment

ntdiary Dec 6, 2023

Choose a reason for hiding this comment

parasharrajat commented Dec 6, 2023

Julesssss commented Dec 21, 2023

ntdiary commented Dec 21, 2023

ntdiary commented Jan 29, 2024

Julesssss left a comment

Choose a reason for hiding this comment

parasharrajat commented Jan 31, 2024 • edited Loading

Julesssss commented Feb 21, 2024

parasharrajat commented Mar 2, 2024 • edited Loading

mallenexpensify commented Mar 28, 2024

ntdiary commented Mar 29, 2024

Julesssss commented Apr 29, 2024

ntdiary commented Apr 29, 2024

ntdiary commented Apr 30, 2024

Julesssss commented Apr 30, 2024

Julesssss commented Jun 17, 2024

ntdiary commented Jun 17, 2024

ntdiary commented Oct 10, 2023 •

edited

Loading

parasharrajat commented Jan 31, 2024 •

edited

Loading

parasharrajat commented Mar 2, 2024 •

edited

Loading