Fix compose box resurfacing issue #9060

marcaaron · 2022-05-17T23:09:17Z

Details

Fixed Issues

$ #9016

Tests & QA (On a mobile device or mWeb only)

Edit a message
Verify the "compose" input at the bottom hides
Blur the input by navigating away or tapping somewhere else
Verify the compose input at the bottom reappears

Verify that no errors appear in the JS console

PR Review Checklist

PR Reviewer Checklist

Screenshots

Web

❌ - Change doesn't affect web

Mobile Web

Mobile web is working - but looks kind of inconsistent on iOS simulator. We should probably create some follow up issues for these bugs if they are not being worked...

There's some weird extra whitespace (maybe dev only)
Scrolling up to the comment isn't working (probably because scrollToIndex / comment linking doesn't work on web)

2022-05-20_08-47-19.mp4

Desktop

❌ - Change doesn't affect desktop

iOS

2022-05-18_14-12-08.mp4

Android

2022-05-20_08-18-44.mp4

marcaaron · 2022-05-18T01:56:38Z

Found a solution for web and mobile web (the edit comment feature needs improvement there) and still need to test iOS/Android.

Gonals

LGTM!

parasharrajat · 2022-05-26T16:33:22Z

Testing...

parasharrajat

BUG: Composer stays hidden on switching the chats.
Steps:

Open any chat on Android.
Edit any message.
Click hardware back button.
Open any other chat.
No Composer.

I was doing this #8255 to solve it.

parasharrajat · 2022-05-25T18:20:07Z

src/pages/home/report/ReportActionItemMessageEdit.js

