[HOLD for payment 2023-08-21] [$250] mWeb - Unresponsive view on 2 factors authentication screen

[kbecciv]

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 to profile settings
2. Then click on security
3. Click on two factors authentication
4. Click on copy and and then go to next
5. Click on Textfield
6. Observe the screen

Expected Result:

Ui should be responsive. And show properly even keyboard is focused.

Actual Result:

Screen is unresponsive. Some view disappears while focused.

Workaround:

Unknown

Platforms:

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

• [x] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [x] iOS / Safari
• [ ] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

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

https://github.com/Expensify/App/assets/93399543/a1d0877a-62f3-41d3-ad09-ad3147b853cd

https://github.com/Expensify/App/assets/93399543/e9361ff2-44d4-4229-8e6b-9edeb069e574

Expensify/Expensify Issue URL: Issue reported by: @niravkakadiya25 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690115036631379

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01deea2e8f64727cde
• Upwork Job ID: 1683466421789040640
• Last Price Increase: 2023-08-08
• Automatic offers:
• s77rt | Reviewer | 26018994
• jscardona12 | Contributor | 26018995
• niravkakadiya25 | Reporter | 26018997

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[trjExpensify]

Yeah, I can repro this one. The next confirmation button should be bottom-docked on this page, like the others. 👍

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[samh-nl]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When opening the keyboard on the 2FA page, the Next button is not placed at the bottom.

What is the root cause of that problem?

In iOS the viewport is not correctly updated when opening the keyboard, leading to the Next button not being positioned at the bottom of the page. This is a known issue in Safari:

https://github.com/vector-im/hydrogen-web/issues/181#issuecomment-797012793

What changes do you think we should make in order to solve the problem?

The VisualViewport API can be used to calculate the correct viewport size.

What alternative solutions did you explore? (Optional)

N/A

[ShogunFire]

It was happening when we had max height on keyboardAvoidingView but was supposed to be fixed here: https://github.com/Expensify/App/pull/16443/files

[AbeDev404]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Responsive view when the keyboard appears.

What is the root cause of that problem?

Cover the full page with KeyAvoidingView or SafeAreaView.

What changes do you think we should make in order to solve the problem?

Cover the full page with KeyAvoidingView or SafeAreaView. And re-style the Next Button.

[s77rt]

@samh-nl Thanks for the proposal. I don't think your RCA is correct.

[s77rt]

@ShogunFire Yeah I remember such a bug. This one is probably a regression. This works fine on version 1.3.43-0.

[s77rt]

@DevWizard0000 Thanks for the proposal. Please follow the proposal template, we need to find the root cause first then we will work out a solution.

[s77rt]

Not overdue. Still looking for proposals

[trjExpensify]

@DevWizard0000 are you interesting in revising your proposal into the correct format?

[melvin-bot[bot]]

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

[ginsuma]

The root cause is minHeight from PR https://github.com/Expensify/App/pull/23222: https://github.com/Expensify/App/blob/8b75ed647769a96bc68e740fd21b4c80e6c4434e/src/styles/getNavigationModalCardStyles/index.website.js#L13 I followed the discussion between @s77rt and @jscardona12 on Slack here and saw both of you decided to remove it, but somehow it still exists. When we open the keyboard, the content moves beyond the top screen. If the content has a height bigger than the current window height, it will not show fully because we added minHeight with the same value to the wrapper in RHP screens. Currently, I don't find out any cases we need minHeight.

[studentofcoding]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Unresponsive view on 2 factors authentication screen on Mobile

What is the root cause of that problem?

App/src/styles/getNavigationModalCardStyles/index.website.js

Line 13 in 8b75ed6

Use KeyAvoidingView or SafeAreaView to cover the full page

What changes do you think we should make in order to solve the problem?

Cover the full page with KeyboardAvoidingView or SafeAreaView.

What alternative solutions did you explore? (Optional)

N/A

cc: @s77rt

[jscardona12]

@ginsuma @s77rt it is needed because with the new navigation implementation the minHeight attribute was on develop. The fix shouldn't be deleting it, it should be setting up to 0. If you delete the minHeight it would cause a side effect.

[s77rt]

@ginsuma Thanks! Indeed that seems to be the offending PR, but we need those changes (or other changes) to fix the 3 mentioned issues in the code comments.

