Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[$1000] [HOLD for payment 2023-06-05] Android status bar colour doesn't match the background #13001

Closed Julesssss closed 1 year ago

Julesssss commented 1 year ago

Problem

While resolving this related issue I uncovered a new problem. The LHN page background colour on mobile does not match the other pages in the app. The background colour is slightly different, and this is because of the need to display LHN and chat components side by side on desktop/web.

While on the LHN page, the status bar is visible. Additionally, the LHN background is different from every other page in the app. This will be far more noticeable once we switch to the dark theme:

Dark Theme dark1 dark2 dark3 dark4

Light Theme Screenshot_20221124_171947

Screenshot_20221124_145436

Solution

As discussed here in Slack, we have a couple of options:

1) Do nothing, live with the unique background colour for mobile. I don't like this as we are breaking mobile because of an issue that only exists on web/desktop (the LHN is not distinguishable when side by side)

2) Ignore the background colour difference, but implement a hack on Android that forces the status bar to not render, therefore resolving part of the problem. I think this option is bad as it introduces technical debt that is difficult to understand, even for someone very familiar with native Android code. Making changes to the status bar in the future will be difficult as the hack is pretty much un-discoverable without finding the introducing PR

3) At the React Native layer, display a different background colour on the LHN depending on the platform. This trades an inconsistency within the mobile apps, to an inconsistency based on whether we display the sidebar. We already use platform-specific code and set attributes based on the screen size, so this feels like the ideal solution to me.

background: props.isSmallScreenSize styles.sidebar : styles.sidebarLarge

4) Programatically set the status bar colour natively. While this would certainly work, we'd have to do it on every page and it is not immediate. We would see the default white status bar appear briefly before the green colour is set.

5) Refactor the app so that StatusBar doesn't sit above the entire app. This way we could set the colour based on context from the current page. This is a large refactor though, and I don't think this is a good use of time.

6) Spend time looking for a solution that lets us style each page on every platform. This is likely to be a native Android solution, but there's a good chance we won't find a better solution than those listed above. There is no guarantee this is possible, and it could take weeks of work.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01721757a40a8d7d9c
  • Upwork Job ID: 1666788536927301632
  • Last Price Increase: 2023-06-08
melvin-bot[bot] commented 1 year ago

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

Julesssss commented 1 year ago

Awaiting the merging of dark mode before I investigate further.

Julesssss commented 1 year ago

Returning to this soon after the notifications doc.

Julesssss commented 1 year ago

Focusing on this once I'm back from OOO

Julesssss commented 1 year ago

No change here.

Julesssss commented 1 year ago

This is likely to be fixed by the navigation reboot changes.

MelvinBot commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

roryabraham commented 1 year ago

@Julesssss I was working on https://github.com/Expensify/App/issues/12261 and this issue was exacerbated because those mockups use various different colors near the top of the screen (not just light and dark green).

I was able to fix this with a pretty small PR: https://github.com/Expensify/App/pull/15471. Can you check it out and see if it's an acceptable approach? It seems to match the behavior we see on iOS.

Basically:

roryabraham commented 1 year ago

Making this a weekly because it's a blocker for https://github.com/Expensify/App/issues/12261

Julesssss commented 1 year ago

Okay, so yeah in order to have the backgrounds extend to the top of the screen we have no option but to have a transparent status bar. However, this introduces some issues we'll need to fix:

Overlapping status Overlapping navigation Excess navigation padding
Screenshot_1677236108 Screenshot_1677236042 Screenshot_1677236430
roryabraham commented 1 year ago

Was really struggling with this today in https://github.com/Expensify/App/pull/15471, I tried a lot of different things, but ultimately the only way I've been successful in making the app display edge-to-edge is with this code:

Window w = getWindow();
w.setFlags(WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS, WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS);

However, it seems like that approach breaks the automatic keyboard avoidance we get through the android:windowSoftInputMode="adjustResize" property on the main activity in AndroidManifest.xml.

With that FLAG_LAYOUT_NO_LIMITS in place, I also tried implementing this nice animated keyboard avoidance trick which doesn't rely on the windowSoftInputMode property, but I didn't have any success. That also only works on Android 11+ (which is probably fine).


