[HOLD for payment 2023-05-09] [$2000] Connect Bank Account: Screen content is shifted to the top if user click on Fix the error button when the checkbox is unselected.

kavimuru 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!

Action Performed:

Login with any account
Go to Workspace => Connect Bank Account => Connect Bank Account Manually => Enter valid information for first steps.
At step 2, enter valid information but uncheck the checkbox, press Save, then press the Fix the error button.
Notice that screen content is shifted to the top of the screen.

Expected Result:

Screen content shouldn't be shifted to the top of the screen

Actual Result:

Screen content is shifted to the top of the screen.

Workaround:

unknown

Platforms:

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

[x] iOS / native

Version Number: 1.2.76-7 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/221250668-08d35eef-4510-4a0e-ae58-c6c4589b5e5f.MP4

https://user-images.githubusercontent.com/43996225/221250697-01dba691-105e-4463-a6d2-968ffa00c926.MP4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01869af11e64bd5d81
Upwork Job ID: 1637757059990470656
Last Price Increase: 2023-04-04

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

@johncschuster Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MelvinBot commented 1 year ago

@johncschuster Eep! 4 days overdue now. Issues have feelings too...

MelvinBot commented 1 year ago

@johncschuster Still overdue 6 days?! Let's take care of this!

johncschuster commented 1 year ago

Sorry for the delay on this. Let me follow the repro steps and get this one moving.

johncschuster commented 1 year ago

I tried following the exact repro steps above on v. 1.2.79-0 and was unable to reproduce the behavior. When I completed filling in the Company information, I left the checkbox unchecked and clicked the submit button to continue. This did not cause the screen content to shift to the top of the page.

Has the behavior been resolved in the latest release?

johncschuster commented 1 year ago

I was unable to reproduce this behavior myself, but @hungvu193 continues to be able to reproduce it. Here's a new video of reproduction:

https://user-images.githubusercontent.com/5579997/224068451-79d574c7-95ad-4691-bfb3-0b608021c5ee.MP4

MelvinBot commented 1 year ago

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

johncschuster commented 1 year ago

This isn't overdue (for Jules) Melv. Chill.

MelvinBot commented 1 year ago

@johncschuster @Julesssss this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Julesssss commented 1 year ago

Hey @johncschuster, I think you still need to fil out this checklist: https://github.com/Expensify/App/issues/15483#issuecomment-1444118303

Personally, I'm unable to verify as I can't get past Plaid due to not having a US bank account (no UK banks are listed). Also, the staging creds don't work for me :/

Julesssss commented 1 year ago

@kavimuru @hungvu193 & @johncschuster can you please share your exact device model and OS version please?

hungvu193: Iphone 13, ios 16.3

kavimuru commented 1 year ago

@Julesssss My device is iPhone X / 15.6

MelvinBot commented 1 year ago

@johncschuster, @Julesssss Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Julesssss commented 1 year ago

Hmm, occurring on many device types then... @johncschuster are you still unable to reproduce? I'm still unable to get past the initial Plaid step :/

MelvinBot commented 1 year ago

@johncschuster @Julesssss this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

Julesssss commented 1 year ago

Okay, we have 2/3 people able to reproduce. Let's open it up for external contribution.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01869af11e64bd5d81

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

hungvu193 commented 1 year ago

Proposal

Proposal

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

Screen content is shifted to the top if user click on Fix the error button when the checkbox is unselected.

What is the root cause of that problem?

The root cause of this issue is that scrollToOverflowEnabled is enable which allow scrollview scrolled beyond its content size.

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

Remove scrollToOverflowEnabled inside CompanyStep in Form component. https://github.com/Expensify/App/blob/382d167f2f92289d7341f9181a79ccc384f07c09/src/pages/ReimbursementAccount/CompanyStep.js#L158

What alternative solutions did you explore? (Optional)

None.

0xmiros commented 1 year ago

@hungvu193 scrollToOverflowEnabled is added intentionally in https://github.com/Expensify/App/pull/14409 which fixes https://github.com/Expensify/App/issues/13909. Make sure that your solution doesn't cause any regressoins.

hungvu193 commented 1 year ago

@0xmiroslav yes, I tested it as well. There's alternative solution that we only add scrollToOverflowEnabled with web version, I don't think it's necessary since I don't see any console error.

MelvinBot commented 1 year ago

@johncschuster @Julesssss @0xmiroslav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

MelvinBot commented 1 year ago

Current assignee @0xmiroslav is eligible for the Internal assigner, not assigning anyone new.

Julesssss commented 1 year ago

It seems to me that we haven't arrived at a desired solution yet. @johncschuster could we please double the bonus? Thanks

0xmiros commented 1 year ago

@hungvu193 it surely causes regression. Please check why scrollToOverflowEnabled was added on that PR.

We cannot remove scrollToOverflowEnabled (https://github.com/Expensify/App/pull/14409/files#r1081634916).

0xmiros commented 1 year ago

@Julesssss @johncschuster let's remove Internal and add External to get more proposals

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

Julesssss commented 1 year ago

Yeah, let's ignore the 4 week limit here, as we were held for a bit.

Seeking new proposals...

Ollyws commented 1 year ago

Proposal

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

On iOS when 'Fix the errors' is clicked, the scrollview scrolls until the input causing the error is at the top (in this case the checkbox), leaving a gap at the bottom if the input is close to the bottom of the scroll view.

What is the root cause of that problem?

This is due to the use of scrollToOverflowEnabled. This prop is iOS specific and allows the scrollview to be programmatically scrolled beyond its content size, so the checkbox is allowed to scroll to the top of the screen and the content size expands to accommodate this, leaving blank space below.

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

We can implement a limit that will scroll to the bottom of the scrollview if we attempt to scroll beyond its contentsize in onFixTheErrorsLinkPressed.

To do this we can utilize onContentSizeChange to get the contentHeight and onLayout to get the layoutHeight (event.nativeEvent.layout.height) of the ScrollViewWithContext. (contentHeight - layoutHeight) will give us the maximum contentOffset.

Then before we scroll to the input we can check if the y value returned by measureLayout is bigger than (contentHeight - layoutHeight), if it is then we scroll to (contentHeight - layoutHeight) and early return, which will scroll us to the bottom of the scrollView.

MelvinBot commented 1 year ago

@johncschuster, @Julesssss, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

0xmiros commented 1 year ago

@Ollyws thanks for your proposal. Your solution seems work but what will happen if picker supports measureLayout? For now, it's not supported so manual scroll isn't applied to picker but once it's supported, it will cause regression.

cubuspl42 commented 1 year ago

Proposal

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

On iOS, when the software keyboard is open for a text input, the text input always stays visible.

This is because in our ScreenWrapper component, we wrap the app content (marked red) in KeyboardAvoidingView, which (on iOS) adds keyboard-aware space under its children, giving space for the scrollable content (marked blue) to scroll down, so the text input can stay visible.

We can observe this fully when scrolling all the way up when the software keyboard is open.

On iOS, the picker opens a modal that's somehow similar to the software keyboard.

The difference is that the window is not resized, so the "push-up" effect cannot take place.

In effect, the picker that opened the modal can become hidden. We'd like an effect similar to the text input/software keyboard experience.

We have a solution in the form of scrollToOverflowEnabled, but that solution has an inherent side-effect of enabling overflow in the scroll view, in general. Which sometimes doesn't look good, like in the case reported in this issue.

We'd like to have a solution that solves the mentioned problem without having problematic side effects.

What is the root cause of that problem?

We handle software keyboard and UIPickerView's modal inconsistently. As a result, sometimes the picker can be hidden by its own modal. Our existing solution to that problem (scrollToOverflowEnabled) creates new problems.

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

We should solve the original problem better.

For iOS, I propose to mirror our handling of the software keyboard for the picker's modal. Let's have an empty space (marked yellow) at the bottom of the screen, below the scroll view (marked pink), when the picker is open. Let's disable scrollToOverflowEnabled. This will have a similar effect to how we treat the software keyboard, giving space for the scrollable content (marked blue) to be "pushed up", although the app window (marked red) stays the same size.

While KeyboardAvoidingView is provided by React Native core, no such thing exists for the picker's modal. Let's fix that.

We could inject the extra space at the bottom of the screen like this:

src/components/withPickerState.js:

// ...

const PickerStateContext = createContext(null);

const pickerStatePropTypes = {
    /** Whether the picker is open */
    isPickerShown: PropTypes.bool.isRequired,

    notifyPickerShown: PropTypes.func,

    notifyPickerHidden: PropTypes.func,
};

const pickerStateProviderPropTypes = {
    /* Actual content wrapped by this component */
    children: PropTypes.node.isRequired,
};

// Otherwise a full mirror of our `withKeyboardState.js`
// ...

src/App.js:

// ...

const App = () => (
    <GestureHandlerRootView style={fill}>
        <ComposeProviders
            components={[
                OnyxProvider,
                SafeAreaProvider,
                PortalProvider,
                SafeArea,
                LocaleContextProvider,
                HTMLEngineProvider,
                WindowDimensionsProvider,
                KeyboardStateProvider,
                PickerStateProvider, // <-- Add a new provider
            ]}
        >
            <CustomStatusBar />
            <ErrorBoundary errorMessage="NewExpensify crash caught by error boundary">
                <Expensify />
            </ErrorBoundary>
        </ComposeProviders>
    </GestureHandlerRootView>
);

// ...

src/components/Picker/BasePicker.js:

// ...

class BasePicker extends PureComponent {
    // ...

    render() {
        // ...

        return (
            <>
            // ...
                    <RNPickerSelect
                        // ...
                        onOpen={() => {
                            this.enableHighlight();
                            this.props.notifyPickerShown();
                            this.scrollToInput();
                        }}
                        onClose={() => {
                            this.disableHighlight();
                            this.props.notifyPickerHidden();
                        }}
                    // ...
                    />
            // ...
            </>
        );
    }
}

src/components/PickerAvoidingView/index.ios.js:

// ...

function PickerAvoidingView(props) {
    return (
        <View style={styles.container}>
            <View style={styles.top}>
                {props.children}
            </View>
            {props.isPickerShown && <View style={styles.bottom} />}
        </View>
    );
}

// ...

src/components/ScreenWrapper/index.js:

class ScreenWrapper extends React.Component {
    // ...

    render() {
        return (
            <SafeAreaConsumer>
            // ...

                            <KeyboardAvoidingView /* ... */>
                                <PickerAvoidingView> // <-- Add this view
                                    // ...
                                </PickerAvoidingView>
                            </KeyboardAvoidingView>
            // ...
            </SafeAreaConsumer>
        );
    }
}

Alternatively, the global state management provider and PickerAvoidingView could be submitted to our picker dependency (react-native-picker-select), or even its dependency (@react-native-picker/picker). The first one isn't really active anymore, but we have a fork. The second one is also not really active anymore, we don't have a fork, but I recently managed to contact the maintainer, so at least we know he's alive.

After applying the solution:

https://user-images.githubusercontent.com/2590174/229563287-4e625e51-9d9e-490d-b1a2-ca5ec4473d1e.mp4

MelvinBot commented 1 year ago

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

johncschuster commented 1 year ago

@Julesssss / @0xmiroslav, what are your thoughts on the current proposals? Do you feel we nee to adjust the bounty to get a fire under this one?

Ollyws commented 1 year ago

@Ollyws thanks for your proposal. Your solution seems work but what will happen if picker supports measureLayout? For now, it's not supported so manual scroll isn't applied to picker but once it's supported, it will cause regression.

@0xmiroslav Are you referring to https://github.com/Expensify/App/pull/16433 ? It's merged now and my solution works fine with picker using measureLayout. All measureLayout is being used for is getting the y position of the form element which we're scrolling to (or in my solution not scrolling to if it will be beyond the bounds of the scrollview)

cubuspl42 commented 1 year ago

@0xmiroslav @Ollyws

From my perspective, the issue with the proposed solution is that it (fundamentally) works only for the "Fix the errors" button, not touching the essence of the problem (as I see it).

When any other piece of code triggers scrolling, the empty space on the bottom (that comes from the scrollToOverflowEnabled) will be seen again.

Indeed, this can be observed when interacting with pickers (although not related to #16433):

Enter a screen, when the picker is very low in the scrollable content
- Best seen if the picker is literally the last thing in the content, but not limited to that case
Tap that picker
Tap "Done"

(I modified the screen to put the picker as a last thing inside the scroll view, so the empty space is well-visible, but it can also be observed without such alteration)

If there are/will be any other cases of scrolling to something besides the "Fix the errors" button, the problem will keep arising.

Julesssss commented 1 year ago

Hey @cubuspl42, thanks for the detailed proposal. It sounds like a decent solution to me, but I'm just wondering whether this is solvable at the native iOS layer instead. The underlying issue sounds similar to the Android keyboard issue, which we handle with the windowSoftInputMode attribute.

If we can rule out a native solution, or show this is a common problem I think we can move forward. cc @0xmiroslav

cubuspl42 commented 1 year ago

@Julesssss I do agree that a cleaner solution would be preferred if available!

By "solving at the native side", do you mean making iOS resize the app window when the modal is open, like it does in case of text inputs?

0xmiros commented 1 year ago

@Ollyws can you update your proposal based on latest codebase? It's scrolling to weird position right now.

0xmiros commented 1 year ago

@cubuspl42 I agree with @Julesssss's feedback. Let's try to fix in low level. ScrollView should be pure component, not adding special values like picker open.

Ollyws commented 1 year ago

@0xmiroslav Strange, it's working fine for me. Here are the exact changes I'm testing with:

Code diff

```diff diff --git a/src/components/Form.js b/src/components/Form.js index 09e86cb869..775a15d024 100644 --- a/src/components/Form.js +++ b/src/components/Form.js @@ -345,7 +345,14 @@ class Form extends React.Component { if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') { // We measure relative to the content root, not the scroll view, as that gives // consistent results across mobile and web - focusInput.measureLayout(formContentRef, (x, y) => formRef.scrollTo({y: y - 10, animated: false})); + const maxOffset = this.contentHeight - this.layoutHeight; + focusInput.measureLayout(formContentRef, (x, y) => { + if (y > maxOffset) { + formRef.scrollTo({y: maxOffset, animated: false}); + return; + } + formRef.scrollTo({y: y - 10, animated: false}); + }); } // Focus the input after scrolling, as on the Web it gives a slightly better visual result @@ -366,6 +373,8 @@ class Form extends React.Component { {({safeAreaPaddingBottomStyle}) => (this.props.scrollContextEnabled ? ( this.contentHeight = h} + onLayout={e => this.layoutHeight = e.nativeEvent.layout.height} style={[styles.w100, styles.flex1]} contentContainerStyle={styles.flexGrow1} keyboardShouldPersistTaps="handled" ```

Julesssss commented 1 year ago

By "solving at the native side", do you mean making iOS resize the app window when the modal is open, like it does in case of text inputs?

I don't know native iOS well enough to be sure, but yeah something like that which prevents us from needing a non-obvious workaround.

cubuspl42 commented 1 year ago

@0xmiroslav

@cubuspl42 I agree with @Julesssss's feedback. Let's try to fix in low level. ScrollView should be pure component, not adding special values like picker open.

Actually, I do agree with that. I tried to do that in the beginning, but I had trouble getting it to work. But after reading your words, I revisited that approach and, after some minor tweaks, it's actually simpler than what I previously proposed. I updated the proposal!

The essence of the approach didn't change much, but the solution is moved out from ScrollViewWithContext. Now it's even closer to what I conceptually was aiming for.

@Julesssss

Please see the updated proposal. Would you agree with me, that this is conceptually what we're looking for, but it would be even better if we could configure the system to do this for us, on the native level?

The sad news is that, on iOS, there doesn't seem to be any equivalent to Android's windowSoftInputMode even for the software keyboard, not to mention UIPickerView's modal (which is not literally a keyboard). To the best of my understanding, everybody is solving this on their own. For the text inputs and software keyboard, I believe that it's React Native which is doing the "push up" logic (I previously thought that it was the iOS itself).

cubuspl42 commented 1 year ago

@0xmiroslav @Julesssss

Guys, I think I got this. After following your advice, I figured out how to do this with (nearly) 100% symmetry to how we handle the software keyboard on iOS. The proposal is updated again.

The PickerAvoidingView couldn't have been a part of React Native, as React Native doesn't provide built-in pickers (anymore).

@react-native-picker/picker doesn't provide PickerAvoidingView, and this is understandable: it requires two extra parts that need explicit injecting into the app (the state manager and the avoiding view) while it provides a bit less benefit than KeyboardAvoidingView. It's necessary to see what you type while you type, but it's just nice to see the picker while you pick (when the picker's modal contains all the information).

Expensify / App