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

Fix Header Bouncing on start (Android) #3610

Merged
merged 2 commits into from
Jun 16, 2021
Merged

Fix Header Bouncing on start (Android) #3610

merged 2 commits into from
Jun 16, 2021

Conversation

rdjuric
Copy link
Contributor

@rdjuric rdjuric commented Jun 16, 2021

Details

Making the StatusBar translucent after the BootSplash was causing abrupt changes to our drawable area, causing the jump that we saw on the issue. It's also no longer necessary since our StatusBar is solid both before and after our JS code loads.

Fixed Issues

#3546

Tests

QA Steps

  1. Force close the app
  2. Open the app
  3. The header doesn't bounce

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Simulator.Screen.Recording.-.iPhone.12.-.2021-06-16.at.10.22.13.mp4

Android

fixHeader.mp4

@rdjuric rdjuric requested a review from a team as a code owner June 16, 2021 13:25
@MelvinBot MelvinBot requested review from timszot and removed request for a team June 16, 2021 13:25
@parasharrajat
Copy link
Member

parasharrajat commented Jun 16, 2021

Oh. Sorry for the Misunderstanding here. I guess we all got it wrong. The issue was that the Report header(where you can see concierge and back) was jumping. And I believe translucency had no effect on it.

Why do I know this? I reported this issue. Moreover, I am not able to reproduce this issue anymore.

One more point, if you look closely the Status bar color is light gray which is different from the LHN(sidebar). We wanted to have the same color as the background. So imagine this, the Sidebar has a red color so the status bar should be red. The setting page has green color thus status bar should have a green color.

cc: @shawnborton Could you please confirm that what I said in the above para, is correct or not?

@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 16, 2021

Hey @parasharrajat! Thanks for jumping in.

The issue was that the Report header(where you can see concierge and back) was jumping. And I believe translucency had no effect on it.

Why do I know this? I reported this issue. Moreover, I am not able to reproduce this issue anymore.

Looks like one of the merged PR's made so that the app no longer opens in the last chat screen after being force closed. This makes the issue harder to reproduce, but the problem with setTranslucent still stands and I'm confident it was the reason for the jump we saw.

We wanted to have the same color as the background. So imagine this, the Sidebar has a red color so the status bar should be red. The setting page has green color thus status bar should have a green color.

I know, but this can be achieved both by StatusBar.setBackgroundColor('transparent') and StatusBar.setTranslucent(true).
The difference between the two is that the setTranslucent method lets the app draw under the status bar, increasing the drawable area.

Our app doesn't want to draw under the StatusBar (we have a CustomStatusBar with setBackgroundColor('transparent') in our root component).

So by keeping this
https://github.com/Expensify/Expensify.cash/blob/655f9efd75a8357ef2eca1aa847e84ae075dfb3b/src/Expensify.js#L154-L160

We're inevitably causing a jump in our UI by first giving the app the right to draw under the StatusBar and them removing that right when we call setBackgroundColor('transparent')

I think this is unwanted and is bound to cause problems.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 16, 2021

Looks like one of the merged PR's made so that the app no longer opens in the last chat screen after being force closed.

It is needed.

We're inevitably causing a jump in our UI by first giving the app the right to draw under the StatusBar and then removing that right when we call setBackgroundColor('transparent')

Not sure if this is correct. Maybe this not how this works. You can cross check this as setBackgroundColor('transparent') is called before calling // StatusBar.setTranslucent(true) in the code.

The translucent status bar should cause a jump from the bottom to the top but in the video jump is happening from top to bottom.

WhatsApp.Video.2021-06-11.at.2.54.07.PM.mp4

But on removing // StatusBar.setTranslucent(true), I Don't see red color inherited via the status bar. Again this needs to be confirmed but I believe this is how it was from the start.

image

There is one more point. I am making all these statements as Translucency is there from the start but this jump issue happened some time back. So I strongly believe that translucency is not the culprit.

@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 16, 2021

I see your point, @parasharrajat. But:

You can see in the QA video that the content is rendered behind the StatusBar, this is only possible in Android if the StatusBar is set to translucent.

We can confirm that the setTranslucent causes a jump in the UI by logging our insets in the SafeAreaInsetsContext, you'll notice that the insets.top will jump from 0 to ~26. This is what caused the header to be drawed under the status bar insets.top === 0, and then below it when insets.top === 26.

There is one more point. I am making all these statements as Translucency is there from the start but this jump issue happened some time back. So I strongly believe that translucency is not the culprit.

The translucency won't cause a jump in every circumstance, but that doesn't mean it isn't the cause. If the screen we're trying to render is rendered only after the SafeAreaInsetsContext updates our insets, then there will be no jump.

But on removing // StatusBar.setTranslucent(true), I Don't see red color inherited via the status bar. Again this needs to be confirmed but I believe this is how it was from the start.

Ah, yes! I meant to write that we can give it the the same color of the screen by setBackgroundColor(screenColor), setBackgroundColor(transparent) really won't make it take the screen color (since the app won't be able to draw background of the screen under the StatusBar)

I think the setTranslucent method really should be reserved for when we want our actual UI to be drawn under the StatusBar (or when presenting an image in full screen, for example), since it messes with our insets and moves the drawable area of the app around.

I don't see any colored screens in the app, but I think that if we want to make the StatusBar background color match our screen color, we should do it imperatively by using the setBackgroundColor method, that's what it was made for after-all 😃

@parasharrajat
Copy link
Member

We can confirm that the setTranslucent causes a jump in the UI by logging our insets in the SafeAreaInsetsContext, you'll notice that the insets.top will jump from 0 to ~26. This is what caused the header to be drawed under the status bar insets.top === 0, and then below it when insets.top === 26.

Ah. Nice point. But Shouldn't it be like when we set Translucent status bar, screen will move up as it needs to be drawn under the status bar? And in this case Insets.top should reduce instead of increasing. insets.top = insets.top - statusbar height.

@parasharrajat
Copy link
Member

I am just curious to know this. Anyways if this PR fixes the issue then I will give 👍 .

@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 16, 2021

Ah. Nice point. But Shouldn't it be like when we set Translucent status bar, screen will move up as it needs to be drawn under the status bar? And in this case Insets.top should reduce instead of increasing. insets.top = insets.top - statusbar height.

The reason this isn't the case is because our inset is calculated in relation to our drawable area. When we setTranslucent(true) we change our drawable area to start right at the top of the screen, and not after the StatusBar.

So when our insets.top is 0, our app touches the top of the screen (the beginning of our drawable area). If the SafeAreaInsetsContext tried to subtract our StatusBar.height from that inset, it would throw us above the edge of the screen (insets.top == -26), and not below the StatusBar (as it does with insets.top == 26).

I am just curious to know this. Anyways if this PR fixes the issue then I will give 👍 .

No worries, it's always good to have more eyes looking at the code 😄 Thanks!

@timszot timszot merged commit f3f3a7d into Expensify:main Jun 16, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.69-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kevinksullivan
Copy link
Contributor

Paid in Upwork!

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants