Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PAID] [$2000] Edit composer remains highlighted when emoji picker menu is opened. #14798

Closed
6 tasks
kavimuru opened this issue Feb 3, 2023 · 76 comments
Closed
6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Feb 3, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open the web app
  2. Open any report having text message.
  3. Click on edit message icon to edit message.
  4. Click on emoji picker icon in edit message composer.

Expected Result:

Edit composer shouldn’t be highlighted

Actual Result:

Edit composer remains highlighted.

Workaround:

unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.2.64-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Screen Shot 2023-01-31 at 4 41 46 PM
1

Expensify/Expensify Issue URL:
Issue reported by: @aneequeahmad
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675165374280019

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a58ed48d7251320e
  • Upwork Job ID: 1621680012125274112
  • Last Price Increase: 2023-02-22
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 3, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅) -- The behavior is not consistent with how the compose box works when clicking the emoji pickers so we should be consistent even if it's not impacting performance.
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs) - Looks unique.
  • This bug is reproducible using the reproduction steps in the OP. S/O
  • This issue is filled out as thoroughly and clearly as possible
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023
@strepanier03 strepanier03 added the External Added to denote the issue can be worked on by a contributor label Feb 4, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 4, 2023
@melvin-bot melvin-bot bot changed the title Edit composer remains highlighted when emoji picker menu is opened. [$1000] Edit composer remains highlighted when emoji picker menu is opened. Feb 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 4, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 4, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 4, 2023

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

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

melvin-bot bot commented Feb 4, 2023

Triggered auto assignment to @chiragsalian (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@tienifr
Copy link
Contributor

tienifr commented Feb 4, 2023

Proposal

This is due to we're preventing the execution of the this.setState({isFocused: false}); of the composer if the emoji button is clicked. The following will fix it:

diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 97fffebebc..ed42503ed3 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -243,10 +243,14 @@ class ReportActionItemMessageEdit extends React.Component {
                         }}
                         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, this.emojiButtonID], lodashGet(event, 'nativeEvent.relatedTarget.id'))) {
+                            if (_.contains([this.saveButtonID, this.cancelButtonID], lodashGet(event, 'nativeEvent.relatedTarget.id'))) {
                                 return;
                             }
                             this.setState({isFocused: false});
+
+                            if (this.emojiButtonID === lodashGet(event, 'nativeEvent.relatedTarget.id')) {
+                                return;
+                            }
                             toggleReportActionComposeView(true, this.props.isSmallScreenWidth);
                         }}
                         selection={this.state.selection}

Pls note that we still need to prevent the toggleReportActionComposeView(true, this.props.isSmallScreenWidth); in case of clicking the emoji button so that we don't reintroduce this bug.

I don't think we should move this.setState({isFocused: false}); above if (_.contains([this.saveButtonID, this.cancelButtonID] because there're totally valid cases where an input still has focus after a save button is clicked (Slack on web does it, when you click Send the composer is still focused). And those cases are not related to this issue and do not cause bugs (the edit composer will be closed after clicking on those buttons) so we shouldn't touch it.

If some time in the future we don't want to close the composer when clicking Save, then the correct behavior IMO is to keep the focus on the input after clicking Save so the user can do more edits if he/she wants to.

@tienifr
Copy link
Contributor

tienifr commented Feb 4, 2023

Working well after the fix

Screen.Recording.2023-02-04.at.09.33.57.mov

@tienifr
Copy link
Contributor

tienifr commented Feb 4, 2023

Furthermore, I also found another issue on mweb (not related to the fix and the current PR, but related for the highlighting composer issue). You can check it out in this slack thread

@jatinsonijs
Copy link
Contributor

Root Case

if (_.contains([this.saveButtonID, this.cancelButtonID, this.emojiButtonID], lodashGet(event, 'nativeEvent.relatedTarget.id'))) {
  return;
 }
 this.setState({isFocused: false});
 toggleReportActionComposeView(true, this.props.isSmallScreenWidth);

Due to above condition isFocused not set to false and Input still look like as focused. We are using this condition for different purpose (to prevent calling of toggleReportActionComposeView in specific condition).

Proposal

We should always set isFocus false on blur, because if onBlur is called that's mean Input is no more focused now, in this case user cannot type anything in it so there is no reason to show input as focused.

diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 97fffebebc..272392b941 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -242,11 +242,12 @@ class ReportActionItemMessageEdit extends React.Component {
                            toggleReportActionComposeView(false, this.props.isSmallScreenWidth);
                        }}
                        onBlur={(event) => {
+                            this.setState({isFocused: false});
+
                            // 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, this.emojiButtonID], lodashGet(event, 'nativeEvent.relatedTarget.id'))) {
                                return;
                            }
-                            this.setState({isFocused: false});
                            toggleReportActionComposeView(true, this.props.isSmallScreenWidth);
                        }}
                        selection={this.state.selection}

@aneequeahmad
Copy link
Contributor

@kavimuru Please mention my optional proposal from slack thread.

As a bug reporter my optional solution should be evaluated first. Thanks

@rushatgabhane
Copy link
Member

As a bug reporter my optional solution should be evaluated first. Thanks

@aneequeahmad if I'm not wrong, we don't have anything like that in our guidelines

@aneequeahmad
Copy link
Contributor

@rushatgabhane Please look at the guideline here it says you may optionally propose a solution for that job on Slack, solutions are ultimately reviewed in GitHub. The onus is on you to propose the solution on GitHub, and/or ensure the issue creator will include a link to your proposal.

