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

[$250] android-distance-Distance receipt is overlapping with navigation bar. #56654

Open
2 of 8 tasks
mitarachim opened this issue Feb 11, 2025 · 28 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@mitarachim
Copy link

mitarachim commented Feb 11, 2025

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: V9.0.96-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
If this was caught during regression testing, add the test name, ID and link from TestRail: N
Email or phone of affected tester (no customers): N
Issue reported by: Applause Internal Team
Device used: Redmi note 10s android 13
App Component: Money Requests

Action Performed:

  1. Launch app
  2. Open workspace chat
  3. Create a distance expense
  4. Open the expense created
  5. Open the receipt
  6. Note receipt is not overlapping with navigation bar.
  7. Tap distance and add a stop & save
  8. Tap distance receipt

Note: if bug cannot be reproduced, please add one more stop and check.

Expected Result:

Distance receipt must not overlap with navigation bar.

Actual Result:

Distance receipt is overlapping with navigation bar.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6739324_1739265126693.Screenrecorder-2025-02-11-14-41-07-323.mp4

Image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021892312402645549520
  • Upwork Job ID: 1892312402645549520
  • Last Price Increase: 2025-03-05
Issue OwnerCurrent Issue Owner: @rayane-d
@mitarachim mitarachim added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Feb 11, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-11 10:22:15 UTC.

Proposal

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

android-distance-Distance receipt is overlapping with navigation bar.

What is the root cause of that problem?

The height of the Distance E-Receipt exceeds the height of the view if we have three waypoints in this OP and we don't have a safe area padding bottom as well

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

We can add safeAreaPaddingBottomStyle to the DistanceEReceipt component similar to how we handled it beforehttps://github.com//pull/54776.

<View style={[styles.flex1, styles.alignItemsCenter]}>

    const { safeAreaPaddingBottomStyle } = useStyledSafeAreaInsets();
....
        <View style={[styles.flex1, styles.alignItemsCenter, safeAreaPaddingBottomStyle]}>

Note: Updating from createModalStyleUtils will cause a regression.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A UI bug

What alternative solutions did you explore? (Optional)

N/A
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@linhvovan29546
Copy link
Contributor

Note: This bug also occurs on iOS

@thelullabyy
Copy link
Contributor

thelullabyy commented Feb 11, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-24 04:48:45 UTC.

Proposal

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

  • The distance receipt is overlapping with the navigation bar.

What is the root cause of that problem?

  • The distance receipt is displayed in a modal with the type centered_unswipeable and the shouldUseModalPaddingStyle set to true. This modal is defined here:

    <Modal
    type={modalType}

  • In theory, the safe are padding should be applied because we already set shouldUseModalPaddingStyle to true, so this bug does not appear but when calculating the padding-bottom value for the modal:

    1. The shouldAddTopSafeAreaPadding value in the following line is 0 because, for the centered_unswipeable modal type, this value is false:

      shouldAddTopSafeAreaPadding,

      This is determined here:

      shouldAddBottomSafeAreaPadding = false;

    2. Similarly, the shouldAddBottomSafeAreaPadding value in the following line is also 0:

      shouldAddBottomSafeAreaPadding: (!avoidKeyboard || !keyboardStateContextValue?.isKeyboardShown) && shouldAddBottomSafeAreaPadding,

    3. As a result, the padding-bottom value applied to the modal content is 0 and if the content is too long, it will overlap the navigation bar as mentioned in the bug:

      style={[styles.defaultModalContainer, modalPaddingStyles, modalContainerStyle, !isVisible && styles.pointerEventsNone]}

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

  • We can update the value in the following line from false to true:

    shouldAddBottomSafeAreaPadding = false;

    This will ensure that the modal receives the appropriate padding and prevents the overlap issue in all cases, not only the distance receipt.

  • We need create the flag so that the above change is only applied when with the modal in:

  • Since the app already has already applied the safeAreaPaddingBottomStyle style to:

<View style={safeAreaPaddingBottomStyle}>{isHighResolution && <HighResolutionInfo isUploaded={isUploaded} />}</View>

we need to remove that style to prevent duplicate safe area padding bottom are applied.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  • Since this is a UI issue, automated tests should focus on verifying that the modal content does not overlap with the navigation bar when the modal type is centered_unswipeable and shouldUseModalPaddingStyle is true.

