Closed kavimuru closed 1 year ago
Triggered auto assignment to @Christinadobrzyn (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Triggered auto assignment to @ctkochan22 (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
I'm not sure if this is something we want to fix now or put on hold as it's a pretty minor flicker and might be fixed with something else so going on right now (although I can't find any GHs that might relate). Adding Engineering
label for a second opinion.
@kavimuru
Can you confirm that this is only on android?
Not to step in, but I could only reproduce this on Android/Chrome.
I've tried downloading the Android Beta URL into BrowserStack, but I can't get it to work, so I'm unsure about Android/Native.
@Christinadobrzyn could you do me a huge favor and get this re-assigned?
For next steps, I'd highly recommend seeing if this is an issue with the keyboard itself. And if there are other instances or past instances. It seems strange that it would happen here.
Triggered auto assignment to @aldo-expensify (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
I reproduced this in Android native, didn't reproduce anywhere else.
hmmm, my theory: this is happening because the keyboard slides up taking some time, meanwhile, I'm guessing we adjust the pdf modal's height instantly, so there is a brief moment where there is a space between the pdf modal and the keyboard that is opening. This space allows you to see the screen behind, the chat.
I does look a bit weird, so I'll make it External
in case a contributor finds an easy solution.
Job added to Upwork: https://www.upwork.com/jobs/~01b4d787358ad1fd5f
Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External
)
Current assignee @aldo-expensify is eligible for the External assigner, not assigning anyone new.
Modal does not fully cover the screen when keyboard opens.
The root cause of this issue is setting statusBarTranslucent
prop of the Modal
in AttachmentModal
to false
here.
We can set statusBarTranslucent
as true
in all models where it is set as false
. Doing this fixes this issue. However, it causes another issue. To fix this new issue, we can add a gap in the Modal
content. This is currently being done for mobile web
using the HeaderGap
component here. We can use a similar component on android
as well.
None
When we type in the password to open a password-protected PDF, the content behind the modal is briefly visible.
This happens due to a bug fix being done incorrectly.
The statusBarTranslucent
when false
makes the screen resize when the keyboard is opening, causing the brief flicker where we can see the content behind it.
It's set to false only for the AttachmentModal
due to this PR. The explanation for that is here. Specifically we don't want the modal to go over the status bar
in the AttachmentModal
.
That fix is not fixing the root cause of the styling issue modal will go over the status bar
. The root cause of it is that we're making the modal full screen, overlapping the status bar when statusBarTranslucent
is true (in https://github.com/Expensify/App/blob/d5f4caa0aae0a3ab3e6242a810ffc4ec620f4453/src/components/Modal/BaseModal.js#L110) but we're not adding a proper safe area padding
for our content. So the content can overlap with the status bar.
We need to do 2 things:
statusBarTranslucent={false}
in AvatarCropModal
and AttachmentModal
so that it will get the default true
value, then the flickering issue here will be fixed.shouldAddTopSafeAreaMargin
and shouldAddTopSafeAreaPadding
available on Android similar to iOS (rather than always setting it to false) by removing this file https://github.com/tienifr/App/blob/7e8e8a1d881914abe7c13768eb13e4930cb8546b/src/styles/getModalStyles/index.android.jstop inset
for Android, the top inset will be set to statusBar
height for Android, and insets.top
(from react-native-safe-area-context
) for iOS
This can be done by defining a getSafeAreaPaddingTop
util similar to here and here (If we still want to use the statusBarTranslucent
in any case in the future, then we need to pass in that params to the getSafeAreaPaddingTop
and return 0 if statusBarTranslucent
is false)
Here's the branch with the diff (if you need it to test for regression)
NA
Working well after the fix:
π @mananjadhav do you have some thoughts on the proposals?
Sorry I missed these. I will review these proposals today.
Modal height is resized earlier when keyboard opens, which makes bottom part transparent and previous screen partially shows
The root cause is well explained by @tienifr here.
So if statusBarTranslucent
set to false in Modal
, it handles keyboard avoiding automatically and therefore screen is already resized before fully opening keyboard.
We can completely remove conditional statusBarTranslucent
prop in Modal
and pass this value true
always.
After making status bar translucent, we should handle top safe area padding in android similar to iOS on these modals:
Here's final solution:
remove android specific getModalStyles file on which shouldAddTopSafeAreaMargin
and shouldAddTopSafeAreaPadding
was always set to false
. Now these margin/paddings are optionally needed in android
In BaseModal
, completely remove statusBarTranslucent
prop and update relevant codes accordingly
const statusBarHeight = StatusBar.currentHeight || 0;
globallydeviceHeight={this.props.windowHeight + statusBarHeight}
https://github.com/Expensify/App/blob/7e8e8a1d881914abe7c13768eb13e4930cb8546b/src/components/Modal/BaseModal.js#L125statusBarTranslucent
value always true
https://github.com/Expensify/App/blob/7e8e8a1d881914abe7c13768eb13e4930cb8546b/src/components/Modal/BaseModal.js#L133safeAreaPaddingTop
(this is the key to fix https://github.com/Expensify/App/issues/9280)
https://github.com/Expensify/App/blob/7e8e8a1d881914abe7c13768eb13e4930cb8546b/src/components/Modal/BaseModal.js#L145
to
safeAreaPaddingTop: safeAreaPaddingTop + statusBarHeight,
statusBarTranslucent
in modalProptypesremove statusBarTranslucent
prop in AttachmentModal
and AvatarCropModal
which are no longer needed
NOTE: my solution cannot be similar to @tienifr's solution which causes regression on various parts. EDIT: Most of efforts I made for this issue is to avoid regression on android tablet. Without my proposal, anyone should have caused this regression not caught by PR review, which would be reported later.
my solution: (same as production)
@tienifr's solution:
my solution: (same as production)
@tienifr's solution:
@situchan your approach is basically the same as mine. My proposal is supposed to illustrate the approach to solve the issue here, not intended as a whole PR ready to be merged, so yes, some tweaks here and there are needed to make sure there's no regression.
Also I don't think we should enable shouldAddTopSafeAreaMargin
and shouldAddTopSafeAreaPadding
on Android since there's known issues with the safe area library behavior on Android here where it returns differently based on device. We should use a new flag like shouldAddStatusBarPaddingTop
, shouldAddStatusBarMarginTop
and enable it on full-screen modals instead (shouldAddStatusBarPaddingTop
is for Android, shouldAddStatusBarMarginTop
for tablet), and use it to decide if we want to add the status bar padding top to the modal in https://github.com/Expensify/App/blob/daf163e04435e293f3f7290edea86c76877e18ee/src/styles/StyleUtils.js#L317.
Looks like I am already running a good amount of backlog, so I'll unassign myself here. @Christinadobrzyn @aldo-expensify
I can help review this issue since I have full context of top safe area on attachment modal on all devices while working with @mananjadhav on https://github.com/Expensify/App/issues/10352
Platform | isSmallScreenWidth | margin:20 | topSafeAreaMargin | bottomSafeAreaMargin | topSafeAreaPadding | bottomSafeAreaPadding |
---|---|---|---|---|---|---|
iPhone | Yes | β | β | β | β | β |
Android | Yes | β | β | β | β | β |
iPad | No | β | β | β | β | β |
Tablet | No | β | β | β | β | β |
Web/Desktop | No | β | β | β | β | β |
This was original values per each platform. So the solution to fix this issue will change this rule a bit, especially Android and Tablet if making status bar translucent is the final solution.
Ohh Yes! I remember. I think @aimane-chnaif would be a good C+ for this one.
Hi, I would like to add some context here. This is an issue with the react native modal component. As @aldo-expensify mentioned, the screen is resized when the keyboard shown when the statusBarTranslucent
is set to false. Setting it to true
makes the modal as a full screen layout, so it does not get affected by the resizing (it gets resized before this PR).
But someone report the modal does not resize when statusBarTranslucent
is true as an issue here. π€·ββοΈ
@mananjadhav @aimane-chnaif thanks, I'll assign @aimane-chnaif then!
π£ @aimane-chnaif You have been assigned to this job by @aldo-expensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
@aimane-chnaif assigned as the C+
@Christinadobrzyn @aldo-expensify @aimane-chnaif 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!
Christina is OoO, so I'm assigning myself to keep this moving!
Sorry, I started too early on this and thought that a proposal had been accepted! I've hired @aimane-chnaif as C+ already, my bad.
Sorry for adding confusion here, I'll do nothing until a proposal is accepted.
Issue not reproducible during KI retests. (First week)
Issue not reproducible during KI retests. (First week)
@mvtglobally still reproducible in android v1.2.82-4 (latest staging)
The root cause and the main solution can be acceptable for all proposals. So the next step is how to avoid any regressions with this solution. @situchan I tested your solution and tests well. But I will give priority to previous proposals first if they provide solution to fix regression. @allroundexperts @tienifr please explain in detail how you will handle top safe areas for both phone and tablet based on solutions stated in your proposals. In this stage, code diff is fine to me since I need to test thoroughly.
@bernhardoj thanks for sharing helpful context. I agree this is upstream issue but it seems no update for a long time on the issue reported. As long as, this is fixable on our side, I think we are good to go.
For more context:
If statusBarTranslucent
= false, screen is resized when keyboard shows. I think this should be expected behavior in general.
But if true, screen is not resized when keyboard shows. This is actually an issue in another aspect but in our case, no problem because input view is far away above keyboard.
We should use a new flag like
shouldAddStatusBarPaddingTop
,shouldAddStatusBarMarginTop
and enable it on full-screen modals instead (shouldAddStatusBarPaddingTop
is for Android,shouldAddStatusBarMarginTop
for tablet), and use it to decide if we want to add the status bar padding top to the modal in https://github.com/Expensify/App/blob/daf163e04435e293f3f7290edea86c76877e18ee/src/styles/StyleUtils.js#L317.
@aimane-chnaif I've explained my approach above here https://github.com/Expensify/App/issues/15373#issuecomment-1459716796, basically I think we shouldn't change the existing top safe areas logic, we should only add the new flag for the statusBarPaddingTop that's relevant to this issue.
Here's my branch for you to test out https://github.com/tienifr/App/pull/26
Please let me know if you need any other info.
Thanks!
@aimane-chnaif so this would be the new platform values table (notice the new StatusBarPaddingTop, StatusBarMarginTop columns)
Platform | isSmallScreenWidth | margin:20 | StatusBarPaddingTop | StatusBarMarginTop | topSafeAreaMargin | bottomSafeAreaMargin | topSafeAreaPadding | bottomSafeAreaPadding |
---|---|---|---|---|---|---|---|---|
iPhone | Yes | β | β | β | β | β | β | β |
Android | Yes | β | β | β | β | β | β | β |
iPad | No | β | β | β | β | β | β | β |
Tablet | No | β | β | β | β | β | β | β |
Web/Desktop | No | β | β | β | β | β | β | β |
(Now that @Christinadobrzyn is back from OoO, I'm going to unassign myself again here!)
Also I don't think we should enable
shouldAddTopSafeAreaMargin
andshouldAddTopSafeAreaPadding
on Android since there's known issues with the safe area library behavior on Android here where it returns differently based on device.
@tienifr I am not convinced this yet. That issue was reported 3 years ago and recent comment refers to https://github.com/th3rdwave/react-native-safe-area-context/issues/349 which is related to windowSoftInputMode which we already configured in our code.
I tested on various android devices and insets.top
is always 0.
@aimane-chnaif I think adding a static HeaderGap
would work. What do you think?
@aimane-chnaif I think adding a static
HeaderGap
would work. What do you think?
@allroundexperts can you explain detail how that will work on tablet? It would be helpful if you share link to your branch so I can review.
@Christinadobrzyn @aldo-expensify @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
@aimane-chnaif sure, I think the SafeArea
for iOS is sort of equivalent to the statusBarHeight
in Android in our use case, so if we want to, we can reuse our existing shouldAddTopSafeAreaMargin
and shouldAddTopSafeAreaPadding
field for the status bar while still remaining safe.
What I suggest is:
shouldAddTopSafeAreaMargin
and shouldAddTopSafeAreaPadding
available on Android similar to iOS (rather than always setting it to false
)top inset
for Android, the top inset will be set to statusBar
height for Android, and insets.top
(from react-native-safe-area-context
) for iOS
This can be done by defining a getSafeAreaPaddingTop
util similar to here and here @aimane-chnaif you can check the diff for this solution from this branch, it's quite clean and easy to understand.
@tienifr can you update original proposal and just add Proposal updated
in new comment?
@aimane-chnaif sure
The statusBarTranslucent when false makes the modal does not cover the screen completely when the keyboard is opening, causing the brief flicker where we can see the content behind it.
@tienifr your root cause analysis isn't correct.
I think π should be the main root cause, am I wrong?
I already explained myself root cause here
Programmatically, this code affects keyboard avoiding behavior depending on statusBarTranslucent
flag:
@aimane-chnaif yeah I meant the same in the RCA, maybe my explanation was a bit confusing, I updated it to be more clear.
Thanks.
Thanks for contributions everyone. I'd like to conclude my review:
@tienifr is the first to provide main solution and linked offending PR with good explanation in root cause section. @situchan provided solution later but root cause is more accurate and explained regression fix enough on their proposal, especially tablet device which is hard to catch.
So it's hard to decide who will be assigned finally. There are several options:
I suggest 3rd option which is good for team collaboration, but @aldo-expensify will do final decision.
I'd be happy to work with @situchan on this π.
Option 3 sounds great as well!
π£ @ienjoyjs! π£
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:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Sorry for being slow here. I read through the proposals and what @aimane-chnaif is proposing here: https://github.com/Expensify/App/issues/15373#issuecomment-1471761398
I know that the proposals have been updated and their current form is not their initial form, but what I understood is:
From the investigation, there seem to be a good amount of regressions we could cause if we get this wrong. I would prefer if @situchan and @tienifr can collaborate on this, and we get a better / more tested final solution. @tienifr agreed with collaborating, do you agree @situchan ?
@aldo-expensify thanks for asking me. I already gave π β€οΈ to suggestions.
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:
You can continue to reproduce this just by tapping outside of the password input and then tapping on the input again, making the keyboard slide out and in again.
Expected Result
last chat should not show when keyboard opens
Actual Result
last chat flashes for a very brief moment when keyboard opens
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.75-0 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:
https://user-images.githubusercontent.com/43996225/220753992-02b7f0d0-7939-46ab-84c1-53f9878eb6e9.mp4
https://user-images.githubusercontent.com/43996225/220754023-ad06ea87-35e4-41fa-8433-d962714baa78.mp4
Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676983368278249
View all open jobs on GitHub
Upwork Automation - Do Not Edit