I mentioned the issue creator @kavimuru in the slack thread to include a link to my proposal. It has happened for the 2nd or 3rd time that my optional solution is not included in the issue. Let me know if it makes sense or i'm missing something

@strepanier03
Copy link
Contributor

strepanier03 commented Feb 7, 2023

@aneequeahmad - I've reviewed the guidelines and it doesn't say that a solution proposed in Slack by the bug reporter is required to be considered before other proposals.

It does say that you can optionally propose a solution in Slack but it's your responsibility to ensure it's added to the issue created for the bug. In this case, we'd already received proposals on the GH by the time you posted your proposal in Slack and commented on the GH about your proposal.

I would recommend posting your proposal on the GH rather than in Slack and relying on a third party to post your solution.

@rushatgabhane
Copy link
Member

reviewing proposals

@aneequeahmad
Copy link
Contributor

@strepanier03 Please look at the slack thread here. I'm pretty sure that the bug reporter has the priority that their proposal is evaluated first.

t it's your responsibility to ensure it's added to the issue created for the bug

I tagged the issue reporter(@kavimuru) in the slack thread here on Feb 2nd to link my optional solution with the issue.

Again, i asked the issue reporter on Feb 4th to link my optional solution with Github issue when this issue was created
I can't comment on Github issue as it is locked before Help Wanted` label is added.

In this case, we'd already received proposals on the GH by the time you posted your proposal in Slack

I posted my proposal in slack on Jan 31st here.

I would recommend posting your proposal on the GH rather than in Slack and relying on a third party to post your solution.

Yes, you're right Github is the right place to post and evaluate proposals. But how can we make sure the bug reporter has the right to post proposal on Github when their is time difference and other factors. In my case When i woke up Help wanted label was added and there were proposals already posted here.

CC: @mallenexpensify

@strepanier03
Copy link
Contributor

I'm not able to send you another offer for that job so I need to make a new one but Upwork's job creation is down again.

2023-03-15_19-38-47

I'll try again in the morning and let you know here when I send the offer.

@aneequeahmad
Copy link
Contributor

@strepanier03 Oh yes, that's unusual. I'll wait for the new offer. Thanks

@aneequeahmad
Copy link
Contributor

@strepanier03 any update here ?

@strepanier03
Copy link
Contributor

@aneequeahmad I am still getting the error but I had some team members trying to help me.

You will see an offer without a job connected that you can ignore. My team member also created an actual job for this and I just hired you for it.

The job is https://www.upwork.com/jobs/~01a58b143d5995e2ff.

The contract you can ignore is from Kadie Alexander. They are going to close it so you might never see it but just in case you do you can ignore it.

@strepanier03
Copy link
Contributor

@rushatgabhane - Once the BugZero checklist is complete I'll be able to handle your Upwork pay out.

@aneequeahmad
Copy link
Contributor

aneequeahmad commented Mar 17, 2023

@strepanier03 accepted the offer. Also, reporting onus is missing in the offer.
Thank you for mentioning

@strepanier03
Copy link
Contributor

@aneequeahmad you should see a contract offer for the reporting bonus, can you take a look?

@strepanier03
Copy link
Contributor

Alright, just paid that out @aneequeahmad - you should be all set.

@rushatgabhane I'll check back in Monday to see if the checklist is finished.

@melvin-bot melvin-bot bot added the Overdue label Mar 20, 2023
@strepanier03
Copy link
Contributor

Just waiting on the BZ checklist to be completed @rushatgabhane @chiragsalian, then we can wrap this one up.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 20, 2023
@strepanier03
Copy link
Contributor

Bumped.

@rushatgabhane

This comment was marked as off-topic.

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 27, 2023

Hi @strepanier03 I've completed the checklist above ^ because I can't edit the original message. Apologies for the delay 🙇

@parasharrajat
Copy link
Member

parasharrajat commented Mar 27, 2023

Downvote on the above post, because I disagree with the tagged issue for the root cause. I can confirm that this feature was working on that PR. See Video #12095 (comment).

Maybe find the correct root cause.

@strepanier03
Copy link
Contributor

@rushatgabhane - I'll check back in tomorrow after you've had time to review @parasharrajat comment here.

@rushatgabhane
Copy link
Member

Yeah I was wrong. Thanks for the help @parasharrajat
I'll get down to the cause today

@strepanier03
Copy link
Contributor

Thank you Rushat 🙌 , if you want to @ mention me when you do I can make the update in the BZ checklist and finish paying this out to you.

@melvin-bot melvin-bot bot added the Overdue label Apr 3, 2023
@MelvinBot
Copy link

@strepanier03, @chiragsalian, @rushatgabhane, @aneequeahmad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@parasharrajat
Copy link
Member

Please remove the invalid comment on the issue https://github.com/Expensify/App/pull/12095/files#r1148725844 and the invalid checklist #14798 (comment) from this issue. Thanks. It helps keep the right context and stats. Thanks.

@rushatgabhane
Copy link
Member

@strepanier03 im not able to find the commit that caused this bug

@melvin-bot melvin-bot bot removed the Overdue label Apr 3, 2023
@strepanier03 strepanier03 changed the title [HOLD for payment 2023-03-16] [$2000] Edit composer remains highlighted when emoji picker menu is opened. [PAID] [$2000] Edit composer remains highlighted when emoji picker menu is opened. Apr 3, 2023
@strepanier03
Copy link
Contributor

I hid those comments from the incorrect PR and paid you out @rushatgabhane.

Thank you all for the work you did here and for helping each other out with the final steps. Closing now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants