[HOLD for payment][$1000] Web - The popup menu overlaps the + icon on room #21809

kbecciv · 2023-06-28T15:17:27Z

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:

Go to staging dot on web chrome and login with User A
Create a new room in public mode
Go to header and copy the share URL
Important step- Now click on + icon on the room so that a popup menu appears. Notice that the 'Attachment' is shown above the + icon (works well)- Do not click anywhere because it's important to have the popup opened
But now , open another web chrome and login with User B and paste the URL that you copied earlier so that User B can have access to the room
Now on User A side, notice that 'Split bill' also arises on popup and notice that the menu popup overlaps the + icon of the room.
If you click somewhere on the screen and again click on + icon, you'll notice this time, the menu doesn't overlaps the + icon and is shown above it

Expected Result:

The popup menu should not overlap the + icon on room when User B joins it

Actual Result:

The popup menu overlaps the + icon on room

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.33-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):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

green-2023-06-27_12.07.30.1.mp4

Recording.5211.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687846865181099

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a5b67e57313ec40b
Upwork Job ID: 1674159615707922432
Last Price Increase: 2023-08-13
Automatic offers:

abdulrahuman5196 | Reviewer | 25812254
priya-zha | Reporter | 25812259

melvin-bot · 2023-06-28T15:17:32Z

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

melvin-bot · 2023-06-28T15:17:35Z

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

kushu7 · 2023-06-28T15:29:26Z

Dupe of #20198
Issue is same about popover

priya-zha · 2023-06-28T15:41:24Z

This issue is about the position of popover over any chat when split bill is introduced when a user joins and and not about the fluctuating behaviour of popup menu.

sakluger · 2023-06-28T20:54:59Z

I agree with @priya-zha - this is a different popover (global-add vs in-chat-add) and also different behavior (the other issue is about switching between offline and online, this issue is about what happens when a new user joins a public room).

melvin-bot · 2023-06-28T20:55:33Z

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

melvin-bot · 2023-06-28T20:55:36Z

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

melvin-bot · 2023-06-28T20:55:42Z

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

ygshbht · 2023-06-28T22:21:35Z

Anyone else receiving this error? All my permissions are set to true

s-alves10 · 2023-06-29T00:06:23Z

Proposal

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

The popup menu overlaps the + icon on room when menu items are dynamically added

What is the root cause of that problem?

App/src/components/PopoverWithMeasuredContent.js

Line 185 in 5997357

onLayout={this.measurePopover}

In PopoverWithMeasuredContent component, onLayout is called only after the first render. So the width and height are not calculated dynamically when contents are changed, and anchor position won't be changed as well.

This is the root cause

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

We have to recalculate the width and height when contents are changed.

Introducing a new state variable children and remove isContentMeasured state

        this.state = {
            children: this.popoverWidth > 0 && this.popoverHeight > 0 ? props.children : null,
            isVisible: false,
        };

Replace the below code

App/src/components/PopoverWithMeasuredContent.js

Line 89 in be7b6a0

    
           return {isContentMeasured: lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0, isVisible: true};

with

            return {
                children: lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0 ? props.children : null,
                isVisible: true
            };

Replace the below code

App/src/components/PopoverWithMeasuredContent.js

Line 115 in be7b6a0

this.setState({isContentMeasured: true});

with

        this.setState({children: this.props.children});

Change the return part of the render function

        return (
            <>
                {this.state.children && (
                    <Popover
                        // eslint-disable-next-line react/jsx-props-no-spreading
                        {...this.props}
                        anchorPosition={shiftedAnchorPosition}
                    >
                        {this.state.children}
                    </Popover>
                )}
                {!_.isEqual(this.state.children, this.props.children) && (
                    <View
                        style={styles.invisible}
                        onLayout={this.measurePopover}
                    >
                        {this.props.children}
                    </View>
                )}
            </>
        );

This works as expected.

Result

21809.mp4

What alternative solutions did you explore? (Optional)

ExGraviton · 2023-06-29T09:47:14Z

I've checked the above proposal, it solves the issue, but it also creates a bug, the popup glitches for a quick second, it's not visible in the above video, because of lower FPS of the video.

Check this high FPS video, in which we can clearly see the glitch.

popup-position-glitch.mp4

