[HOLD for payment 2023-08-03] [$1000] Emoji picker's position changes on decreasing the screen size #17603

kavimuru · 2023-04-18T17:12:44Z

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:

Open the app
Go to any chat > Open Emoji Picker
Decrease the screen size like shown in video
Notice that emoji picker's position changes

Expected Result :

The emoji picker jumps to the LHN instead of behaving the correct way (as in the second video).

Actual Result :

Emoji picker's position changes

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.1
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

Current:

Screen.Recording.2023-04-18.at.12.31.19.AM.mov

Expected:

Recording.270.mp4

Expensify/Expensify Issue URL:
Issue reported by: @daraksha-dk
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681758191524309

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019f4f66f49e6e10ed
Upwork Job ID: 1676073590122438656
Last Price Increase: 2023-07-04

MelvinBot · 2023-04-18T17:12:51Z

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

sophiepintoraetz · 2023-04-19T03:38:31Z

This is not an issue but expected behaviour due to the screen resizing.

kadiealexander · 2023-07-03T06:49:22Z

Hey Sophie! The first video in the OP is the one that shows the bug. The emoji picker jumps to the LHN instead of behaving the correct way (as in the second video). This was also reported here but I'm reopening this one instead to ensure the original bug reporter is credited.

samh-nl · 2023-07-03T07:20:59Z

Proposal

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

The emoji picker positions itself to the top-left corner of the screen if the window dimensions change.

What is the root cause of that problem?

The anchor referenced by emojiPopoverAnchor.current (EmojiPicker.js) no longer exists. The context menu is used as the anchor, however after clicking "Add reaction" the context menu is closed, thereby removing the anchor and making repositioning based on the anchor position impossible.

App/src/components/EmojiPicker/EmojiPicker.js

Lines 42 to 46 in 5a713dd

    
           const emojiPopoverDimensionListener = Dimensions.addEventListener('change', () => { 
        
               calculateAnchorPosition(emojiPopoverAnchor.current).then((value) => { 
        
                   setEmojiPopoverAnchorPosition(value); 
        
               }); 
        
           });

This causes the position to be x=0, y=0.

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

We need to use the same anchor that is used by BaseReportActionContextMenu (which is the chat message).

What alternative solutions did you explore? (Optional)

N/A

Originally posted by @samh-nl in #21982 (comment)

melvin-bot · 2023-07-04T03:41:00Z

Job added to Upwork: https://www.upwork.com/jobs/~019f4f66f49e6e10ed

melvin-bot · 2023-07-04T03:41:04Z

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

melvin-bot · 2023-07-04T03:41:09Z

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

sophiepintoraetz · 2023-07-04T03:41:10Z

Thanks, @kadiealexander - that description helped!

The emoji picker jumps to the LHN instead of behaving the correct way (as in the second video).

alitoshmatov · 2023-07-04T06:59:49Z

Proposal

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

What is the root cause of that problem?

This is caused by this line where we are checking if device is touchscreen and medium size, if so we are removing emoji picker button

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

