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

[kbecciv]

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. Go to staging dot on web chrome and login with User A
2. Create a new room in public mode
3. Go to header and copy the share URL
4. 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
5. 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
6. 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.
7. 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?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

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

https://github.com/Expensify/App/assets/93399543/d132a818-32c3-4d38-a355-df0bfe34f660

https://github.com/Expensify/App/assets/93399543/79b704e2-ede9-4d13-bec0-c7669da7247e

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
2023-07-26
• Automatic offers:
• abdulrahuman5196 | Reviewer | 25812254
• priya-zha | Reporter | 25812259

[ShogunFire]

I am pretty sure Firefox is not supported anymore: https://expensify.slack.com/archives/C01GTK53T8Q/p1688112331063719?thread_ts=1688096088.194439&cid=C01GTK53T8Q

[abdulrahuman5196]

It is not a browser specific change. Anyways, I tested again in chrome, still not working

https://github.com/Expensify/App/assets/46707890/ec62c1d9-8aa9-4c2d-a1cb-20ba11d69cda

Code change I made

Also you where switching tabs in your result video - https://github.com/Expensify/App/issues/21809#issuecomment-1627884903 By which it might would work, not working by keeping both side by side.

[ShogunFire]

Interesting thanks

[s-alves10]

Proposal

Updated

@abdulrahuman5196 I added some optimization and works perfect now

[melvin-bot[bot]]

@sakluger @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[ExGraviton]

@ExGraviton your proposal here main solution #21809 (comment) is not working for me.

@abdulrahuman5196 I think it's not working because the popoverWidth and popoverHeight are not in the state https://github.com/Expensify/App/blob/59973575da78597ccac89781b4d7117110b82078/src/components/PopoverWithMeasuredContent.js#L66-L67

Can you check it again after removing shouldComponentUpdate, if it works then we can move popoverWidth and popoverHeight to the state https://github.com/Expensify/App/blob/59973575da78597ccac89781b4d7117110b82078/src/components/PopoverWithMeasuredContent.js#L97-L105

[ExGraviton]

Proposal

Updated

[sakluger]

If @ExGraviton's updated proposal does not solve the issue, then I'll increase the bounty or check with our expert contributors for interest.

[s-alves10]

@abdulrahuman5196 What do you think of my proposal?

[abdulrahuman5196]

Ok. I will recheck the proposals in a while.

[sakluger]

@abdulrahuman5196 were you able to check the updated proposal? I'd like to make sure we have a look at that before making any changes.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[abdulrahuman5196]

Reviewing this now

[melvin-bot[bot]]

@sakluger @abdulrahuman5196 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

[abdulrahuman5196]

@ExGraviton From the information here - https://github.com/Expensify/App/issues/21809#issuecomment-1642862972

I don't see any information why we want to make this change to solve the issue? like adding popover height to state or similar?

[abdulrahuman5196]

@s-alves10

We don't need shouldComponentUpdate in the component as you can be noticed from its code

IMO i don't think we should remove it. https://github.com/Expensify/App/blob/be7b6a09cde880cd96470e4e9e0c2f921499dd20/src/components/PopoverWithMeasuredContent.js#L102-L104

[abdulrahuman5196]

@sakluger I am still not convinced on the current proposals. I have asked some questions on the same. Meanwhile since the issue has already become internal. Can we check with expert groups as well to see if we get a better proposal as well.

[s-alves10]

@abdulrahuman5196

As you can see from the code, we compare all props and state to determine if we should update component with one exception isVisible state. I thought this is exactly what react reconciliation does. That wa's the reason.

Yeah, isVisible is important and we need to have it!

[s-alves10]

Proposal

Updated

[ExGraviton]

@abdulrahuman5196

@ExGraviton From the information here - #21809 (comment)

I don't see any information why we want to make this change to solve the issue? like adding popover height to state or similar?

