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
2.99k stars 2.5k forks source link

[HOLD for payment 2023-03-02] [$3000] mWeb/safari - Create Group button is hidden by keyboard when selecting participants #11463

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

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


Issue found when executing PR https://github.com/Expensify/App/pull/11309

Action Performed:

  1. Login with any account
  2. Tap on FAB icon
  3. Select New Group
  4. Type any email, or select any existing option
  5. Verify that the green "Create group" button is not hidden by the keyboard instead of anchored above it

Expected Result:

Create group button is not hidden

Special note: Reiterating for emphasis for the clarity of future contributors! We're only looking for proposals that will address upstream fixes to KeyboardAvoidingView. Thank you!

Actual Result:

Create Group button is hidden when tap on selected user

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.10.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/193172169-4295a539-bbb9-43a4-87c5-833117dd3e75.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @laurenreidexpensify (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 - @mananjadhav (External)

melvin-bot[bot] commented 1 year ago

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

laurenreidexpensify commented 1 year ago

Not overdue, reviewing this morning

laurenreidexpensify commented 1 year ago

https://www.upwork.com/jobs/~01948891954d6b32c7

wildan-m commented 1 year ago

In the video, the create group button is hidden when no participant is selected and then shown after one of the participants is selected.

Is that expected behavior?

Should we always show the "Create Group" button even when there is no selected participant?

mananjadhav commented 1 year ago

@flodnv @laurenreidexpensify Can you please confirm @wildan-m's comment above? Base on the code it does look like we're hiding the Button when we create the group. You can reproduce this even if you don't select the input text. Just scroll the list as soon as you click on the Create Group option.

const shouldShowFooter = (this.props.shouldShowConfirmButton || this.props.footerContent)
            && !(this.props.canSelectMultipleOptions && _.isEmpty(this.props.selectedOptions));

Second line of the condition in the above snippet.

flodnv commented 1 year ago

the create group button is hidden when no participant is selected and then shown after one of the participants is selected.

Is that expected behavior?

I don't know, but I would say yes...?

Should we always show the "Create Group" button even when there is no selected participant?

Why would we do that? Why would we show the Create Group button before you select anyone to add to the group? What would you expect to happen if you click on it? 😕

wildan-m commented 1 year ago

I think this issue is not valid then. The tester might forgot the expected behavior, or I misinterpreted the issue?

flodnv commented 1 year ago

Oh, great point actually! @kbecciv why would you expect Create group button is not hidden?

kbecciv commented 1 year ago

@flodnv I'm expecting all environments worked the same, this is only my opinion.

https://user-images.githubusercontent.com/93399543/196209810-a1d17d21-a3f0-49bc-960f-4f0cc9ddd3a3.mp4

mananjadhav commented 1 year ago

@kbecciv In this second video you've selected one account and hence it shows up the Create group button. Are you saying for mWeb Safari also you select the row and yet it didn't show the button?

kbecciv commented 1 year ago

Correct, attach the video for mWeb/Safari

https://user-images.githubusercontent.com/93399543/196234350-66e3c79f-ac36-475d-9422-072de190b1af.MP4

wildan-m commented 1 year ago

@kbecciv, (1) The button didn't show, or (2) it's covered by the keyboard? (We can find it when scrolling down)

@mananjadhav, @flodnv, if it's covered by the keyboard I think this issue is related to KeyboardAvoidingView, and most tasks related to it are on hold. Seems the internal team is working on it.

Similar issues can be found here: #10273

laurenreidexpensify commented 1 year ago

bountry increased to $1000

mananjadhav commented 1 year ago

@flodnv @kbecciv I am having a little problem in reproducing this. I did check the second video where the item was selected, but could this be that the button is hiding behind the keyboard? and is that what we plan to fix?

Can we have a better reproduction steps with the complete video? I couldn't produce this on iPhone simulator and real device.

flodnv commented 1 year ago

Can we have a better reproduction steps with the complete video?

Agreed.

I am still confused about https://github.com/Expensify/App/issues/11463#issuecomment-1281152039 -- only one account is selected, so by design, we don't show the button.

Hummmm I tested on my Android app on my phone, and it does show the button when selecting only 1 participant 😬

laurenreidexpensify commented 1 year ago

@kbecciv can you please record a longer video with more detailed reproduction steps to help us confirm if this is still reproducible? thanks

kbecciv commented 1 year ago

Checking with team, will update here shortly

kbecciv commented 1 year ago

Issue is still reproduced in build 1.2.24.0 Steps:

  1. Login with any account
  2. Tap on FAB icon
  3. Select New Group
  4. Type any email
  5. Hold your finger when cursor at the end
  6. Verify that Create group button is not hidden

https://user-images.githubusercontent.com/93399543/200343338-aaff56a5-8cfb-43f0-9785-24debe3c473b.mp4

mananjadhav commented 1 year ago

@kbecciv Pardon me for my ignorance, but I still couldn't make out if the button is hidden behind the keyboard?

kbecciv commented 1 year ago

@mananjadhav Correct, this PR https://github.com/Expensify/App/pull/11309 mention on step 6. Verify that Create group button is not hidden. Which is Fail in mweb/safari

image

mananjadhav commented 1 year ago

Okay thanks clears up the steps @kbecciv , but in your video are we scrolling till the end, that is now what I am able to see the Create Group button behind the keyboard. Here's the video.

https://user-images.githubusercontent.com/3069065/200636605-9ffd51a6-5b4e-4ce6-8235-7870510da55d.MP4

flodnv commented 1 year ago

@mananjadhav I think the suggestion is that the "Create group" button should not be hidden behind the keyboard, just like on a native Android app.

flodnv commented 1 year ago

could this be that the button is hiding behind the keyboard? and is that what we plan to fix?

Yes!

JmillsExpensify commented 1 year ago

Removing Lauren since we're consolidating keyboard issues under one BZ. cc @tgolen

JmillsExpensify commented 1 year ago

Also great discussion thus far. This is super easy to reproduce. I've updated the testing steps in the OP for clarity.

  1. Login with any account
  2. Tap on FAB icon
  3. Select New Group
  4. Type any email, or select any existing option
  5. Verify that the green "Create group" button is not hidden by the keyboard instead of anchored above it
mananjadhav commented 1 year ago

Verify that the green "Create group" button is not hidden by the keyboard instead of anchored above it

Thanks for the comments @flodnv @JmillsExpensify. This statement was critical to get aligned on the expected output.


Waiting for proposals.

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @mananjadhav, @flodnv Whoops! This issue is 2 days overdue. Let's get this updated quick!

mananjadhav commented 1 year ago

Waiting for proposals.

JmillsExpensify commented 1 year ago

On that note, it's been more than a week since we last raised the price, and I also noticed that the linked Upwork job is closed. Here's a fresh Upwork post: https://www.upwork.com/jobs/~0123e8d88f2993e390. We're open for proposals!

hammad-6304 commented 1 year ago

Proposal @JmillsExpensify Hi hope you are doing great, I saw this job posting on Upwork. I reproduce this issue on different browser eg Safari and chrome and successfully able to resolve this issue.

This issue occurs only on mobile browsers, React Web does not provide KeyboardAvoidingView component by default so we have to create it manually.

Whenever keyboard pops-in in mobile browsers it triggers the change in dimensions of View-Port.

This issue can be fixed by sticking with the view-port dimensions so whenever it changes our View will adjust its height according to the new view-port.

I fixed this issue, I attached screen recordings just for reference of this resolved issue please review it once.

Note: It's working perfectly on the rest of platforms eg: Desktop, ios and android and also fixed on web now.

Looking forward to hear from your side,

Thanks.

https://user-images.githubusercontent.com/103407647/201782781-45c0c3ad-6f6e-4027-b9d8-86eae0b62505.MP4

https://user-images.githubusercontent.com/103407647/201782843-71e3811b-b3d4-46a1-96a3-9a7161cec97a.MP4

hammad-6304 commented 1 year ago

Proposal

Root cause

Create Group Button is hidden by the on-screen keyboard on mobile browsers, Since React-Web doesn't provide KeyboardAvoidingView by default so few components are hidden by the on-screen keyboard.

Solution

Creating KeyboardAvoidingView manually for the web version to Lift-Up all the View behind the keyboard so it can be visible. On-Screen Keyboard triggers the change in 'ViewPort' we can watch on this change and can lift-up the viewPort accordingly by applying suitable offset to make the button visible even if keyboard is open or closed on web platform.

Reference videos are attached of the fixed version of this issue on both Safari & **Chrome

https://user-images.githubusercontent.com/103407647/201874996-ce8a0503-b265-431a-8676-590c9df197c4.MP4

**.

https://user-images.githubusercontent.com/103407647/201874923-07f451ae-317f-406d-a276-7dae196175ba.MP4

cc: @JmillsExpensify @flodnv @kbecciv

flodnv commented 1 year ago

@mananjadhav what are your thoughts on this proposal?

mananjadhav commented 1 year ago

I agree with @hammad-6304 that it isn't a problem just with Create Group, but I can reproduce this on other screens as well.

Thanks for the proposal @hammad-6304. The expected output looks to be correct.

But we use react-native-web, and afaik it does have KeyboardAvoidingView. So I think it would be better to fix any problem with that implementation? Does your solution fix that? or implements a new component altogether?

hammad-6304 commented 1 year ago

@mananjadhav totally agree with your point react-native-web provide KeyboardAvoidingView but, the problem is, it is not fully matured and context rich yet. Only latest versions of react-native-web ships this feature still not fully functional but in Mock State (Reference is attached below) which will not be good enough to handle these scenarios on variety of browsers and devices since different browsers have different pre-processing & JS engines.

Solution

Yes you are right @mananjadhav, The solution i proposed will handle all these scenarios and will be compatible not only with current issue (Create Group Button is hidden) but through out the application.

It will be a new component handling all the scenarios for every possible web-based devices which will be totally dynamic and easy to consume anywhere in the codebase where needed. It'll be having its own default behaviours but will be totally customizable Via Props.

cc: @JmillsExpensify @flodnv

Screenshot 2022-11-15 at 10 19 42 PM
mananjadhav commented 1 year ago

Thanks for the comment @hammad-6304. I am aware it's a mock state, but I was wondering if we should fix this upstream? or write a component ourselves. @flodnv do you have any opinion on this one?

Could you elaborate on your proposal? to share some technical details? code footprint we're looking to add? Also I hope you've gone through our contributing guidelines to ensure it fits into our style and coding guides.

hammad-6304 commented 1 year ago

Yes, We can fix this upstream but there're a lot of chances that it'll be bottle neck for us, eg: proper versioning, compatibility, bridging and linking through different versions or Base-Platform. Since React-Native changed its architecture in the new version so we if we Fix this upStream we'll have to follow the new one so there'll be conflicts or either we need to upgrade our app with new one. So, by keeping in mind all the possible scenarios i think it'll be way better to go with a custom new component we'll be having strong grip and more control on it.

And yes Opinions matters most let's wait for the opinion from @flodnv.

Technical Details

it'll be a custom KeyboardAvoidingView component.

Yes i have gone through the contributing guidelines and also through the code base and am sure this solution is properly aligned with the guidelines, coding guides and performance meters.

mananjadhav commented 1 year ago

Thanks for the details.

We have listeners for view port like withWindowDimensions. We can use those. We can work out the kinks in the PR. While we do maintain our own forks for react-native and so on, I am fine with us implementing a component here.

@flodnv What are your thoughts. I am more or less convinced with @hammad-6304's approach here.

🎀 👀 🎀  C+ reviewed

hammad-6304 commented 1 year ago

Thanks for the responses.

Looking forward.

Julesssss commented 1 year ago

I'm going to hold this one temporarily, as we already have spent some time working on a custom KeyboardAvoidingView component internally. Thanks for your patience here.

CC @tgolen for when you return from OOO

Internal C+ discussion here

hammad-6304 commented 1 year ago

@Julesssss Thanks for the update.

Can you please confirm me, is there any further actions for me on this proposal and solution? What should i have to do next? Can i have an invitation to be the part of community eg: Slack internal groups and stackoverflow community?

Thanks, eagerly waiting for the humble response from your side.

CC @mananjadhav @flodnv @JmillsExpensify

flodnv commented 1 year ago

@hammad-6304 we're discussing this internally, we'll let you know of any further development in this GH issue.

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

tgolen commented 1 year ago

I've read through this issue and taken some of the proposals into account, but I do not think we should fix this in our codebase. We should absolutely be fixing this in the upstream repos and it will not only make the react-native-web lib better for us, but for everyone else using it. This bug isn't so bad that we need any kind of temporary hack or work around in our codebase.

flodnv commented 1 year ago

Agreed, we will only accept proposals that fix this upstream. Taking this off hold. Thanks!

JmillsExpensify commented 1 year ago

Reiterating for emphasis for the clarity of future contributors! We're only looking for proposals that will address upstream fixes to KeyboardAvoidingView. Thank you!

mananjadhav commented 1 year ago

Reiterating for emphasis for the clarity of future contributors! We're only looking for proposals that will address upstream fixes to KeyboardAvoidingView. Thank you!

@JmillsExpensify Can we also updated the requirement in the GH body? Just in case someone misses in the comments.

JmillsExpensify commented 1 year ago

Sure thing. Done!

flodnv commented 1 year ago

Maybe this should HOLD on https://github.com/Expensify/App/issues/10670