fix: issue 23908 by dummy-1111 · Pull Request #27190 · Expensify/App

dummy-1111 · 2023-09-11T19:29:28Z

Details

Show the main composer only when there is no focused message in edit mode

Fixed Issues

$ #23908
PROPOSAL: #23908 (comment)

Tests

Note: These tests are for small screen devices. Step 5 - 6 are for mobile platforms.

Open ND and log in with any account
Open any chat(chat C) and send two messages(message A and message B)
Switch message A to edit mode
Switch message B to edit mode
Hide keyboard(press back button)
Verify that the main composer isn't shown
Click the cancel button of message A
Verify that the main composer isn't shown
Switch message A to edit mode again
Open EmojiPicker on message A and close it
Click message B to focus on it
Click the cancel button of message A
Verify that the main composer isn't shown
Delete all contents of message B and click Save
Confirm delete modal appears and click cancel
Verify that the main composer isn't shown(On ios safari, message B gets blurred and main composer is shown)
Again click Save
Click delete on confirm delete modal
Verify that the main composer is shown
Switch message A to edit mode
Delete all contents of the message then click Save button and click cancel on confirm delete modal
Switch to another chat and return back to chat C
Click cancel in message A
Verify that the message A isn't highlighted

Verify that no errors appear in the JS console

Offline tests

Open ND and log in with any account
Open any chat(chat C) and send two messages(message A and message B)
Switch message A to edit mode
Switch message B to edit mode
Hide keyboard(press back button)
Verify that the main composer isn't shown
Click the cancel button of message A
Verify that the main composer isn't shown
Switch message A to edit mode again
Open EmojiPicker on message A and close it
Click message B to focus on it
Click the cancel button of message A
Verify that the main composer isn't shown
Delete all contents of message B and press Enter or click Save
Confirm delete modal appears and click cancel
Verify that the main composer isn't shown(On ios safari, message B gets blurred and main composer is shown)
Again press Enter or click Save
Click delete on confirm delete modal
Verify that the main composer is shown
Switch message A to edit mode
Click Save button and click cancel on confirm delete modal
Switch to another chat and return back to chat C
Click cancel in message A
Verify that the message A isn't highlighted

QA Steps

Open ND and log in with any account
Open any chat(chat C) and send two messages(message A and message B)
Switch message A to edit mode
Switch message B to edit mode
Hide keyboard(press back button)
Verify that the main composer isn't shown
Click the cancel button of message A
Verify that the main composer isn't shown
Switch message A to edit mode again
Open EmojiPicker on message A and close it
Click message B to focus on it
Click the cancel button of message A
Verify that the main composer isn't shown
Delete all contents of message B and press Enter or click Save
Confirm delete modal appears and click cancel
Verify that the main composer isn't shown(On ios safari, message B gets blurred and main composer is shown)
Again press Enter or click Save
Click delete on confirm delete modal
Verify that the main composer is shown
Switch message A to edit mode
Click Save button and click cancel on confirm delete modal
Switch to another chat and return back to chat C
Click cancel in message A
Verify that the message A isn't highlighted

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

23908_mac_chrome.mp4

Mobile Web - Chrome

23908_android_chrome.mp4

Mobile Web - Safari

23908_ios_safari.mp4

Desktop

23908_mac_desktop.mp4

iOS

23908_ios_native.mp4

Android

23908_android_native.mp4

…extMenu

melvin-bot · 2023-09-11T19:36:36Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

s77rt · 2023-09-11T19:52:17Z

src/pages/home/report/ReportActionItemMessageEdit.js