Line 1123 in ecfcd83

    
           {DeviceCapabilities.canUseTouchScreen() && this.props.isMediumScreenWidth ? null : (

Thus when this button disappears emoji picker popover will lose it is target position

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

I am not sure about this line:

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

Line 1123 in ecfcd83

    
           {DeviceCapabilities.canUseTouchScreen() && this.props.isMediumScreenWidth ? null : (

It is not logical to me, for example in mid sized tablets we do not show emoji picker button, how are they going to use it then?
This is image of ipad mini in chrome responsive mode

I think we should remove this condition and want to show emoji picker button always.

What alternative solutions did you explore? (Optional)

Also we can hide emoji picker if anchor element is unmounted. I checked if emojiPopoverAnchor.current will be null if referenced element is unmounted but no it is still pointing to the element, but the element is not present in dom.

We can check if element is visible and if not we can hide emoji picker, like this:

if (!emojiPopoverAnchor.current.checkVisibility()) {
                hideEmojiPicker(false);
                return;
            }

hoangzinh · 2023-07-05T13:55:55Z

Proposal

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

Emoji picker's position changes on decreasing the screen size

What is the root cause of that problem?

If we're on tablet device (touchable device & medium screen), we won't show EmojiPickerButton:

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

Lines 1147 to 1155 in e753f4d

    
           {DeviceCapabilities.canUseTouchScreen() && this.props.isMediumScreenWidth ? null : ( 
        
               <EmojiPickerButton 
        
                   isDisabled={isBlockedFromConcierge || this.props.disabled} 
        
                   onModalHide={() => { 
        
                       this.focus(true); 
        
                   }} 
        
                   onEmojiSelected={this.addEmojiToTextBox} 
        
               /> 
        
           )}

We have this logic since this GH issue [HOLD for payment 2022-03-24] Tablet landscape - It's difficult to select emoji in tablet - reported by @Tushu17 #7585

Back to our case, if we're in the Chrome dev tool and change to touchable view, then shrink to medium screen, the EmojiPickerButton will be disappear => It leads to calculate anchor position of emojiPopoverAnchor.current incorrectly

App/src/components/EmojiPicker/EmojiPicker.js

Lines 43 to 45 in e753f4d

    
           calculateAnchorPosition(emojiPopoverAnchor.current).then((value) => { 
        
               setEmojiPopoverAnchorPosition(value); 
        
           });

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

To fix this issue #21982 as well. I think we can expose another method called clearEmojiPopoverAnchor here and call this method inside componentWillUnmount lifecycle of Open EmojiPicker buttons. There are only 3 buttons so it's not really an issue

Then in window size change callback, we check if emojiPopoverAnchor.current is cleared, we skip recalculate its position and hide the emoji picker.

What alternative solutions did you explore? (Optional)

I think if the EmojiPickerButton is disappear/unmounted in this case, we can hide the Emoji picker too. In order to do this, we can call the util EmojiPickerAction.hideEmojiPicker() inside the componentWillUnmount (or useEffect with return function) of EmojiPickerButton => That means, when the EmojiPickerButton is unmounted by somehow, it will hide the Emoji picker too.

    useEffect(() => () => {
        EmojiPickerAction.hideEmojiPicker()
    }, []);

Result:

Screen.Recording.2023-07-10.at.23.25.58.mp4

sophiepintoraetz · 2023-07-06T03:48:27Z

Just waiting on @allroundexperts to review!

allroundexperts · 2023-07-06T04:55:18Z

I can't reproduce this @sophiepintoraetz. Can you please confirm?

alitoshmatov · 2023-07-06T05:28:26Z

@allroundexperts You have to simulate in responsive mode of chrome, this issue is reproducible only in medium sized screen and touchscreen enabled

allroundexperts · 2023-07-06T06:36:56Z

@allroundexperts You have to simulate in responsive mode of chrome, this issue is reproducible only in medium sized screen and touchscreen enabled

@alitoshmatov Can you please share a video of how you're reproducing this behaviour?

alitoshmatov · 2023-07-06T06:38:59Z

@allroundexperts

Screen.Recording.2023-07-06.at.11.38.11.AM.mov

allroundexperts · 2023-07-06T06:43:23Z

Thanks @alitoshmatov!

allroundexperts · 2023-07-06T07:41:20Z

@alitoshmatov's proposal looks good to me.

@sophiepintoraetz We can either hide the emoji picker menu when the button is hidden OR we can make the button always visible. I prefer to hide the emoji picker menu when the button gets hidden instead of showing the button always. @sophiepintoraetz Can you please confirm the expected behaviour here?

In either case, @alitoshmatov's proposal works well.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2023-07-06T07:41:27Z

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

hoangzinh · 2023-07-13T11:43:54Z

@allroundexperts @Li357 Thanks for accepting my proposal. The PR is ready #22792. Please help me review it. Thanks

melvin-bot · 2023-07-20T20:10:30Z

@hoangzinh, @Li357, @allroundexperts, @sophiepintoraetz Whoops! This issue is 2 days overdue. Let's get this updated quick!

sophiepintoraetz · 2023-07-24T04:05:24Z

Still in review.

melvin-bot · 2023-07-27T21:29:18Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-07-27T21:29:21Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.46-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Fix Emoji picker's position changes on decreasing the screen size #22792

If no regressions arise, payment will be issued on 2023-08-03. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@hoangzinh requires payment
@allroundexperts does not require payment (Eligable for Manual Requests)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-07-27T21:29:23Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
[@allroundexperts] 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:
[@allroundexperts] 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:
[@allroundexperts] Determine if we should create a regression test for this bug.
[@allroundexperts] 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.
[@sophiepintoraetz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

allroundexperts · 2023-07-31T02:10:33Z

Checklist

remove emoji picker menu from tablets #7851
https://github.com/Expensify/App/pull/7851/files#r1278691903
I think a Slack discussion is not needed here as this was an edge case that requires someone to change his window size.
Similar to the third point, a regression test might be un-necessary here as this was a rare case where someone would change his window size while keeping the emoji picker open.

sophiepintoraetz · 2023-08-02T05:26:05Z

Payouts due (I believe the date for the bonus calculation is correct):

Issue Reporter: $250 @daraksha-dk
Contributor: $500 @hoangzinh r (assigned 13 Jul - PR merged 26 Jul = 14 working days lapsed)
Contributor+: $500 @ @allroundexperts (assigned 13 Jul - PR merged 26 Jul = 14 working days lapsed)

Eligible for 50% #urgency bonus? N (penalty applied)

hoangzinh · 2023-08-02T16:28:42Z

@sophiepintoraetz could we consider again the penalty in this case? Reasons:

I started to implement the proposal after got assigned within 24h.
During implement, I found the original proposal can cause regression in other cases => I had to spend time to research, then discussed with @allroundexperts .
@allroundexperts and I was actively working on the PR. We didn't intentionally delay the PR merging

sophiepintoraetz · 2023-08-03T03:19:07Z

@hoangzinh I appreciate the ask but I do not think that's fair to other individuals who submitted proposals and could have completed this job/other contributors who have been penalised for the same reasons. I am glad that you were thorough but we do make the guidelines clear - I would suggest taking more care in your original proposal!

allroundexperts · 2023-08-03T03:23:28Z

I would agree with @sophiepintoraetz on this one. Though just for record, it did not take 14 working days to get this merged. This was merged within 9 days (excluding the weekends).

sophiepintoraetz · 2023-08-03T03:33:44Z

@allroundexperts - sorry - I used a calculator which I thought was accurate, you're right in that it wasn't 14 days but am I correct in saying that there are 10 days between assignment and merging?

allroundexperts · 2023-08-03T03:39:06Z

I used this to calculate the exact period. This is pretty accurate AFAIK.

sophiepintoraetz · 2023-08-03T04:31:23Z

Got it. Moot point seeing as we're in agreement but I appreciate the accuracy. Reconfirming that the payments will be
Issue Reporter: $250 @daraksha-dk
Contributor: $500 @hoangzinh r (assigned 13 Jul - PR merged 26 Jul = 14 working days lapsed)
Contributor+: $500 @ @allroundexperts (assigned 13 Jul - PR merged 26 Jul = 14 working days lapsed)

sophiepintoraetz · 2023-08-03T04:34:07Z

Offers sent to @daraksha-dk and @hoangzinh

daraksha-dk · 2023-08-03T06:52:29Z

Accepted the offer. Thanks @sophiepintoraetz

hoangzinh · 2023-08-03T08:00:47Z

Accepted the offer. Thanks @sophiepintoraetz

sophiepintoraetz · 2023-08-04T04:34:54Z

Payments done - @allroundexperts - just drop the request here when you're ready!

allroundexperts · 2023-08-04T05:45:30Z

Requested. Thanks!

JmillsExpensify · 2023-08-08T03:03:03Z

Reviewed the above payment details for @allroundexperts. This is approved for payment in NewDot.

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 18, 2023

melvin-bot bot assigned sophiepintoraetz Apr 18, 2023

This comment was marked as off-topic.

Sign in to view

sophiepintoraetz closed this as completed Apr 19, 2023

daraksha-dk mentioned this issue Jul 1, 2023

Web - Resizing browser triggers reaction emoji picker to shift to LHN #21982

Closed

6 tasks

kadiealexander reopened this Jul 3, 2023

sophiepintoraetz added the External Added to denote the issue can be worked on by a contributor label Jul 4, 2023

melvin-bot bot changed the title ~~Emoji picker's position changes on decreasing the screen size~~ [$1000] Emoji picker's position changes on decreasing the screen size Jul 4, 2023

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

melvin-bot bot assigned allroundexperts Jul 4, 2023

melvin-bot bot assigned Li357 Jul 6, 2023

melvin-bot bot added the Reviewing Has a PR in review label Jul 13, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jul 27, 2023

melvin-bot bot changed the title ~~[$1000] Emoji picker's position changes on decreasing the screen size~~ [HOLD for payment 2023-08-03] [$1000] Emoji picker's position changes on decreasing the screen size Jul 27, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Jul 27, 2023

allroundexperts mentioned this issue Jul 31, 2023

remove emoji picker menu from tablets #7851

Merged

7 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 3, 2023

sophiepintoraetz closed this as completed Aug 4, 2023

[HOLD for payment 2023-08-03] [$1000] Emoji picker's position changes on decreasing the screen size #17603

[HOLD for payment 2023-08-03] [$1000] Emoji picker's position changes on decreasing the screen size #17603

Comments

kavimuru commented Apr 18, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result :

Actual Result :

Workaround:

Platforms:

MelvinBot commented Apr 18, 2023

This comment was marked as off-topic.

sophiepintoraetz commented Apr 19, 2023

kadiealexander commented Jul 3, 2023 • edited Loading

samh-nl commented Jul 3, 2023

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)

melvin-bot bot commented Jul 4, 2023

melvin-bot bot commented Jul 4, 2023

melvin-bot bot commented Jul 4, 2023

sophiepintoraetz commented Jul 4, 2023

alitoshmatov commented Jul 4, 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)

hoangzinh commented Jul 5, 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)

sophiepintoraetz commented Jul 6, 2023

allroundexperts commented Jul 6, 2023

alitoshmatov commented Jul 6, 2023

allroundexperts commented Jul 6, 2023

alitoshmatov commented Jul 6, 2023

allroundexperts commented Jul 6, 2023

allroundexperts commented Jul 6, 2023

melvin-bot bot commented Jul 6, 2023

hoangzinh commented Jul 13, 2023

melvin-bot bot commented Jul 20, 2023

sophiepintoraetz commented Jul 24, 2023

melvin-bot bot commented Jul 27, 2023

melvin-bot bot commented Jul 27, 2023 • edited by sophiepintoraetz Loading

melvin-bot bot commented Jul 27, 2023 • edited by sophiepintoraetz Loading

allroundexperts commented Jul 31, 2023

Checklist

sophiepintoraetz commented Aug 2, 2023

hoangzinh commented Aug 2, 2023 • edited Loading

sophiepintoraetz commented Aug 3, 2023

allroundexperts commented Aug 3, 2023 • edited Loading

sophiepintoraetz commented Aug 3, 2023

allroundexperts commented Aug 3, 2023

sophiepintoraetz commented Aug 3, 2023

sophiepintoraetz commented Aug 3, 2023

daraksha-dk commented Aug 3, 2023

hoangzinh commented Aug 3, 2023

sophiepintoraetz commented Aug 4, 2023

allroundexperts commented Aug 4, 2023

JmillsExpensify commented Aug 8, 2023

kavimuru commented Apr 18, 2023 •

edited by melvin-bot bot

Loading

kadiealexander commented Jul 3, 2023 •

edited

Loading

alitoshmatov commented Jul 4, 2023 •

edited

Loading

hoangzinh commented Jul 5, 2023 •

edited

Loading

melvin-bot bot commented Jul 27, 2023 •

edited by sophiepintoraetz

Loading

melvin-bot bot commented Jul 27, 2023 •

edited by sophiepintoraetz

Loading

hoangzinh commented Aug 2, 2023 •

edited

Loading

allroundexperts commented Aug 3, 2023 •

edited

Loading