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

[$1000] Details - In data of birth year selection keyboard area shows blank screen #24397

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 10, 2023 · 39 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering 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

@lanitochka17
Copy link

lanitochka17 commented Aug 10, 2023

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:

  1. go staging
  2. open personal details and data of birth
  3. click year selection
  4. on int keyboard is open on that time scroll the area means blank screen on keyboard area

Expected Result:

When keyboard closed year should be occupy the keyboard area

Actual Result:

Showing blank screen

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.52.5

Reproducible in staging?: Yes

Reproducible in production?: Yes

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: Any additional supporting documentation

WhatsApp.Video.2023-08-03.at.4.04.27.PM.mp4
0-02-01-d14fd87386681adc6f8485dc84e58b221a972eb77a184abd6743cb67b34387dd_16b1c9497ebfc8df.mp4

Expensify/Expensify Issue URL:

Issue reported by: @pradeepmdk

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691059074463359

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01add3f6e75afd90f5
  • Upwork Job ID: 1691500803327131648
  • Last Price Increase: 2023-08-29
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

Triggered auto assignment to @sophiepintoraetz (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2023
@sophiepintoraetz
Copy link
Contributor

Damnit - going to wait until I have access to an android tomorrow - Jules has one!

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2023
@sophiepintoraetz
Copy link
Contributor

screen-20230815-110458.2.mp4

@sophiepintoraetz sophiepintoraetz added the External Added to denote the issue can be worked on by a contributor label Aug 15, 2023
@melvin-bot melvin-bot bot changed the title Details - In data of birth year selection keyboard area shows blank screen [$1000] Details - In data of birth year selection keyboard area shows blank screen Aug 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 15, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 15, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 15, 2023

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

@JKobrynski
Copy link
Contributor

JKobrynski commented Aug 16, 2023

@lanitochka17 @sophiepintoraetz I managed to reproduce this issue and I also found these:

Potential bug 1
Potential bug 2
Potential bug 3
Potential bug 4

Aren't they similar? If so, it might be an application-wide problem, not just birth year selection

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Aug 16, 2023

Looks like problem with react-native-web maybe.
And if that's the case, it will not help with upgrade version. Already checked.

@sarious
Copy link
Contributor

sarious commented Aug 16, 2023

Looks like problem with react-native-web maybe. And if that's the case, it will not help with upgrade version. Already checked.

Agree with @shubham1206agra. It doesn't look like the bug in Expensify application.

@shubham1206agra
Copy link
Contributor

Looks like problem with react-native-web maybe. And if that's the case, it will not help with upgrade version. Already checked.

@sophiepintoraetz @abdulrahuman5196 I maybe have found the problem.

Android behaviour after the fix:

Screen.Recording.2023-08-17.at.2.49.46.PM.mov

iOS safari behaviour after the fix:

Screen.Recording.2023-08-17.at.2.51.57.PM.mov

@shubham1206agra
Copy link
Contributor

@abdulrahuman5196 @sophiepintoraetz Is this ideal behaviour?
If so, should I start PR upstream and put a patch here meanwhile (I will put a detailed proposal after soft approval from your side)?

@sophiepintoraetz
Copy link
Contributor

I think this looks good but I'll also get a 👍 from @abdulrahuman5196 too!

@sarious
Copy link
Contributor

sarious commented Aug 18, 2023

Looks like problem with react-native-web maybe. And if that's the case, it will not help with upgrade version. Already checked.It looks better. But It seems like an incorrect behaviour after the fix. As I understand, correct behaviour is when you start scrolling the list, the Virtual Keyboard should disappear. So you scrolling without keyboard. But in your fix we see Virtual Keyboard, and it close when you end scrolling.

@sophiepintoraetz @abdulrahuman5196 I maybe have found the problem.

Android behaviour after the fix:

Screen.Recording.2023-08-17.at.2.49.46.PM.mov
iOS safari behaviour after the fix:

Screen.Recording.2023-08-17.at.2.51.57.PM.mov

It looks better. But It seems like an incorrect behaviour after the fix. As I understand, correct behaviour is when you start scrolling the list, the Virtual Keyboard should disappear. So you scrolling without keyboard (and there is more space for list items). But in your fix we see Virtual Keyboard, and it close when you end scrolling.

@shubham1206agra
Copy link
Contributor

Looks like problem with react-native-web maybe. And if that's the case, it will not help with upgrade version. Already checked.It looks better. But It seems like an incorrect behaviour after the fix. As I understand, correct behaviour is when you start scrolling the list, the Virtual Keyboard should disappear. So you scrolling without keyboard. But in your fix we see Virtual Keyboard, and it close when you end scrolling.

@sophiepintoraetz @abdulrahuman5196 I maybe have found the problem.

Android behaviour after the fix:

Screen.Recording.2023-08-17.at.2.49.46.PM.mov
iOS safari behaviour after the fix:

Screen.Recording.2023-08-17.at.2.51.57.PM.mov

It looks better. But It seems like an incorrect behaviour after the fix. As I understand, correct behaviour is when you start scrolling the list, the Virtual Keyboard should disappear. So you scrolling without keyboard (and there is more space for list items). But in your fix we see Virtual Keyboard, and it close when you end scrolling.

Yes I know that
But react-native-web doesn't support onScrollBeginDrag for now
That's why I am trying to close at the end instead.

I am looking into this. Will give an update once I get something which corrects the behaviour completely.

@melvin-bot melvin-bot bot added the Overdue label Aug 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 21, 2023

@sophiepintoraetz, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@sophiepintoraetz
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Aug 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

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

@abdulrahuman5196
Copy link
Contributor

Reviewing now

@abdulrahuman5196
Copy link
Contributor

@sophiepintoraetz For now there is no proposal to review.

@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2023

@sophiepintoraetz @abdulrahuman5196 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!

@shubham1206agra
Copy link
Contributor

@abdulrahuman5196 My proposal requires patching react-native-web. (I will post the details later).
But now the upgradation of react-native-web is happening. I say we will hold this issue till then.
PR Link: #24482
Cause changes will happen after this, and I need some more time to reach a better proposal (I will post about this after some time)

@abdulrahuman5196
Copy link
Contributor

My proposal requires patching react-native-web. (I will post the details later).
But now the upgradation of react-native-web is happening. I say we will hold this issue till then.

We would need to know what is the fix actually? Without really knowing what is the fix suggested we might not be able to check this.
We also need to think of what if the suggested doesn't work. So I think we can only hold if we have some information on the solution.

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2023
@abdulrahuman5196
Copy link
Contributor

@sophiepintoraetz Currently we don't have solid proposal.
@shubham1206agra mentioned to provide some updates but there is no proposal yet.

But if we want to increase the bounty we can.

@shubham1206agra
Copy link
Contributor

My proposal requires patching react-native-web. (I will post the details later).
But now the upgradation of react-native-web is happening. I say we will hold this issue till then.

We would need to know what is the fix actually? Without really knowing what is the fix suggested we might not be able to check this. We also need to think of what if the suggested doesn't work. So I think we can only hold if we have some information on the solution.

The fix requires some modification to the ScrollResponder System of the ScrollView component. Mostly to enable the option for the keyboardShouldPersistTaps prop and then set the prop to never for the web. (This approach fixes most things except one, i.e., long quick scroll will show the same behaviour cause the list is still scrolling and we have finished touch).

For the RNWeb, the complete implementation of ScrollView will not come before 0.21.
See necolas/react-native-web#1592

So we have 2 options either keep open the Keyboard when scrolling in this case or do this partial fix.
The complete fix requires implementation of onScroll(Begin|End)Drag which will not implemented before 0.21 in RNWeb.

@melvin-bot
Copy link

melvin-bot bot commented Aug 29, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Aug 30, 2023
@sophiepintoraetz
Copy link
Contributor

I can drop this into the agency rooms as well to see if we can get some more proposals in, @abdulrahuman5196 (and if @shubham1206agra clarifications don't work)

@melvin-bot melvin-bot bot removed the Overdue label Aug 30, 2023
@sophiepintoraetz
Copy link
Contributor

Dropped it into callstack

@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

@sophiepintoraetz @abdulrahuman5196 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!

@sophiepintoraetz
Copy link
Contributor

@abdulrahuman5196 I'm checking in with the contributors on the above linked issue, as Callstack suggested it may have caused this regression. I've asked them to confirm but maybe you can take a look as well?

@s77rt
Copy link
Contributor

s77rt commented Sep 1, 2023

I have checked, seems unrelated to that PR (judging from versions timeline). But I can't reproduce the bug, is it still reproducible?

@shubham1206agra
Copy link
Contributor

I am unable to repro this.

But it looks like the reason is some kind of race condition.
So can you please hold this issue for RNWeb upgrade PR, and finally confirm the issue repro after the merge?

@sophiepintoraetz
Copy link
Contributor

@shubham1206agra, can you link the upgrade PR please? @abdulrahuman5196 - do you agree with holding?

@shubham1206agra
Copy link
Contributor

#24482

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@abdulrahuman5196
Copy link
Contributor

@sophiepintoraetz

IMO, I don't think we should hold if the issue is not reproducible. There is no concrete reason to think it would come again.

We can close this issue. If it becomes reproducible in future, the contributor can comment here and we can re-open this issue if required.

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@sophiepintoraetz
Copy link
Contributor

Agreed. I think we have enough details now to keep an eye on this but will close this out for now. Thanks all for the input!

@pradeepmdk
Copy link
Contributor

@sophiepintoraetz since this issue is still there #26732 can we open this?

@shubham1206agra
Copy link
Contributor

Let's just wait for RNWeb PR to merge

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 Engineering 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
None yet
Development

No branches or pull requests

8 participants