When measurePopover is called the second time, isContentMeasured is unchanged, so even if popoverWidth and/or popoverHeight is changed (which are not state variables) the state remains the same, and we are preventing re-rendering if the state is unchanged in shouldComponentUpdate.

So, basically popoverWidth and popoverHeight is updated correctly, but the render is not called, so it remains in the previous position.

Moving popoverWidth and popoverHeight to state would correctly detect the changes, and render it again in the correct position.

I suggested to remove shouldComponentUpdate was just for quick testing, we actually don't need to remove it.

[sakluger]

Just asked for our expert contributors to take a look, we'll see what they think!

[s-alves10]

@abdulrahuman5196 @sakluger

RCA is clear and we have the solution working perfect. What blocks us to approve the proposals?

[waterim]

Hi! I'm Artem from Callstack - expert contributor group Maybe I can help with this one?

[sakluger]

@s-alves10 - @abdulrahuman5196 still has some concerns around the proposal which is why we're considering another proposal.

@waterim thanks! I've assigned you, please post your proposal as soon as it's ready to review.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @priya-zha 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[s-alves10]

@abdulrahuman5196

You didn't tell me about your concerns and asked for help from others all of a sudden. We have spent much time on this issue as you know. I'm a bit disappointed.

[neil-marcellini]

@sakluger why did I get assigned? Are we taking this internal or has the C+ approved a proposal that I should review?

[abdulrahuman5196]

Reviewing today

[waterim]

Not a proposal! Just one question: Why this proposal was not accepted? Because of this small glitch?

[waterim]

Update: Have an idea, will post a proposal tomorrow

[sakluger]

@neil-marcellini you were added because I assigned @waterim.

It seems like there's some confusion on this issue, so before we move forward, I want to make sure we're going the right direction. @abdulrahuman5196 I could use your advice here on the best path forward - both @s-alves10 and @ExGraviton offered updated proposals around the time that I asked for help from waterim. @abdulrahuman5196 do you think that either of the latest proposals from those two would work here?

[abdulrahuman5196]

Yes. Doing a another check now

[abdulrahuman5196]

Going back last I checked the proposal was not working/I had posted a concern on the proposal.

Checking both updated proposal again,

On @s-alves10 's proposal here https://github.com/Expensify/App/issues/21809#issuecomment-1612257519, We are storing the children in a state and doing a re-computation when the children changes. For this I had raised concern previously as well, I am not super inclinded to do a direct children comparision and update the component accordingly. Since any minor change in children will start a re-measurement. Last I checked this proposal also had a visible glitch issue, assuming it should have been solved by the latest updated. Still the above concern holds.

On @ExGraviton 's proposal here https://github.com/Expensify/App/issues/21809#issuecomment-1612735374 The main proposal was not working last time and it has been updated now. But since we are adding onLayout to Popover itself, basically everytime the popover itself layouts we will be doing the measurement again. And alternate proposal is similar to the proposal above.

Note: I couldn't directly test if the updated proposals are working since the popover is migrated to functional component.

So far the best approach we got is to compare children directly and update the component. But not convinced if thats the best path.

IMO best path forward, we should choose a way where we don't put more stress on the Popover component and also fixes this issue. If the only way is to put stress on the Popover, I would suggest Do nothing here, since the issue is super edge case

@neil-marcellini Let me know if we should take it differently. (Since you have been assigned here already 😮‍💨 )

[waterim]

Proposal

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

The popup menu overlaps the + icon on room when children is adding dynamically.

What is the root cause of that problem?

https://github.com/Expensify/App/blob/6c9e459f8114f1cd2986a83ede1d73fb150aa366/src/components/PopoverWithMeasuredContent.js#L152 In PopoverWithMeasuredContent component, onLayout is called only on an invisible View for a first render setting isContentMeasured to true. Thats why width and height are not calculated dynamically when children(Menu Items) is changing.

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