Also the same result(as s-alves10's proposal) can be achieved by just adding the onLayout in the popup itself.

 <Popover
    // eslint-disable-next-line react/jsx-props-no-spreading
    {...this.props}
    anchorPosition={shiftedAnchorPosition}
 >
+    <View onLayout={this.measurePopover}>
        {this.props.children}
+    </View>
 </Popover>

edit: undo last edit

melvin-bot · 2023-06-29T09:47:17Z

📣 @ExGraviton! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

ExGraviton · 2023-06-29T09:48:27Z

Contributor details
Your Expensify account email: ExGraviton@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01c4d5a0214da52f73

melvin-bot · 2023-06-29T09:48:29Z

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

ExGraviton · 2023-06-29T09:50:16Z

Proposal

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

Popup menu overlaps the plus icon(actions button) when the menu is dynamically changed after rendering.

What is the root cause of that problem?

The popup menu uses PopoverWithMeasuredContent, which calculates the anchor position only once in the first render, the dynamic changes are not handled by PopoverWithMeasuredContent, which causes the popup to be placed in incorrect position.

App/src/components/PopoverWithMeasuredContent.js

Line 168 in 5997357

return this.state.isContentMeasured ? (

App/src/components/PopoverWithMeasuredContent.js

Lines 183 to 188 in 5997357

    
           <View 
        
               style={styles.invisible} 
        
               onLayout={this.measurePopover} 
        
           > 
        
               {this.props.children} 
        
           </View>

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

We can wrap the contents of Popover in a View with onLayout={this.measurePopover}.

 <Popover
   // eslint-disable-next-line react/jsx-props-no-spreading
   {...this.props}
   anchorPosition={shiftedAnchorPosition}
 >
+    <View onLayout={this.measurePopover}>
       {this.props.children}
+    </View>
  </Popover>

And move popoverWidth and popoverHeight to state.

What alternative solutions did you explore? (Optional)

Calculate the anchor position whenever the content changes.

We can make the following changes in PopoverWithMeasuredContent

Add a new state variable measuredChildren

{ measuredChildren: null }

Set state measuredChildren = this.props.children when the content is measured

this.setState({isContentMeasured: true, measuredChildren: this.props.children});    }

If this.props.children != measuredChildren, recalculate the position again.

return (
    <>
        {this.state.isContentMeasured && (
            <Popover
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...this.props}
                anchorPosition={shiftedAnchorPosition}
            >
                {this.state.measuredChildren || this.props.children}
            </Popover>
        )}
        {!_.isEqual(this.state.measuredChildren, this.props.children) && (
            <View
                style={styles.invisible}
                onLayout={this.measurePopover}
            >
                {this.props.children}
            </View>
        )}
    </>
);

Result(alternate proposal)

popup-position-fix.mp4

The alternate proposal also solves the above mentioned glitch.

sakluger · 2023-07-01T00:14:16Z

@abdulrahuman5196 what do you think of the proposals so far?

abdulrahuman5196 · 2023-07-02T10:06:44Z

@s-alves10 From the proposal here #21809 (comment)
I agree there is something going wrong with the position calculation. And I almost agree on the root cause.

But for the solution I wouldn't want to directly compare the children which is essentially a node and doesn't seem to be the optimised way. Maybe some other way of comparing?

abdulrahuman5196 · 2023-07-02T10:08:29Z

@ExGraviton I don't think we could consider the proposal here #21809 (comment) a valid new proposal, since it seems to be a minor optimization over the proposal here #21809 (comment)

ExGraviton · 2023-07-02T10:31:13Z

Hi @abdulrahuman5196, I also proposed that the same solution by just adding the onLayout in the popup itself, can we consider that?

 <Popover
   // eslint-disable-next-line react/jsx-props-no-spreading
   {...this.props}
   anchorPosition={shiftedAnchorPosition}
 >
+    <View onLayout={this.measurePopover}>
       {this.props.children}
+    </View>
  </Popover>

s-alves10 · 2023-07-02T12:22:09Z

@abdulrahuman5196

Yes. you're right, but PopoverWithMeasuredContent is a general component and there is no assumption on its children. I'm afraid there would no perfect way to compare them.

However, we have some other ways as described here. I think 3rd method is preferred.

Please let me know your thoughts

abdulrahuman5196 · 2023-07-05T14:22:19Z

@s-alves10 I am unable to understand what you are mentioning on the link?