Now, I keep coming back to the official documentation which says that this snippet should make the app display edge-to-edge:

WindowCompat.setDecorFitsSystemWindows(getWindow(), false);

That works for the splash screen, but as soon as the main activity kicks in it stops working. I have had a number of theories why this doesn't work in our app, but the one that seems the most likely is that it is working, but the insets are being consumed by some parent view to our main app. To support this theory, if you look at the Android Studio LayoutInspector you can see that the root view seems to encapsulate the navigation and status bars (as expected), but for some reason the app content doesn't display behind them.

Investigating this theory further, I did a few things:


Anyways, now I'm quite stumped with this issue.

roryabraham commented 1 year ago

I asked for help from @janicduplessis here

janicduplessis commented 1 year ago

I can have a look at this!

MelvinBot commented 1 year ago

📣 @janicduplessis! 📣

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:

  1. 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.
  2. 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.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
janicduplessis commented 1 year ago

Contributor details Your Expensify account email: janicduplessis@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0103a29331a221d7f6

MelvinBot commented 1 year ago

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

janicduplessis commented 1 year ago

I think I got it working for status bar here https://github.com/Expensify/App/pull/15656, do you also want to make the navigation bar transparent?

janicduplessis commented 1 year ago

I made some more progress on edge to edge display, currently the only issue seems to be with Modal, we use it for example in the bottom sheet, it doesn't seem to render under the navigation bar so it doesn't look the best. I think it would require a fix in RN.

image image image

:(

image
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-03-17. :confetti_ball:

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.

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

Julesssss commented 1 year ago

^ this was for a regression, PR is still in review

janicduplessis commented 1 year ago

https://github.com/Expensify/App/pull/15778 is ready to review and contains a fix for the regression we found.

I also made some progress on transparent navigation bar for edge to edge display.

@Julesssss @roryabraham When you can could you have a look at this comment https://github.com/Expensify/App/pull/15747#issuecomment-1468868385

MelvinBot commented 1 year ago

This issue has not been updated in over 15 days. @janicduplessis 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!

Julesssss commented 1 year ago

Bumping this @janicduplessis, there are some comments on the PR still, but it's looking pretty good.

janicduplessis commented 1 year ago

I’m currently out on vacation, will work on moving the PR forward when I’m back in around 1-2 weeks.

Julesssss commented 1 year ago

Sounds good. Enjoy!

Julesssss commented 1 year ago

Hey @janicduplessis, do you have time to return to this issue? No worries if not, just curious as I completely forgot about it.

janicduplessis commented 1 year ago

@Julesssss The PR is just waiting for review :)

Julesssss commented 1 year ago

Oh sweet, I didn't notice those comments. Will review it properly again tomorrow

janicduplessis commented 1 year ago

@Julesssss Awesome, thanks!

Julesssss commented 1 year ago

I retested the changes today on new and old Android devices and it's looking great. I think we can merge once the conflict is resolved.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-06-05. :confetti_ball:

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.

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @lschurr (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

Julesssss commented 1 year ago

Hey Lauren, the PR was already merged by @janicduplessis, who's with Margelo I believe. So we just need to pay out

lschurr commented 1 year ago

Sorry who needs to be paid for this one @Julesssss? It looks like maybe @mananjadhav and @aimane-chnaif worked on this PR? https://github.com/Expensify/App/pull/15778

Julesssss commented 1 year ago

Oh yeah, my bad:

mrousavy commented 1 year ago

For Margelo we can discuss over our Slack channel :)

lschurr commented 1 year ago

Cool @aimane-chnaif and @mananjadhav - please apply to the job: https://www.upwork.com/jobs/~01721757a40a8d7d9c

mananjadhav commented 1 year ago

@lschurr Same session issue here too. Would you please be able to send an invite here too? Thanks for helping out.

lschurr commented 1 year ago

@mananjadhav - I invited you to the job.

mananjadhav commented 1 year ago

Applied @lschurr

lschurr commented 1 year ago

This is paid. Closing.