+
+        if (isFocusedRef.current || EmojiPickerAction.isActive(props.action.reportActionID) || ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
+            setShouldShowComposeInputKeyboardAware(true);
+            isFocusedRef.current = true;


This does not seem right, why we are doing this?

We have discussed the new issue of re-focusing for a brief moment here. This issue is introduced when I add onMouseDown={e => e.preventDefault()} props to the save button for prevent loosing focus.

deleteDraft method is called right before unmount is called. Setting this value ensures that the current action is active. We can fix the above issue(both Enter key or Save button) by this change

I removed the above code. In order to prevent auto refocusing of composer input after delete modal is closed, I blurred the input element before showing the confirm modal
Please check the latest code

We want to prevent refocusing on that input if we press Delete on the delete modal right? But what if we press Close on the modal? Will the input get refocused again? (check on both web and native)

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

Line 260 in 2163c54

ReportActionContextMenu.showDeleteModal(props.reportID, props.action, false, deleteDraft, () => InteractionManager.runAfterInteractions(() => textInputRef.current.focus()));

We set focus in cancel handler, but it seems that focusing doesn't work on ios safari.

@s77rt

This issue already exists on staging and not a regression. Will you check again?

Yeah not something that we should be concerned with here. Safari limit the focus functionality.

s77rt · 2023-09-11T19:53:11Z

src/pages/home/report/ReportActionItemMessageEdit.js

+                                // Clear active report action when another action gets focused
+                                if (!EmojiPickerAction.isActive(props.action.reportActionID)) {
+                                    EmojiPickerAction.clearActive();
+                                }
+                                if (!ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
+                                    ReportActionContextMenu.clearActiveReportAction();
+                                }


Shouldn't we do this on onBlur?

No. We should keep active report action until another draft gets focused. This wasn't already confirmed?

@s-alves10 This is not resolved yet. Can we do this any other way?

Clear active report action when another action gets focused
We shouldn't clear active action on blur because blurred action can be active.

I think we confirmed this in the proposal

Note: I guess we can clear active in onModalHide of EmojiPickerButton for EmojiPicker, and in confirm/cancel callback of showDeleteModal for ReportActionContextMenu. Please let me know your thoughts

@s77rt

I think the new approach can cause a regression.

I see. I don't think this is a blocker for now. Will need to test though

src/libs/setShouldShowComposeInputKeyboardAware/index.native.js

src/pages/home/report/ReportActionItemMessageEdit.js

…efocusing

src/pages/home/report/ReportActionItemMessageEdit.js

s77rt · 2023-09-12T12:41:05Z

src/pages/home/report/ReportActionItemMessageEdit.js

+    // - it is focused or
+    // - EmojiPicker's activeID is equal to this action's reportActionID or
+    // - ReportActionContextMenu's reportActionID is equal to this action's reportActionID
+    const isActive = useCallback((reportActionID) => isFocusedRef.current || EmojiPickerAction.isActive(reportActionID) || ReportActionContextMenu.isActiveReportAction(reportActionID), []);


NAB. isActive does not need to accept a parameter, the report action id will always be props.action.reportActionID

s77rt · 2023-09-12T19:29:10Z

src/pages/home/report/ReportActionItemMessageEdit.js

+    // We consider the report action in edit mode is active when
+    // - it is focused or
+    // - EmojiPicker's activeID is equal to this action's reportActionID or
+    // - ReportActionContextMenu's reportActionID is equal to this action's reportActionID


Suggested change

// We consider the report action in edit mode is active when

// - it is focused or

// - EmojiPicker's activeID is equal to this action's reportActionID or

// - ReportActionContextMenu's reportActionID is equal to this action's reportActionID

// We consider the report action active if it's focused, its emoji picker is open or its context menu is open

s77rt · 2023-09-12T19:30:18Z

src/pages/home/report/ReportActionItemMessageEdit.js

            // Skip if this is not the focused message so the other edit composer stays focused.
            // In small screen devices, when EmojiPicker is shown, the current edit message will lose focus, we need to check this case as well.
-            if (!isFocusedRef.current && !EmojiPickerAction.isActive(props.action.reportActionID)) {
+            // When delete modal is opened, the current edit message will lose focus, we need to check this case as well


The comments seems outdated, please update accordingly

s77rt · 2023-09-12T19:30:59Z

src/pages/home/report/ReportActionItemMessageEdit.js

-            ComposerActions.setShouldShowComposeInput(true);
+            setShouldShowComposeInputKeyboardAware(true);
        };
+        // eslint-disable-next-line react-hooks/exhaustive-deps


Please add a comment for the disabled eslint rule

s77rt · 2023-09-12T19:32:45Z

src/pages/home/report/ReportActionItemMessageEdit.js

+            setShouldShowComposeInputKeyboardAware(true);
+            ReportActionComposeFocusManager.clear();
+            ReportActionComposeFocusManager.focus();
+            if (EmojiPickerAction.isActive(props.action.reportActionID)) {
+                EmojiPickerAction.clearActive();
+            }
+            if (ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
+                ReportActionContextMenu.clearActiveReportAction();
+            }


Do we need to call setShouldShowComposeInputKeyboardAware, EmojiPickerAction.clearActive and ReportActionContextMenu.clearActiveReportAction here? Won't they get called on the unmount phase?

I think we don't need to call them here. Removed

s77rt · 2023-09-12T19:37:19Z

src/pages/home/report/ReportActionItemMessageEdit.js

-            ComposerActions.setShouldShowComposeInput(true);
+            setShouldShowComposeInputKeyboardAware(true);


NAB. Do we need to make this keyboard aware? Will this introduce any delay?

I don't think so. I think showing composer after keyboard is closed is good. If keyboard isn't visible, this would show composer immediately

dummy-1111 · 2023-09-12T22:01:38Z

@s77rt

Please take a look again

s77rt · 2023-09-13T19:08:09Z

In step 21

Click Save button and click cancel on confirm delete modal

Please change to:
21. Delete all contents of the message then click Save button and click cancel on confirm delete modal

s77rt · 2023-09-13T19:17:13Z

src/pages/home/report/ReportActionItemMessageEdit.js

-        ReportActionComposeFocusManager.focus();
+
+        if (isActive()) {
+            ReportActionComposeFocusManager.clear();
+            ReportActionComposeFocusManager.focus();
+        }


I think this may be causing a regression. Can we remove the condition here? Currently if we delete a message the focus is not set back to the main composer but it should

I think that solution won't work. We still need to find a way to fix the regression though

I think we should focus the main composer if the deleted message is active. Please let me know if I am wrong

That's correct but apparently this is not the behavior

Screen.Recording.2023-09-14.at.5.29.25.PM.mov

@s77rt

There were some changes recently. This is due to this PR. Let me find a way

I fixed the bug of isActiveReportAction. Now this works fine as expected

I guess this is resolved, right? Because I too agree the expected behaviour is that composer should get the focus.

yes

#27190 (comment)

cc @s77rt

ok, thanks.

s77rt · 2023-09-13T20:05:12Z

Can you specify that the tests are for mobile only as the main composer will only get hidden in mobile

s77rt · 2023-09-13T21:28:20Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mweb-chrome.mov

Mobile Web - Safari

mweb-safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

dummy-1111 · 2023-09-13T23:03:41Z

Delete all contents of message B and click Save

Confirm delete modal appears and click cancel

Verify that the main composer isn't shown

Fails on iOS Safari. Any idea the reason behind that?

Please check the chat history #27190 (comment)

dummy-1111 · 2023-09-13T23:05:27Z

Can you specify that the tests are for mobile only as the main composer will only get hidden in mobile

I think these tests are for small screen devices

dummy-1111 · 2023-09-13T23:10:40Z

@s77rt

I updated the test steps accordingly. Will you check again?

s77rt · 2023-09-14T16:30:21Z

#27190 (comment)

dummy-1111 · 2023-09-14T18:02:54Z

@s77rt

Please check #27190 (comment)

dummy-1111 · 2023-09-15T13:22:45Z

@techievivek

Please take a look

techievivek · 2023-09-15T15:29:00Z

Taking a look.

techievivek

Great work on this one. 🙇 @s77rt @s-alves10

OSBotify · 2023-09-15T15:56:10Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-09-18T10:40:15Z

🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.71-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-09-20T08:12:45Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.71-12 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

parasharrajat · 2024-01-02T11:59:59Z

src/pages/home/report/ReportActionItem.js

+    // When active action changes, we need to update the `isContextMenuActive` state
+    const isActiveReportActionForMenu = ReportActionContextMenu.isActiveReportAction(props.action.reportActionID);
+    useEffect(() => {
+        setIsContextMenuActive(isActiveReportActionForMenu);
+    }, [isActiveReportActionForMenu]);


As this effect depends on temporary data it is not synced with the lifecycle of other components creating inconsistencies between states of components. It caused #28029

sangar-1028 added 8 commits September 11, 2023 10:14

fix: keep keyboardDidHideListener at most one

002241b

fix: prevent blurring when buttons are clicked on web

920d7c8

fix: add clearActiveReportAction for EmojiPicker and ReportActionCont…

09fe747

…extMenu

fix: show composer only when there is no focused composer

3951fd7

fix: update isContextMenuActive dynamically in ReportActionItem

d0e69fe

fix: minor bugs

1b7e45d

fix: bug because of refocus before unmount

b0dcc75

fix: lint error

164f88d

dummy-1111 marked this pull request as ready for review September 11, 2023 19:36

dummy-1111 requested a review from a team as a code owner September 11, 2023 19:36

melvin-bot bot requested review from s77rt and removed request for a team September 11, 2023 19:36

s77rt suggested changes Sep 11, 2023

View reviewed changes

sangar-1028 added 3 commits September 11, 2023 19:20

fix: create isActive method, add comments

b887abd

fix: blur input before showing confirm delete modal to prevent auto r…

77da6ed

…efocusing

fix: remove console log

418ddd4

s77rt reviewed Sep 12, 2023

View reviewed changes

src/pages/home/report/ReportActionItemMessageEdit.js Outdated Show resolved Hide resolved

s77rt reviewed Sep 12, 2023

View reviewed changes

sangar-1028 added 3 commits September 12, 2023 08:15

Merge branch 'main' into fix/issue-23908

b79780d

fix: restore emoji button

b2f4e3a

fix: restore previous code

98d573e

s77rt suggested changes Sep 12, 2023

View reviewed changes

sangar-1028 added 3 commits September 12, 2023 16:24

fix: update comments

3a246e6

fix: conflicts

b96510d

fix: remove same code from deleteDraft

a74b2b2

dummy-1111 requested a review from s77rt September 13, 2023 15:26

s77rt suggested changes Sep 13, 2023

View reviewed changes

sangar-1028 added 4 commits September 14, 2023 11:46

fix: initialize textinput on mount

e4a381f

Merge branch 'main' into fix/issue-23908

97aaa26

revert wrong changes

c24df9c

fix: ReportActionContextMenu.isActiveReportAction

4e9083f

s77rt approved these changes Sep 15, 2023

View reviewed changes

melvin-bot bot requested a review from techievivek September 15, 2023 10:57

techievivek approved these changes Sep 15, 2023

View reviewed changes

techievivek merged commit 2427a06 into Expensify:main Sep 15, 2023

OSBotify mentioned this pull request Sep 18, 2023

Deploy Checklist: New Expensify 2023-09-18 #27654

Closed

91 tasks

melvin-bot bot mentioned this pull request Sep 20, 2023

[HOLD for payment 2023-09-27] [$2000] Chat - Main composer is displayed when editing two messages #23908

Closed

6 tasks

This was referenced Sep 29, 2023

[HOLD for payment 2023-10-28] [$500] Chat - Focus is lost after deleting a message from edit mode #28420

Closed

[CP Staging] Fix the main composer not regaining focus after action is deleted #28456

Closed

tienifr mentioned this pull request Oct 4, 2023

Fix: Composer not focused on click while editor's emoji modal is open #28238

Merged

56 tasks

narefyev91 mentioned this pull request Nov 13, 2023

Chat - Compose box reappears when trying to edit a message, exit the chat and go to it again #31238

Closed

6 tasks

barros001 mentioned this pull request Nov 30, 2023

[HOLD #31594][$500] mWeb/Safari - The conversation list gets hidden after adding an emoji #32217

Closed

6 tasks

dummy-1111 deleted the fix/issue-23908 branch December 21, 2023 07:39

parasharrajat reviewed Jan 2, 2024

View reviewed changes

parasharrajat mentioned this pull request Jan 2, 2024

[HOLD for payment 2024-01-05] [$500] Web - Message background color is stuck on cancel delete #28029

Closed

6 tasks

		ComposerActions.setShouldShowComposeInput(true);
		setShouldShowComposeInputKeyboardAware(true);

Conversation

dummy-1111 commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Sep 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dummy-1111 commented Sep 12, 2023

Uh oh!

s77rt commented Sep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dummy-1111 commented Sep 11, 2023 •

edited

Loading

techievivek Sep 15, 2023 •

edited

Loading

dummy-1111 Sep 15, 2023 •

edited

Loading

s77rt commented Sep 13, 2023 •

edited

Loading