[s77rt]

@studentofcoding Thanks for the proposal. You mentioned the offending PR but there is no root cause analysis. Can you please explain why that PR caused the regression?

[jscardona12]

Proposal

Please re-state the problem that we are trying to solve in this issue. Unresponsive view on 2 factors authentication screen on Mobile

What is the root cause of that problem?

The root cause is that because of App/src/styles/getNavigationModalCardStyles/index.website.js the height of the settings page will always be the window height.

The Root cause is that every time the keyboard appears on safari iit scrolls up the window changing the window height to windowHeight - window.scrollY. When the height become smaller the Next button goes up and do not stay in position because the styles are based on flex.

The minHeight is needed because it fixes the 3 mentioned issues in the code comments.

What changes do you think we should make in order to solve the problem?

We need to take add to the minHeight the window.scrollY. if the keyboard is not open it is 0. if it's open it would add the height that is being taken off the settingsInitialPage.

    export default ({windowHeight, isSmallScreenWidth}) => ({
    ...getBaseNavigationModalCardStyles({isSmallScreenWidth}),

// This height is passed from JavaScript, instead of using CSS expressions like "100%" or "100vh", to work around
// Safari issues:
// https://github.com/Expensify/App/issues/12005
// https://github.com/Expensify/App/issues/17824
// https://github.com/Expensify/App/issues/20709

height: windowHeight + window.scrollY,
minHeight: windowHeight,

});

What alternative solutions did you explore? (Optional)

in getWindowHeightAdjustment instead of return 0;

  return window.scrollY;

Both Solution Working:

https://github.com/Expensify/App/assets/16781411/465480aa-d297-40da-80bc-d4f9a5e7a3da

cc: @s77rt

[s77rt]

@jscardona12 Thanks for the proposal. Can you explain why this issue only occurs in Safari?

[jscardona12]

@s77rt it's a safari known issue.

https://bugs.webkit.org/show_bug.cgi?id=141832 Safari it's the only browser that considers this a feature.

Here is an article explaining it better: https://blog.opendigerati.com/the-eccentric-ways-of-ios-safari-with-the-keyboard-b5aa3f34228d

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[jscardona12]

@mvtglobally it's still reproducible on Dev

[s77rt]