-                        onPress={this.publishDraft}
+                        onPress={() => {
+                            this.publishDraft();
+                            toggleReportActionComposeView(true, VirtualKeyboard.shouldAssumeIsOpen());


This is indirectly happening on publishDraft => deleteDraft

I'm not following. What is happening exactly?

I meant that this is already being called in publishDraft. So this call is a duplicate.

Got it tried to remove it. Let me know if you see any issues? Tested Android and it worked well.

parasharrajat · 2022-05-25T18:20:13Z

src/pages/home/report/ReportActionItemMessageEdit.js

@@ -210,6 +210,14 @@ class ReportActionItemMessageEdit extends React.Component {
                            ReportScrollManager.scrollToIndex({animated: true, index: this.props.index}, true);
                            toggleReportActionComposeView(false, VirtualKeyboard.shouldAssumeIsOpen());
                        }}
+                        onBlur={(event) => {
+                            // Return to prevent re-render when save button is pressed which cancels the onPress event by re-rendering
+                            if (lodashGet(event, 'nativeEvent.relatedTarget.id') === 'saveButton') {


Should we move saveButton to CONST? may be not. As this would be local to the component.

Yeah, maybe it can be private to this file, but agree makes sense as a variable.

marcaaron · 2022-05-26T17:38:47Z

@parasharrajat Thanks! And that's interesting so... the hardware back button is not causing the input to lose focus? I'll do some testing now and see.

marcaaron · 2022-05-26T18:16:18Z

Ah ok so revealing the sidebar does not blur the input. The solution you shared only works if the ReportScreen unmounts and that doesn't happen when we navigate to the same chat. Maybe we can move the reset into the componentDidUpdate() of ReportActionsView 🤔

parasharrajat · 2022-05-26T18:22:03Z

Maybe we can move the reset into the componentDidUpdate() of ReportActionsView

Sounds like a potential solution. I will test more on other platforms to see if there are other such cases.

parasharrajat · 2022-05-27T18:38:54Z

Please merge main. There are conflicts.

marcaaron · 2022-06-01T00:49:50Z

Updated. Best solution I could come up with is to reset the composer whenever we set the currently viewed reportID. Which happens whenever we mount a new report screen or update the reportID.

parasharrajat · 2022-06-01T14:18:20Z

Thanks,I have fixed my Mac. I will review this shortly and we can close this.

parasharrajat · 2022-06-01T20:50:42Z

There are definitely some weird things happening when we are navigating back with the browser in mWeb. Mostly I am confused why we end up navigating to a previous chat instead of showing the drawer. The behavior seems kind of unexpected.

This happening as there is no back handler for mWeb like native. Pressing the back button navigates the page back to previous history item.

As the url is always r/reportID for both open/closed state. previous history item will always be r/reporid for another report. Thus it switch to another report on pressing back. The default status of Drawer is closed thus we see LHN with respect to change of report.

[BackHandler](https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/exports/BackHandler/index.js.) is not implemented on RN-web There is a good reason for that but I am not able to find the issue where maintainer explained it.

parasharrajat · 2022-06-01T20:55:31Z

package-lock.json

-      "integrity": "sha512-goW1b+d9q/HIwbVYZzZ6SsTr4IgE+WA44A0GmPIQstuOrgsFcT7VEJ48nmr9GaRtNu0XTKacFLGnBPAM6Afouw==",
+      "integrity": "sha1-1TswzfkxPf+33JoNR3CWqm0UXFA=",


This is strange change. I think you need to update your npm v6 to latest minor version.

This is unnecessary change for this PR. I also got the same in one of the PR and then upgrading to latest v6 of npm fixed it.

Ah hmm looks like someone already updated expensify-common then we don't need it.

marcaaron · 2022-06-01T21:02:06Z

This happening as there is no back handler for mWeb like native. Pressing the back button navigates the page back to previous history item.

Yes am aware of an open issue in react-native-web to implement the BackHandler and also some issues that react-navigation has related to browser back nav on web (it's using popstate events not the BackHandler and also has a specific implementation for web).

I think it could also have to do with the CustomActions stuff. Probably eventually would be good to:

Determine how browser navigation should work
See if react-navigation can reasonable support what we want
If not, then re-write the CustomActions or create a custom navigator for web
If yes, then kill the CustomActions

That all has to be designed and researched though and probably not a near-term priority.

parasharrajat

Rest changes looking good to me. Thanks for fixing the issue.

I agree, the back handler issue needs a proper discussion and analysis.

Reiterating issues that we ignored on this PR:

ScrollToBottom does not work on mWeb.

cc: @Beamanator

marcaaron · 2022-06-01T21:19:47Z

Yes, I think scrolling to the comment will not work until we solve the same issues we are struggling with here.

marcaaron · 2022-06-01T21:21:18Z

@Gonals Can you give this another quick look whenever you get a chance? 🙇

package-lock.json

Gonals

Tested well!

Gonals · 2022-06-07T12:32:22Z

@Beamanator all yours!

Beamanator

Interesting situation happening here in Android app. Steps:

Edit comment
Start adding emojis
In background, see that chats move up
Dismiss emoji picker
See that Report Action Compose is visible, but hides automatically

Note: I also reproduced on iOS, but the iOS simulator is much quicker and the ReportActionCompose component hides much quicker

Screen.Recording.2022-06-07.at.9.59.14.AM.mov

Beamanator · 2022-06-07T15:15:47Z

src/libs/EmojiUtils.js

@@ -77,7 +78,7 @@ function isSingleEmoji(message) {
 * @returns {Boolean}
 */
 function containsOnlyEmojis(message) {
-    const trimmedMessage = message.replace(/ /g, '').replaceAll('\n', '');
+    const trimmedMessage = Str.replaceAll(message.replace(/ /g, ''), '\n', '');


NAB; If .replaceAll is a native JS function (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) why do we prefer using a lib for it here?

Also, .replace(/ /g, '') is basically another .replaceAll(message, ' ', '') so why not replace that one too?

it's poly-filled and not available on some mobile browsers

Cool ya makes sense

Any thoughts on my second comment?

Also, .replace(/ /g, '') is basically another .replaceAll(message, ' ', '') so why not replace that one too?

Mostly added this to fix a bad crash with iOS 14 so that wasn't my priority. We could change the usages, but that doesn't stop people from making this mistake in the future. I also added an eslint rule to prevent people from using string.prototype.replaceAll.

src/pages/home/ReportScreen.js

src/pages/home/report/ReportActionCompose.js

Beamanator · 2022-06-07T15:43:55Z

src/pages/home/report/ReportActionItemMessageEdit.js

@@ -1,3 +1,4 @@
+import lodashGet from 'lodash/get';


NAB: Interested in using lodashGet in some other places in this component?

Ex:

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

Line 113 in cff0bce

this.debouncedSaveDraft(this.props.action.message[0].html);

Not in this PR unless that code is being modified already. My usual preference is to only make the changes that are necessary.

Beamanator · 2022-06-07T15:45:03Z

src/libs/toggleReportActionComposeView/index.js

@@ -1,6 +1,6 @@
 import * as Session from '../actions/Session';

-export default (shouldShowComposeInput, isSmallScreenWidth) => {
+export default (shouldShowComposeInput, isSmallScreenWidth = true) => {


Thinking out loud: index.js is for web & mobile (since we have a index.native.js file) so... should this default to false, not small screen width?

Thought about this lib name: This isn't really "toggling" anything, is it? It's just setting Session.setShouldShowComposeInput based on whatever is passed... Maybe it's due for a rename

Sorry, I don't really understand this concern. Are you seeing a bug somewhere or something that needs to be addressed?

Nope, no bug - this is related to my other 2 comments about web & desktop probably usually not being small screens, so it's weird to me that we default isSmallScreenWidth to true here.

Again, not a blocker & doesn't need to be changed until we see a bug somewhere so can resolve this

Beamanator · 2022-06-07T16:57:05Z

Also seeing this in iOS following this flow:

Send message, start editing (without sending)
Verify ReportActionCompose hides
Navigate away, then navigate back
See that the comment edit input & report action compose input both show

Screen.Recording.2022-06-07.at.10.55.24.AM.mov

marcaaron · 2022-06-07T17:03:20Z

See that the comment edit input & report action compose input both show

Yes, that is the expected behavior.

marcaaron · 2022-06-07T17:04:26Z

Hey @Beamanator - good thoughts here and thanks for the review. Nothing is a blocker for me and it's important to get this one merged. If you are passionate about any of the concerns you had in the review I think we can create a follow up to address them.

OSBotify · 2022-06-07T17:14:39Z

✋ 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 · 2022-06-08T16:56:59Z

🚀 Deployed to staging by @marcaaron in version: 1.1.74-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	cancelled 🔪
🕸 web 🕸	failure ❌

OSBotify · 2022-06-09T15:43:01Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.74-2 🚀

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

parasharrajat · 2022-07-18T14:59:16Z

src/pages/home/report/ReportActionItemMessageEdit.js

+                        onBlur={(event) => {
+                            // Return to prevent re-render when save/cancel button is pressed which cancels the onPress event by re-rendering
+                            if (_.contains([this.saveButtonID, this.cancelButtonID], lodashGet(event, 'nativeEvent.relatedTarget.id'))) {
+                                return;
+                            }
+
+                            toggleReportActionComposeView(true, VirtualKeyboard.shouldAssumeIsOpen());
+                        }}


@marcaaron Could you please help me understand this change? It has caused an issue #9252 and I am trying to figure out what is it doing? Especially What do I need to test for this?

What are you curious about exactly?

Want to maybe walk me through what you think it's doing?

So it is checking that if the Cancel or save button is clicked on the Editor, return, otherwise show the composer.

So do we want to show the composer on blur?

IIRC, in this case we are ignoring the blur event because when we are blurring because the cancel or save button is clicked the method to toggle the composer (toggleReportActionComposeView) would prevent the user from taking the actions. I assumed here (perhaps incorrectly) that it is related to re-rendering of the component - but the onPress method for the buttons never seemed to run. Let me know if that helps!

Got, It. Thanks.

Do we want to show the composer on blur?

Yes on blur and it should happen when we save changes or cancel the edit right? Can we have this conversation on whatever issue you are working on so I can get more insight into what you're trying to figure out?

Please bring whatever conversation you'd like to have to a relevant issue.

marcaaron added 2 commits May 17, 2022 12:49

add diff

f1a598d

add changes

06386db

marcaaron self-assigned this May 17, 2022

remove logs

43fb53c

marcaaron changed the title ~~Fix compose box resurfacing issue~~ [WIP] Fix compose box resurfacing issue May 18, 2022

marcaaron mentioned this pull request May 18, 2022

[No QA] Add polyfill for replaceAll() to fix Safari iOS 13 Expensify/expensify-common#447

Merged

marcaaron added 4 commits May 18, 2022 09:31

Use Str for replaceAll

4a77995

bump expensify-common

ed8d0aa

Fix iOS issue

f00048a

remove random newline

95e4723

marcaaron changed the title ~~[WIP] Fix compose box resurfacing issue~~ Fix compose box resurfacing issue May 20, 2022

marcaaron marked this pull request as ready for review May 20, 2022 19:06

marcaaron requested a review from a team as a code owner May 20, 2022 19:06

melvin-bot bot requested review from Gonals and removed request for a team May 20, 2022 19:06

marcaaron requested a review from parasharrajat May 25, 2022 17:07

marcaaron mentioned this pull request May 25, 2022

$1000 - When you edit a comment, the composer stays hidden on other chats - reported by @thesahindia #8031

Closed

marcaaron requested a review from Beamanator May 25, 2022 17:08

Gonals previously approved these changes May 26, 2022

View reviewed changes

parasharrajat suggested changes May 26, 2022

View reviewed changes

reset composer state when sidebar opens

46b7aa7

marcaaron dismissed Gonals’s stale review via 46b7aa7 May 26, 2022 18:28

marcaaron added 2 commits May 26, 2022 08:34

Also fix cancel button issue

9d73a57

cleanup

8cdd5e0

parasharrajat reviewed Jun 1, 2022

View reviewed changes

use main package-lock.json

f339b23

parasharrajat previously approved these changes Jun 1, 2022

View reviewed changes

fix conflict

7669502

marcaaron dismissed parasharrajat’s stale review via 7669502 June 2, 2022 01:13

marcaaron commented Jun 2, 2022

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

fix conflict

cff0bce

Gonals approved these changes Jun 7, 2022

View reviewed changes

Beamanator reviewed Jun 7, 2022

View reviewed changes

marcaaron merged commit 1af9250 into main Jun 7, 2022

marcaaron deleted the marcaaron-fixComposeBox branch June 7, 2022 17:04

OSBotify mentioned this pull request Jun 8, 2022

Deploy Checklist: New Expensify 2022-06-08 #9359

Closed

29 tasks

kavimuru mentioned this pull request Jun 8, 2022

Mobile/Small screen - Chat - Compose box is not appearing after tapped/dragged somewhere else #9364

Closed

parasharrajat reviewed Jul 18, 2022

View reviewed changes

aneequeahmad mentioned this pull request Aug 1, 2022

[$4000] Android - Edit message - keyboard is dismissed after selecting an emoji @Tushu17 #9252

Closed

This was referenced Mar 22, 2023

[HOLD for payment] [$2000] Wrong chat composer highlighted after clicking out of emoji modal #14848

Closed

[HOLD for payment 2023-06-02] [$4000] mWeb - Chat - Reopening Edit option highlights the Delete option in a blue frame #15105

Closed

		"integrity": "sha512-goW1b+d9q/HIwbVYZzZ6SsTr4IgE+WA44A0GmPIQstuOrgsFcT7VEJ48nmr9GaRtNu0XTKacFLGnBPAM6Afouw==",
		"integrity": "sha1-1TswzfkxPf+33JoNR3CWqm0UXFA=",

Fix compose box resurfacing issue #9060

Fix compose box resurfacing issue #9060

Conversation

marcaaron commented May 17, 2022 • edited Loading

Details

Fixed Issues

Tests & QA (On a mobile device or mWeb only)

PR Review Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented May 18, 2022

Gonals left a comment

Choose a reason for hiding this comment

parasharrajat commented May 26, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented May 26, 2022

marcaaron commented May 26, 2022

parasharrajat commented May 26, 2022

parasharrajat commented May 27, 2022 • edited Loading

marcaaron commented Jun 1, 2022

parasharrajat commented Jun 1, 2022

parasharrajat commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Jun 1, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

marcaaron commented Jun 1, 2022

marcaaron commented Jun 1, 2022

Gonals left a comment

Choose a reason for hiding this comment

Gonals commented Jun 7, 2022

Beamanator left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beamanator commented Jun 7, 2022

marcaaron commented Jun 7, 2022

marcaaron commented Jun 7, 2022

OSBotify commented Jun 7, 2022

OSBotify commented Jun 8, 2022

OSBotify commented Jun 9, 2022

parasharrajat Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

marcaaron commented May 17, 2022 •

edited

Loading

parasharrajat commented May 27, 2022 •

edited

Loading

parasharrajat Jun 1, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

Beamanator left a comment •

edited

Loading

parasharrajat Jul 18, 2022 •

edited

Loading