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

[C+ Checklist Needs Completion] [$500] Android - Task - Background image in task report appears cropped after cancelling the task #33922

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 4, 2024 · 57 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 4, 2024

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


Version Number: 1.4.21-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Go to 1:1 DM > + > Assign task
  2. Create a task
  3. Open the task report
  4. Tap 3-dot menu > Cancel
  5. Cancel the task

Expected Result:

The background image in task report will not appear cropped after cancelling the task

Actual Result:

The background image in task report appears cropped after cancelling the task

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6331783_1704328677391.Screen_Recording_20240103_170106_New_Expensify__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f32de82628d0256b
  • Upwork Job ID: 1742715497820069888
  • Last Price Increase: 2024-01-25
  • Automatic offers:
    • aimane-chnaif | Contributor | 28124230
    • aswin-s | Contributor | 28143615
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 4, 2024
@melvin-bot melvin-bot bot changed the title Android - Task - Background image in task report appears cropped after cancelling the task [$500] Android - Task - Background image in task report appears cropped after cancelling the task Jan 4, 2024
Copy link

melvin-bot bot commented Jan 4, 2024

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

Copy link

melvin-bot bot commented Jan 4, 2024

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

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

melvin-bot bot commented Jan 4, 2024

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

Copy link

melvin-bot bot commented Jan 4, 2024

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

@aswin-s
Copy link
Contributor

aswin-s commented Jan 4, 2024

Withdrawing my first proposal as it seems to break on web.

@aswin-s
Copy link
Contributor

aswin-s commented Jan 4, 2024

Proposal

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

The EmptyStateBackground image on deleted task page looks clipped

What is the root cause of that problem?

Animated background image used to be an SVG image. Recently that was replaced with a PNG. Earlier we used to set the container width to 200% to make the SVG image appear covering the screen on small screens.

width: '200%',

Now this is not required as PNG images properly apply the resizeMode within the container.

The PNG image height is set to 300px for small screen devices and it is positioned 'absolutely' with in it's parent container. Parent container of the AnimatedBackground image has a much smaller height. Being the first item in the inverted Flatlist the absolutely positioned image goes out of the contentContainer bounds. On Android anything outside the content container of Flatlist gets clipped even if we provide overflow:'visible'. That's why the image gets clipped only on Android.

Below image shows contentContainer with red border applied around it on iOS and Android.

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

Set BackgroundImage container width to 100%

if (isSmallScreenWidth) {
        return {
            height: emptyStateBackground.SMALL_SCREEN.IMAGE_HEIGHT,
            width: '100%',
            position: 'absolute',
        };
}

Now the image translation logic needs to be adjusted as width is now set to 100%

const IMAGE_OFFSET_X = windowWidth / 2;

to

const IMAGE_OFFSET_X = 0;

Also replace the fragment wrapper here with a View