The simplest solution here will be to change stylings for shiftedAnchorPosition: change top shift to bottom because from bottom we don't need to change a position of the popover each time menuItems changes. https://github.com/Expensify/App/blob/6c9e459f8114f1cd2986a83ede1d73fb150aa366/src/components/PopoverWithMeasuredContent.js#L131-L133 This part of the code should look like this:

    const shiftedAnchorPosition = {
        left: adjustedAnchorPosition.left + horizontalShift,
        bottom: windowHeight - (adjustedAnchorPosition.top + popoverHeight) - verticalShift,
    };

This solution fixes the issue with recalculating and glitches and works smoothly on all Popovers.

Example how it works:

https://github.com/Expensify/App/assets/39777589/631ef1bf-43aa-4051-b695-053e9a70e390

What alternative solutions did you explore?

For alternative solution we can wrap the Popover child in a View with onLayout={measurePopover}, like this:

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

This solutions also works good, but it can cause some random glitches sometimes when MenuItems is changing.

[s-alves10]

This component is used for EmojiPicker as well. They can be positioned at any position and we have to calculate the width and height of the children

[s-alves10]

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 updated

What is the root cause of that problem?

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?

PopoverWithMeasuredContent is a general component and we need to have a way to measure its layout dynamically. One way to do this is to compare the children props but this leads to redundant recalculation even for minor change.

Considering this component is general-purpose, I think the best way to do this is to let parent component determine when to measure.

1. Add new props shouldMeasure(default = false) and onMeasured(default = undefined) to the PopoverWithMeasuredContent component

2. Introduce a new state variable children(this is for removing glitches)

   const [children, setChildren] = useState(() => isContentMeasured ? props.children : null);

3. If shouldMeasure props changes to true, we think it's needed to measure layout again

   useEffect(() => {
       if (props.shouldMeasure) {
           setIsContentMeasured(false);
       }
   }, [props.shouldMeasure]);

4. In measurePopover function, add the following code

       setChildren(props.children);
       if (props.onMeasured) {
           props.onMeasured();
       }

   where onMeasure callback is used to notify parent that contents is measured

5. Update the return part. This code removes the glitches with the help of state children

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

6. In PopoverMenu component, add the following code and pass shouldMeasure and onMeasured as props to PopoverWithMeasuredContent component.

   const [shouldMeasure, setShouldMeasure] = useState(false);
   
   useEffect(() => {
       setShouldMeasure(true);
   }, [props.menuItems.length]);
   
   const onMeasured = () => {
       setShouldMeasure(false);
   }

   We set shouldMeasure state to true when props.menuItems length changes

Using this solution, we can determine when to measure layout based on its own data. This improves the reusability of the PopoverWithMeasuredContent component.

Also, the solution works perfect without any glitches

Result

https://github.com/Expensify/App/assets/126839717/3452c1d9-653d-4c78-86b8-d1d135b8fe5e

What alternative solutions did you explore? (Optional)

[abdulrahuman5196]

@waterim I agree on the point here https://github.com/Expensify/App/issues/21809#issuecomment-1661497263 by @s-alves10 , shifting positions might not solve all cases.

[abdulrahuman5196]

@neil-marcellini Could you kindly check on this comment here - https://github.com/Expensify/App/issues/21809#issuecomment-1660757309. Maybe should we Do Nothing here or go on a track?

[s-alves10]

@abdulrahuman5196

What do you think about my new proposal? That is a new idea

cc @neil-marcellini

[waterim]

@abdulrahuman5196 Actually, I tested all popovers and works exactly the same as before. All the difference here that we are not recalculating each time the position of the Popover from the top(as it is right now), we are just recalculating it from the bottom, but from the bottom it will not change after we are adding a new MenuItem thats why we don't have any glitches and issues with UI part. You can see how it works on the video together with EmojiPicker

[s-alves10]

@waterim

Please test with emoji picker on messages, not on composer

[waterim]

@s-alves10 As I can see it works exactly the same

[s-alves10]

Please make a long chat history and try to hover any chat at the top of the screen and click emoji picker

[waterim]

@s-alves10 Same as before

[s-alves10]

Can you please record a video?