@jscardona12 Thanks for the update. I think having the height controlled by the scroll position is a hacky solution. Even the very first solutions (https://github.com/Expensify/App/pull/12509 and https://github.com/Expensify/App/pull/19682) looks hacky, it may be better to rethink an entire new approach. Safari issues are tricky to handle and they mostly backfire.

[jscardona12]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• mWeb - Sign out - Button hidden under bottom, hard to sign out
• mWeb - Unresponsive view on 2 factors authentication screen

  What is the root cause of that problem?

  The root cause is that because of The root cause is a know issue in safari where it fails to update correctly the heights when the windowHeight is beig updated by external soures like the smartBanner.

What changes do you think we should make in order to solve the problem?

We need to add position: fixedintead of position: absolute these makes that safari updates the height value correctly. also add inset: 0for the View to fit all the page. and lastly because of the isSmallScreenWidth overflow-x: hiddento prevent the horizontal scroll.

    export default ({windowHeight, isSmallScreenWidth}) => ({
    ...getBaseNavigationModalCardStyles({isSmallScreenWidth}),

          position: "fixed",
         inset: 0,
         overflowX: "hidden",
         height: '100%',
    });

Aditionally becasue of the autofocus in the Verify.js the Keyboard is making a window scroll where sometimes the DOM is not able to update and the Verify page will look moved to the right.

To prevent this in BaseTwoFactorAuthForm.js we need to add the property autoFocus = false to avoid the keyboard appearing and messing up with the update.

return (
                  <MagicCodeInput
                  autoComplete={props.autoComplete}
                  textContentType="oneTimeCode"
                  label={props.translate('common.twoFactorCode')}
                  nativeID="twoFactorAuthCode"
                  name="twoFactorAuthCode"
                  value={twoFactorAuthCode}
                  onChangeText={onTextInput}
                  onFulfill={validateAndSubmitForm}
                  errorText={formError.twoFactorAuthCode ? props.translate(formError.twoFactorAuthCode) : 
        ErrorUtils.getLatestErrorMessage(props.account)}
                  ref={inputRef}
                 autoFocus={false}
               />
           );

What alternative solutions did you explore? (Optional)

Both Errors Working:

https://github.com/Expensify/App/assets/16781411/4eea2d05-380d-48e6-8974-3e58e4532023

cc: @s77rt

[s77rt]

@jscardona12 Thanks for the update. I don't think the solution is reliable, and we don't want to disable a feature to fix a bug.

[jscardona12]

@s77rt the solution wouldn't be reliable because of the disabling the keyboard autofocus or in general? If in general, why would it be unreliable?

[s77rt]

@jscardona12 I didn't test the solution but the fact that it does not work in all the cases e.g. in autofocus makes it unreliable.

[trjExpensify]

Ah interesting, I wonder whether the breakthrough from @staszekscp in https://github.com/Expensify/App/pull/23922 will help here?

[melvin-bot[bot]]

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

[s77rt]

@trjExpensify I think that issue is unrelated to this

[trjExpensify]

Fair enough!

[melvin-bot[bot]]

@trjExpensify @s77rt 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!

[s77rt]

Does not look we are close Melvin. Safari issues are tricky issues.

[trjExpensify]

Bumping the price to $2k!

@s77rt, out of curiosity, how are we're achieving this on other screens?

[melvin-bot[bot]]

Upwork job price has been updated to $2000

[s77rt]

@trjExpensify The bug only occurs if you focus an input that is located in the bottom half of the screen (or located in where the keyboard would normally show up) in that case Safari will scroll up which does not seem to work well with the solution added in https://github.com/Expensify/App/pull/23222.

[trjExpensify]

Gotcha. Let's see if someone has any bright ideas!

[jscardona12]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• mWeb - Sign out - Button hidden under bottom, hard to sign out
• mWeb - Unresponsive view on 2 factors authentication screen

  What is the root cause of that problem?

  The root cause is that because of The root cause is a know issue in safari where it fails to update correctly the heights when the windowHeight is beig updated by external soures like the smartBanner.

What changes do you think we should make in order to solve the problem?

We need to add position: fixedintead of position: absolute these makes that safari updates the height value correctly. Also this implementation allow to delete height and minHeight on getNavigationModalCardStyles/index.website.js

    export default ({isSmallScreenWidth}) => ({
    ...getBaseNavigationModalCardStyles({isSmallScreenWidth}),

          position: "fixed",
    });

What alternative solutions did you explore? (Optional)

Both Errors Working:

23481

https://github.com/Expensify/App/assets/16781411/3eb5c7cd-2fe5-441a-bacd-be3d12f9002b

20709

https://github.com/Expensify/App/assets/16781411/f801c528-2e9e-4a13-a9a0-05b5b52383dc

cc: @s77rt i fix the autofocus problem. let me know your thoughts

[s77rt]

@jscardona12 Thanks for the update. I think having to add position:'absolute', height: '100%' to every effected page (e.g. VerifyPage) is a workaround. Can we have a solution that makes the changes to the ScreenWrapper itself? (global solution)

Note: Please do not write every new proposal in a new comment, instead just update the existing one.

[jscardona12]

@s77rt in the updated version of main, the keyboard is not getting opened with autofocus, you need to tap the input for it to appear. i dont know if this is an error or an expected behaviour (@trjExpensify) If this is an expected behaviour we don't need to do anything on ScrapperWrapper, if it is, should this bug be reported?

[s77rt]

@jscardona12 It's still auto focusing from my side.

[jscardona12]

@s77rt that's weird, in my phone the input is focused but the keyboard does not appeared.

https://github.com/Expensify/App/assets/16781411/3b1bce60-0a28-4218-9878-eaae96aaf245

[s77rt]

@jscardona12 That's another issue I think. Safari limits the auto focus functionality. This works fine on the Simulator though.

[jscardona12]

@s77rt But it worked before i made merge, should we base behaviour on real devices or in simulators?

[s77rt]

@jscardona12 Let make sure it works on both (the auto focus is another problem).

[jscardona12]

@s77rt proposal updated. In the simulator and in real device is working fine.

[s77rt]

@jscardona12 Can you please revisit your proposal? It does not seem right? I can't read it :sweat_smile: Also it seems you are suggesting to remove height and and minHeight, please include that in your proposal as well.