If it is better could you include the same in your proposal and ping? So that i can review again?

abdulrahuman5196 · 2023-08-12T18:23:30Z

#21809 (comment)
cc: @sakluger

s-alves10 · 2023-08-12T23:28:55Z

@waterim 's solution was already discussed by other contributors. Changing anchor bottom or top isn't important here, I think. I really don't understand your decision.

@abdulrahuman5196
Please tell me if I missed something

s77rt · 2023-08-13T02:03:59Z

@abdulrahuman5196 Given you have submitted an approval on a proposal already I will be unassigning myself here. Feel free to tag me if anything. Thanks!

abdulrahuman5196 · 2023-08-13T06:42:21Z

@s-alves10

Changing anchor bottom or top isn't important here, I think.

Change the anchor position alone didn't work as you had already pointed out in slack(Which was their first solution), but their latest proposal with combined approach of layout measurement solves the issue. And currently it is the simple and best available solution so far.

On a side note: Regarding your proposal here #21809 (comment) , I am not aligned on the solution. Since it like we are trying to do solutions/workarounds to not directly compare children and also not aligned on the below point. Popover should be self-sufficient to re-align itself when its elements changes instead of parent providing the information.

I think the best way to do this is to let parent component determine when to measure.

s-alves10 · 2023-08-13T11:25:55Z

@abdulrahuman5196

Thank you for your explanation.

I'm not saying my solution should be selected. The selected solution has the one difference: anchor position. We had a similar discussions and simple solutions already.

I am not aligned on the solution. Since it like we are trying to do solutions/workarounds to not directly compare children and also not aligned on the below point. Popover should be self-sufficient to re-align itself when its elements changes instead of parent providing the information.

Maybe. But the parent knows when we should remeasure the wrapper exactly.

Anyway, I respect the decision. I wanted to say the review process should be fair. If the RCA is correct and the solutions are similar, I think the first proposal should be selected.

abdulrahuman5196 · 2023-08-13T11:45:22Z

I'm not saying my solution should be selected. The selected solution has the one difference: anchor position. We had a #21809 (comment) and simple solutions already.

Yes. I knew this already and pointed it out in slack as well.
https://expensify.slack.com/archives/C01GTK53T8Q/p1691086409147509?thread_ts=1691081056.624619&cid=C01GTK53T8Q

But without anchor position it is not working properly which i tested yesterday also. And the anchor position shift is not a minor change nor a PR level comment. So IMO the selected proposal can be considered a new proposal.

neil-marcellini · 2023-08-17T20:08:21Z

@waterim 's proposal here - #21809 (comment) looks good and works well.

I agree 👍

sakluger · 2023-08-23T21:33:50Z

Just summarizing where we are - @waterim's PR is still in review, should get merged after a few more tests.

The timeline is a bit funky on this one. @neil-marcellini approved the proposal on Aug 17, but there was already a draft PR on Aug 14. There won't be an efficiency bonus on this one, but given the complexity, there definitely shouldn't be a penalty either.

aimane-chnaif · 2023-08-23T21:38:52Z

@waterim is from callstack and agency PRs don't apply bonus/penalty timeline rule

melvin-bot · 2023-09-18T09:28:07Z

This issue has not been updated in over 15 days. @sakluger, @neil-marcellini, @waterim, @abdulrahuman5196 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

abdulrahuman5196 · 2023-09-29T07:00:32Z

@sakluger I think the payment is pending on this issue?

abdulrahuman5196 · 2023-09-29T07:01:44Z

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

Not regression

Determine if we should create a regression test for this bug.
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.

No. Minor case won't need any regression test

sakluger · 2023-09-29T07:39:03Z

Looks like the automation missed this GH issue. Thanks @abdulrahuman5196 for the bump in Slack.

Summarizing payouts for this issue:

Bug reporter: @priya-zha $250 (paid via Upwork)
Contributor: @waterim $1000 (contractor from Callstack, no payment required)
Contributor+: @abdulrahuman5196 $1000 (paid via Upwork)

@waterim could you please share your Upwork profile so that I can send you an offer?

aimane-chnaif · 2023-09-29T07:54:57Z

@waterim is from callstack. No upwork

abdulrahuman5196 · 2023-09-30T05:51:56Z

@sakluger We can close this issue.

sakluger · 2023-10-02T20:15:53Z

