App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Software keyboard covers part of the submit button on login page

Open roryabraham opened this issue 3 years ago • 21 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open the login page
  2. Tap on the text input to enter text
  3. Click Continue
  4. Password field will be shown

Expected Result:

The software keyboard should open, and the text input, submit button, etc... should shift up such that it's completely in view

Actual Result:

The submit button is partially covered.

Workaround:

n/a – just ignore the visual bug

Platform:

Where is this issue occurring?

  • Mobile Web (Android Chrome)

Version Number: Reproducible in staging?: Reproducible in production?: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

View all open jobs on GitHub

roryabraham avatar Oct 10 '22 16:10 roryabraham

Interestingly, if you scroll the page up and down with the keyboard open, then this bug no longer happens. You can completely close the keyboard, scroll back to the top, and then tap on the TextInput and it does not happen again. If you then refresh the page, then it will happen again.

roryabraham avatar Oct 10 '22 16:10 roryabraham

I removed the HOLD on this and assigned it to me because this is fixed in https://github.com/Expensify/App/pull/11586

tgolen avatar Oct 13 '22 17:10 tgolen

Linked PR hit production 12 hours ago. I tested this and all looked good. @tgolen are you seeing the same?

JmillsExpensify avatar Nov 03 '22 03:11 JmillsExpensify

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

  • [ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
  • [ ] The PR that introduced the bug has been identified. Link to the PR:
  • [ ] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [ ] A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [ ] Payment has been made to the issue reporter (if applicable)
  • [ ] Payment has been made to the contributor that fixed the issue (if applicable)
  • [ ] Payment has been made to the contributor+ that helped on the issue (if applicable)

melvin-bot[bot] avatar Nov 03 '22 13:11 melvin-bot[bot]

hey @JmillsExpensify, thanks for testing that! I had originally thought that this was fixed by my PR, and included it in the list of fixes. However, after some testing, we found that there was still an issue on Android Chrome. You can see here, on the password form, the sign in button is still partially covered:

image

tgolen avatar Nov 03 '22 13:11 tgolen

Oh nice! I tested on iOS and wasn't able to see it anymore. I'll update the testing reproduction steps and remove Safari so this issue focuses on Android wWeb moving forward. What do you think?

JmillsExpensify avatar Nov 03 '22 13:11 JmillsExpensify

Yeah, sounds great! I also saw that it was working on iOS 👍

tgolen avatar Nov 03 '22 13:11 tgolen

Nice, done! In other news we really need a way for Bug Zero to test Android. The majority of the team doesn't have a way to do it right now.

JmillsExpensify avatar Nov 03 '22 14:11 JmillsExpensify

Nice, done! In other news we really need a way for Bug Zero to test Android. The majority of the team doesn't have a way to do it right now.

We're going to work with Software Mansion to hopefully make this easier in the near future.

roryabraham avatar Nov 03 '22 17:11 roryabraham

That would be really awesome, thanks for the heads up.

JmillsExpensify avatar Nov 03 '22 18:11 JmillsExpensify

Contributors - this issue is open for proposals! The related Upwork job is here: https://www.upwork.com/jobs/~010b091da824bb7cb2.

JmillsExpensify avatar Nov 06 '22 20:11 JmillsExpensify

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

melvin-bot[bot] avatar Nov 06 '22 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 06 '22 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 06 '22 20:11 melvin-bot[bot]

we can customize the login form height by using eventListener in the keyboard show and hide status, using the windows height size decrease the size of keyboard and show the login form.

at show status we will decrease the size of keyboard from window-height

at hide status we will use the full window-height for the login-form

we use Animated and UIManager class( components) from 'react-native'

AmanuelAsfaw avatar Nov 07 '22 12:11 AmanuelAsfaw

When I was initially looking at this, I felt like the root cause of the problem came from this style:

https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/styles/styles.js#L943

I think the top margin is always going to be a % of the page height, and the page height doesn't change when the keyboard is open, then the marginTop is the wrong way to implement the spacing in the design.

Could we instead look at changing that marginTop style to something which is based on flex layout and maybe has a max height that is equal to whatever the 40% margin height was adding?

tgolen avatar Nov 07 '22 15:11 tgolen

Maybe it was actually this style:

https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/styles/styles.js#L952

I think it's maybe one of them, if not both

tgolen avatar Nov 07 '22 16:11 tgolen

Still waiting for proposals that address the root cause and issue.

JmillsExpensify avatar Nov 08 '22 19:11 JmillsExpensify

Very sorry for being too late to respond, I clone the source code from git and try to test from my local machine but when I try to run a command for web npm run web it doesn't work for me. I corrected my npm(8.11.0) and node(16.15.1) version but it still does not work for me.

I see a helper component which is a HOC to solve this problem in your source code https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/components/withKeyboardState.js WithKeyboardState https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/components/withKeyboardState.js it give the status of keyboard showing or not, using that status we can change the style using props in the line https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/pages/signin/SignInPageLayout/SignInPageContent.js#L63 SignInPageContent https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/pages/signin/SignInPageLayout/SignInPageContent.js#L63

in my proposal we have to use the state of keyboard showing or not status in the SignInPageContent https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/pages/signin/SignInPageLayout/SignInPageContent.js#L63 page

if the keyboard shows get first we calculate windows-height, keyboard-height, currently-focused-field-height then decrease the keyboard-height and currently-focused-field-height from the windows-height we can get the top marginTop for that KeyboardAvoidingView

or we use the calculated proportional value of marginTop using % for the above value we get.

I put a snippet code on git https://gist.github.com/AmanuelAsfaw/1e87af061927fd21f92be7527a3d5e93 Source-code https://gist.github.com/AmanuelAsfaw/1e87af061927fd21f92be7527a3d5e93

On Tue, Nov 8, 2022 at 10:43 PM Jason Mills @.***> wrote:

Still waiting for proposals that address the root cause and issue.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11701#issuecomment-1307742108, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMAQSWZOBPAY7NJXJF4JFYLWHKUONANCNFSM6AAAAAARBQV6PM . You are receiving this because you commented.Message ID: @.***>

AmanuelAsfaw avatar Nov 11 '22 13:11 AmanuelAsfaw

Thanks for the response. A quick heads up that @tgolen is out of office until next week, though for the sake of clarity, we do require that for a proposal to be accepted, it has to follow our guidelines. Here's an example of a proposal that was later accepted. Thank you!

JmillsExpensify avatar Nov 11 '22 19:11 JmillsExpensify

Thank you for the clarification. I wait for the response

On Fri, Nov 11, 2022 at 10:53 PM Jason Mills @.***> wrote:

Thanks for the response. A quick heads up that @tgolen https://github.com/tgolen is out of office until next week, though for the sake of clarity, we do require that for a proposal to be accepted, it has to follow our guidelines https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#propose-a-solution-for-the-job. Here's an example of a proposal https://github.com/Expensify/App/issues/11236#issuecomment-1256398719 that was later accepted. Thank you!

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11701#issuecomment-1312142742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMAQSW3FPOF2YJ3QDMQBXELWH2P4ZANCNFSM6AAAAAARBQV6PM . You are receiving this because you commented.Message ID: @.***>

AmanuelAsfaw avatar Nov 12 '22 12:11 AmanuelAsfaw

Open source community - we're still awaiting for approved proposals! It's been a week, so we're doubling the price to $500. Upwork job is here: https://www.upwork.com/jobs/~010b091da824bb7cb2.

JmillsExpensify avatar Nov 14 '22 19:11 JmillsExpensify

Great, thanks for the update, I think I finished the process on github, so I can submit the proposal on Upwork?

On Mon, Nov 14, 2022 at 10:54 PM Jason Mills @.***> wrote:

Open source community - we're still awaiting for approved proposals! It's been a week, so we're doubling the price to $500. Upwork job is here: https://www.upwork.com/jobs/~010b091da824bb7cb2.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11701#issuecomment-1314292944, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMAQSW65WLHZZCQLVY56TCTWIKKFXANCNFSM6AAAAAARBQV6PM . You are receiving this because you commented.Message ID: @.***>

AmanuelAsfaw avatar Nov 15 '22 08:11 AmanuelAsfaw

@AmanuelAsfaw can please submit proposals on github. Once it's approved, you can then apply on upwork

Recommended read - https://github.com/Expensify/ReactNativeChat/blob/main/contributingGuides/CONTRIBUTING.md

rushatgabhane avatar Nov 15 '22 16:11 rushatgabhane

Proposal

To get the Keyboard height and showing status using this higher order component App/src/components/withKeyboardState.js and add some features on the below line 8 https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/components/withKeyboardState.js#L8 add this code keyboardHeight: PropTypes.number.isRequired,

, change line 15 https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/components/withKeyboardState.js#L15-L17 to this code

this.state = { isShown: false, keyboardHeight: 0, }; and change line 23 - 25 https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/components/withKeyboardState.js#L23-L25 to code (event) => { this.setState({isShown: true, keyboardHeight: event.endCoordinates.height }); }, now we get the height of Keyboard, now we can access the keyboard height form SingInPage

Now access height and change mirginTop in side the page while the keyboard showing On line 63 https://github.com/Expensify/App/blob/debeb86a0a1134c578331499a25bb9096b8dc8b0/src/pages/signin/SignInPageLayout/SignInPageContent.js#L63 check the status of keyboard showing and if showing use the calculate marginTop value this code props.isShown? { marginTop: getCalculateMarginTop(), }: styles.signInPageNarrowContentMargin : {}, prepare function to get the calculate marginTop value

function getCalculateMarginTop(){
    const windows_height = Dimensions.get('window').height;
    const keyboard_height = props.keyboardHeight ? props.keyboardHeight : 0;
    let current_focused_field_height = 0;
    
    const { State: TextInputState } = TextInput;
    const current_focused_field = TextInputState.currentlyFocusedField();

    UIManager.measure(current_focused_field, (originX, originY, width, height, pageX, pageY) => {
        current_focused_field_height = height;
    });

    const margin_top = windows_height - keyboard_height - current_focused_field_height;
    return margin_top;
}

AmanuelAsfaw avatar Nov 16 '22 14:11 AmanuelAsfaw

@AmanuelAsfaw Thank you for the proposal. I think that is kind of on the wrong track though. I'm going to take this over and explore my theory that I posted here: https://github.com/Expensify/App/issues/11701#issuecomment-1305784305

tgolen avatar Nov 16 '22 22:11 tgolen

Thanks for closing the loop @tgolen.

JmillsExpensify avatar Nov 17 '22 05:11 JmillsExpensify

I got some good progress on this today and have a WIP PR here: https://github.com/Expensify/App/pull/12848

I should be able to wrap it up tomorrow.

It basically entailed an entire refactoring of how that view is laid out, and it's much better now.

tgolen avatar Nov 17 '22 23:11 tgolen

Had unassigned because of pending work. But my backlog got cleared sooner than expected :)

@JmillsExpensify could you please reassign me if that's okay. I can then start reviewing the PR

rushatgabhane avatar Nov 18 '22 04:11 rushatgabhane

@tgolen Super great to hear!

@rushatgabhane done.

JmillsExpensify avatar Nov 18 '22 08:11 JmillsExpensify