What alternative solutions did you explore? (Optional)

  • We can update this style:

<GestureHandlerRootView style={styles.flex1}>

                <GestureHandlerRootView style={[styles.flex1, safeAreaPaddingBottomStyle]}>

so the safe area padding bottom is applied to all types of attachments.

  • Since the app already has already applied the safeAreaPaddingBottomStyle style to:

<View style={safeAreaPaddingBottomStyle}>{isHighResolution && <HighResolutionInfo isUploaded={isUploaded} />}</View>

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Feb 12, 2025

Working with an Android user to get this tested. Will update soon!

@melvin-bot melvin-bot bot added the Overdue label Feb 17, 2025
Copy link

melvin-bot bot commented Feb 18, 2025

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

@CortneyOfstad CortneyOfstad added the External Added to denote the issue can be worked on by a contributor label Feb 19, 2025
Copy link

melvin-bot bot commented Feb 19, 2025

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

@melvin-bot melvin-bot bot changed the title android-distance-Distance receipt is overlapping with navigation bar. [$250] android-distance-Distance receipt is overlapping with navigation bar. Feb 19, 2025
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 19, 2025
Copy link

melvin-bot bot commented Feb 19, 2025

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

@CortneyOfstad
Copy link
Contributor

Hey @rayane-d! We have a few proposals above for you to review when you have a chance. Thanks!

@thelullabyy
Copy link
Contributor

@linhvovan29546 Please follow the contributor guideline:

If you want to make an entirely new proposal or update an existing proposal, please go back and edit your original proposal, then post a new comment to the issue in this format to alert everyone that it has been updated:

I noticed that in your latest edit, you mentioned: "Note: Updating from createModalStyleUtils will cause a regression." - This is an important point that could impact the final decision. However, I noticed that you did not post an "Updated proposal" comment to formally alert reviewers of the changes. cc @rayane-d

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Feb 20, 2025

My bad, I think the C+ hasn't been reviewed yet, so I haven't posted the comment update.
Updated proposal

@thelullabyy
Copy link
Contributor

Note: Updating from createModalStyleUtils will cause a regression.

Thanks @linhvovan29546. Could you provide C+ and me with more details about this statement?

@rayane-d
Copy link
Contributor

I'm able to reproduce 👍

Screenshots

Image

Screen.Recording.2025-02-24.at.12.30.18.AM.mov

Image

@rayane-d
Copy link
Contributor

Note: Updating from createModalStyleUtils will cause a regression.

Thanks @linhvovan29546. Could you provide C+ and me with more details about this statement?

@linhvovan29546 it would be great if you could share more information. Thanks!

@rayane-d
Copy link
Contributor

@thelullabyy Can you find examples of other places where this bug is also reproducible?

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Feb 24, 2025

@linhvovan29546 it would be great if you could share more information. Thanks!

@rayane-d The reason I mentioned this is that since we use modals in many places, applying changes to the root could cause regressions elsewhere (If we believe this fix covers all places, then that's fine.). IMO, we should avoid adding padding to the root view of modal and instead apply it to the scrollable content. Below is an example of one instance I encountered

Image

@thelullabyy
Copy link
Contributor

Can you find examples of other places where this bug is also reproducible?

  • The bug is reproducible with PDF and video attachments:

Image
Image

@thelullabyy
Copy link
Contributor

Proposal updated

@linhvovan29546
Copy link
Contributor

I think we should hold off on this until #56219 is merged, as it will change AttachmentModal from a modal to a screen.

@rayane-d
Copy link
Contributor

I think we should hold off on this until #56219 is merged, as it will change AttachmentModal from a modal to a screen.

cc @parasharrajat for visibility

Copy link

melvin-bot bot commented Feb 25, 2025

@CortneyOfstad @rayane-d 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!

@parasharrajat
Copy link
Member

Let me check and let you update.

@CortneyOfstad
Copy link
Contributor

Thanks @parasharrajat!

Copy link

melvin-bot bot commented Feb 26, 2025

📣 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 Feb 28, 2025

@rayane-d Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Feb 28, 2025
Copy link

melvin-bot bot commented Mar 4, 2025

@rayane-d 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@rayane-d
Copy link
Contributor

rayane-d commented Mar 4, 2025

holding on #56219

@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2025
Copy link

melvin-bot bot commented Mar 5, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Status: LOW
Development

No branches or pull requests

6 participants