Got it, thanks for clarifying! Closing out 👍

kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 28, 2023

melvin-bot bot assigned sakluger Jun 28, 2023

sakluger added the External Added to denote the issue can be worked on by a contributor label Jun 28, 2023

melvin-bot bot changed the title ~~Web - The popup menu overlaps the + icon on room~~ [$1000] Web - The popup menu overlaps the + icon on room Jun 28, 2023

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

melvin-bot bot assigned abdulrahuman5196 Jun 28, 2023

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

melvin-bot bot removed the Overdue label Jul 5, 2023

s77rt removed their assignment Aug 13, 2023

waterim mentioned this issue Aug 14, 2023

The popup menu overlaps the + icon on room #24542

Merged

58 tasks

sakluger assigned abdulrahuman5196 Aug 14, 2023

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Help Wanted Apply this label when an issue is open to proposals by contributors Daily KSv2 labels Aug 14, 2023

abdulrahuman5196 mentioned this issue Aug 27, 2023

Payment issue for E/App PR#24542 #25942

Closed

melvin-bot bot removed the Weekly KSv2 label Sep 18, 2023

melvin-bot bot added the Monthly KSv2 label Sep 18, 2023

sakluger added Daily KSv2 and removed Monthly KSv2 labels Sep 29, 2023

sakluger changed the title ~~[$1000] Web - The popup menu overlaps the + icon on room~~ [HOLD for payment][$1000] Web - The popup menu overlaps the + icon on room Sep 29, 2023

sakluger closed this as completed Oct 2, 2023

[HOLD for payment][$1000] Web - The popup menu overlaps the + icon on room #21809

[HOLD for payment][$1000] Web - The popup menu overlaps the + icon on room #21809

Comments

kbecciv commented Jun 28, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023 • edited by sakluger Loading

Bug0 Triage Checklist (Main S/O)

kushu7 commented Jun 28, 2023

priya-zha commented Jun 28, 2023

sakluger commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

melvin-bot bot commented Jun 28, 2023

ygshbht commented Jun 28, 2023 • edited Loading

s-alves10 commented Jun 29, 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)

ExGraviton commented Jun 29, 2023 • edited Loading

melvin-bot bot commented Jun 29, 2023

ExGraviton commented Jun 29, 2023

melvin-bot bot commented Jun 29, 2023

ExGraviton commented Jun 29, 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)

sakluger commented Jul 1, 2023

abdulrahuman5196 commented Jul 2, 2023

abdulrahuman5196 commented Jul 2, 2023

ExGraviton commented Jul 2, 2023 • edited Loading

s-alves10 commented Jul 2, 2023 • edited Loading

abdulrahuman5196 commented Jul 5, 2023

abdulrahuman5196 commented Aug 12, 2023

s-alves10 commented Aug 12, 2023 • edited Loading

s77rt commented Aug 13, 2023 • edited Loading

abdulrahuman5196 commented Aug 13, 2023

s-alves10 commented Aug 13, 2023 • edited Loading

abdulrahuman5196 commented Aug 13, 2023

neil-marcellini commented Aug 17, 2023

sakluger commented Aug 23, 2023

aimane-chnaif commented Aug 23, 2023

melvin-bot bot commented Sep 18, 2023

abdulrahuman5196 commented Sep 29, 2023

abdulrahuman5196 commented Sep 29, 2023

sakluger commented Sep 29, 2023 • edited Loading

aimane-chnaif commented Sep 29, 2023

abdulrahuman5196 commented Sep 30, 2023

sakluger commented Oct 2, 2023

kbecciv commented Jun 28, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jun 28, 2023 •

edited by sakluger

Loading

ygshbht commented Jun 28, 2023 •

edited

Loading

s-alves10 commented Jun 29, 2023 •

edited

Loading

ExGraviton commented Jun 29, 2023 •

edited

Loading

ExGraviton commented Jun 29, 2023 •

edited

Loading

ExGraviton commented Jul 2, 2023 •

edited

Loading

s-alves10 commented Jul 2, 2023 •

edited

Loading

s-alves10 commented Aug 12, 2023 •

edited

Loading

s77rt commented Aug 13, 2023 •

edited

Loading

s-alves10 commented Aug 13, 2023 •

edited

Loading

sakluger commented Sep 29, 2023 •

edited

Loading