<>
<AnimatedEmptyStateBackground />
<View style={[StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}>
<ReportActionItemSingle
action={parentReportAction}
showHeader={_.isUndefined(props.draftMessage)}
report={props.report}
>
<RenderHTML html={`<comment>${props.translate('parentReportAction.deletedTask')}</comment>`} />
</ReportActionItemSingle>
<View style={styles.reportHorizontalRule} />
</View>
</>

<View >
  <AnimatedEmptyStateBackground />
  <View style={[StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}>
    <ReportActionItemSingle
      action={parentReportAction}
      showHeader={_.isUndefined(props.draftMessage)}
      report={props.report}
    >
        <RenderHTML html={`<comment>${props.translate('parentReportAction.deletedTask')}</comment>`} />
    </ReportActionItemSingle>
    <View style={styles.reportHorizontalRule} />
  </View>
</View

Result

Before After
Screenshot_1704336463 Screenshot_1704336747

What alternative solutions did you explore? (Optional)

None (edited)

@bernhardoj
Copy link
Contributor

Proposal

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

The report background image is cut off on a canceled task report.

What is the root cause of that problem?

In short, the view container height is smaller than the background height.

The image height is defined by IMAGE_HEIGHT.

App/src/CONST.ts

Lines 134 to 143 in d800d60

SMALL_SCREEN: {
IMAGE_HEIGHT: 300,
CONTAINER_MINHEIGHT: 200,
VIEW_HEIGHT: 185,
},
WIDE_SCREEN: {
IMAGE_HEIGHT: 450,
CONTAINER_MINHEIGHT: 500,
VIEW_HEIGHT: 275,
},

On a small screen, the height is 300. When the task is canceled, we render a simple [Deleted task] message.

if (ReportUtils.isCanceledTaskReport(props.report, parentReportAction)) {
return (
<>
<AnimatedEmptyStateBackground />
<View style={[StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}>
<ReportActionItemSingle
action={parentReportAction}
showHeader={_.isUndefined(props.draftMessage)}
report={props.report}
>
<RenderHTML html={`<comment>${props.translate('parentReportAction.deletedTask')}</comment>`} />
</ReportActionItemSingle>
<View style={styles.reportHorizontalRule} />
</View>
</>
);

The height is around 70 and we have a margin-top style from getReportWelcomeTopMarginStyle. It's defined by VIEW_HEIGHT above and the value is 185. So, the total height is around 255 and it's smaller than 300.

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

  1. Update the min-height (defined by CONTAINER_MINHEIGHT) at least to 300.
  2. Then, we will update the task view in ReportActionItem by wrapping it with a View following the same pattern in ReportActionItemParentAction
    <View style={StyleUtils.getReportWelcomeContainerStyle(props.isSmallScreenWidth)}>
    <AnimatedEmptyStateBackground />
    <View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]} />
    {parentReportAction && (
    <ReportActionItem
<View style={StyleUtils.getReportWelcomeContainerStyle(props.isSmallScreenWidth)}>
    <AnimatedEmptyStateBackground />
    <View style={[StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}/>
    <View>
        <ReportActionItemSingle
            action={parentReportAction}
            showHeader={_.isUndefined(props.draftMessage)}
            report={props.report}
        >
            <RenderHTML html={`<comment>${props.translate('parentReportAction.deletedTask')}</comment>`} />
        </ReportActionItemSingle>
        <View style={styles.reportHorizontalRule} />
    </View>
</View>

(do the same for non-canceled task)

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 4, 2024

Proposal

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

For android, an empty state image in the task report clipped and doesn't looks correct

What is the root cause of that problem?

Currently, Parent of the absolute image have flex-direction: column-reverse; and due to that absolute position starting points will be incorrect. Due to that it overflows in the empty area which will be the unalloted area and in android it clipped to the boundary which creates this issue.

Problem arises after this PR where the parent View gets removed which we had earlier here.
https://github.com/Expensify/App/pull/30815/files#diff-e6bee90bf18ae0cad1bb445ed9a9c0d989476874f1005810c0f8e93f1f39b99cL635

Issue
Screen.Recording.2024-01-04.at.23.51.59.mp4

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

We should re-add parent by replacing fragment with View below places which corrects the starting position of an absolute image.

<>
<AnimatedEmptyStateBackground />

<>
<AnimatedEmptyStateBackground />

@melvin-bot melvin-bot bot added the Overdue label Jan 8, 2024
@greg-schroeder
Copy link
Contributor

Awaiting proposal review @alitoshmatov

@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 2024
@alitoshmatov
Copy link
Contributor

Missed this one, working now

@alitoshmatov
Copy link
Contributor

@Pujan92 Why was view replaced by fragment element on that PR?

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 10, 2024

Based on this comment the styles applied to that View is adjusted to other place, seems that is the reason they have removed the View. But that View is useful to set its absolute emptystate bg child position correctly

Copy link

melvin-bot bot commented Jan 11, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jan 12, 2024
@greg-schroeder
Copy link
Contributor

Thoughts on the above @alitoshmatov?

@melvin-bot melvin-bot bot removed the Overdue label Jan 18, 2024
@Expensify Expensify deleted a comment from melvin-bot bot Jan 18, 2024
@Expensify Expensify deleted a comment from melvin-bot bot Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

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

Copy link

melvin-bot bot commented Jan 18, 2024

@greg-schroeder @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Mar 19, 2024

@aimane-chnaif Looks like this got fixed for Android on main (PR #35232). However it introduced the regression here. Also on web it still shows cropped image on some resolutions. Should I still proceed? @situchan FYI Since you reviewed the PR #35232

@aimane-chnaif As mentioned above the original issue is fixed. However the image still gets cropped on web when window is resized

Ah right. I also reproduced on android while moving phone vertically

@deetergp deetergp removed the Reviewing Has a PR in review label Mar 27, 2024
@deetergp
Copy link
Contributor

So it sounds like we've still got some issues to fix here?

@aimane-chnaif
Copy link
Contributor

So it sounds like we've still got some issues to fix here?

No, it's past comment. We're waiting for PR to hit staging.

@deetergp
Copy link
Contributor

No, it's past comment. We're waiting for #36261 to hit staging.

Perfect! Thanks for setting me straight @aimane-chnaif 👍

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Mar 29, 2024
@melvin-bot melvin-bot bot changed the title [$500] Android - Task - Background image in task report appears cropped after cancelling the task [HOLD for payment 2024-04-05] [$500] Android - Task - Background image in task report appears cropped after cancelling the task Mar 29, 2024
Copy link

melvin-bot bot commented Mar 29, 2024

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

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

Copy link

melvin-bot bot commented Mar 29, 2024

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:

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

@greg-schroeder
Copy link
Contributor

C and C+ paid. Can you complete the checklist @aimane-chnaif? Thanks!

@greg-schroeder greg-schroeder changed the title [HOLD for payment 2024-04-05] [$500] Android - Task - Background image in task report appears cropped after cancelling the task [C+ Checklist Needs Completion] [$500] Android - Task - Background image in task report appears cropped after cancelling the task Apr 5, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
@deetergp
Copy link
Contributor

Bump @aimane-chnaif

@melvin-bot melvin-bot bot removed the Overdue label Apr 15, 2024
@greg-schroeder
Copy link
Contributor

Friendly bump to get this checklist done @aimane-chnaif!

@melvin-bot melvin-bot bot added the Overdue label Apr 26, 2024
@greg-schroeder
Copy link
Contributor

@aimane-chnaif any chance we can knock this out?

@melvin-bot melvin-bot bot removed the Overdue label Apr 26, 2024
@melvin-bot melvin-bot bot added the Overdue label May 6, 2024
@greg-schroeder
Copy link
Contributor

@aimane-chnaif please jump on this one so we can close it, thank you!

@melvin-bot melvin-bot bot removed the Overdue label May 7, 2024
@greg-schroeder
Copy link
Contributor

Oh, Aimane is OOO. @aswin-s any chance you could step in for the checklist?

@greg-schroeder
Copy link
Contributor

Bump @aimane-chnaif or @aswin-s

@aimane-chnaif
Copy link
Contributor

Sorry I am back.

@aimane-chnaif
Copy link
Contributor

This bug happened after we add background image in cancelled task report. No specific PR which caused regression.

Regression Test Proposal

  1. Navigate to one of the reports below:
  • regular chat
  • workspace
  • money request
  • transaction detail
  • thread
  • task
  • cancelled task
  1. Verify that the image in report background is not clipped at the top

@greg-schroeder
Copy link
Contributor

No sweat! Thanks @aimane-chnaif

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. External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants