search

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
3.34k stars 2.77k forks source link

[HOLD for payment 2023-07-05] [$2000] iOS - Keyboard does not hide when switching between attachments with focused password input field #19156

Closed mvtglobally closed 1 year ago

mvtglobally 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:

  1. Open the App
  2. Login with any account
  3. Send several attachments including PDF-protected file
  4. Click to preview on PDF-protected file -> Press Enter a password
  5. Switching to another attachment

Expected Result:

Keyboard should be hidden

Actual Result:

Keyboard does not hide

Workaround:

unknown

Platforms:

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

Version Number: v1.3.15-6 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/43995119/6f87bbd7-de1d-4df5-8197-a060b73f80c0

Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014d32d06c969c5ab1
  • Upwork Job ID: 1661854565030178816
  • Last Price Increase: 2023-06-21
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

ahmedGaber93 commented 1 year ago

Proposal

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

Keyboard does not hide when switching between attachments with focused password input field.

What is the root cause of that problem?

There is no Keyboard.dismiss(); when next/prev button press

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

  1. When the user navigates away from the PDF password form by clicking on the arrow buttons, dismiss the keyboard. call Keyboard.dismiss(); in next/prev button onPress here and here

  2. When the user clicks on the eye icon while the keyboard is open, it should toggle immediately without dismissing keyboard. Add keyboardShouldPersistTaps="handled" to AttachmentCarousel Flatlist here.

  3. When the user navigates away from the PDF password form to other attachments by swiping, dismiss the keyboard Add keyboardDismissMode="on-drag" to AttachmentCarousel Flatlist here.

  4. When the user navigates back to the PDF password form, focus the input and bring up the keyboard again pass isPageActive to AttchmentView here then here then here

    // in AttchmentView
isPageActive={this.state.attachments[this.state.page].source === item.source}
// in PDFPasswordForm
componentDidUpdate(prevProps) {
  if (!prevProps.isPageActive && this.props.isPageActive && this.textInputRef) {
      this.textInputRef.focus();
  }
}

<TextInput ref={(el) => (this.textInputRef = el)}  />

  5. fix auto-focus doesn't work when swipe between protected PDFs quickly

This because when you swipe quickly, swipe not complete to view the whole PDF. We can fix it by reduce itemVisiblePercentThreshold: 95 to 75 https://github.com/Expensify/App/blob/035783a7984e0214677b5364d9351c968887bf58/src/components/AttachmentCarousel/index.js#L53-L57

  1. fix auto-focus not work in safari (this issue happened with me last week, but now it is not reproducible). If this issue appears again, the fix should be. surround this.textInputRef.focus(); with timeout 300 if browser is mobile safari

    // in PDFPasswordForm
import * as Browser from '../../libs/Browser';

componentDidUpdate(prevProps) {
  if (!prevProps.isPageActive && this.props.isPageActive && this.textInputRef) {
      const isMobileSafari = Browser.isMobileSafari();
      if(isMobileSafari){
          setTimeout(() => this.textInputRef.focus(), 300)
      }else{
          this.textInputRef.focus();
      }
  }
}

    What alternative solutions did you explore? (Optional)

sonialiap commented 1 year ago

I'm unable to reproduce on the browserstack emulator on iPhone 13 v16.3.

The arrow buttons are inconsistent. I was not able to figure out how to get them to show up if they weren't showing up. Swiping between attachments closes the keyboard.

Enjoy my struggle 😂

https://github.com/Expensify/App/assets/17131195/16ea95e1-6b6d-43a5-8722-75b1d304e971

sonialiap commented 1 year ago

@ahmedGaber93 since you proposed a solution, are you able to reproduce this issue?

ahmedGaber93 commented 1 year ago

@sonialiap yes, I am able to reproduce.

https://github.com/Expensify/App/assets/41129870/e86d35f5-50d2-4be1-8b04-0194734dc0ca

sonialiap commented 1 year ago

Thanks! Adding the external label

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~014d32d06c969c5ab1

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

ahmedyasin21 commented 1 year ago

Myself Ahmed Yasin, a certified React Native Developer.

Since you're using keyboard.dismiss() . it'll surely work if you place it on a right place and in a right way. Once you flow the documentation right. It'll be solve. I have faced this issue while developing one of my app. And I resolved and saved the solution in may notes. I do this with every single error I faced.

I'm right for this job. So , hoping of response.

melvin-bot[bot] commented 1 year ago

📣 @ahmedyasin21! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
ahmedyasin21 commented 1 year ago

Contributor details Your Expensify account email: ahmedyasin1947@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/mahmedyasin

Myself Ahmed Yasin, a certified React Native Developer.

Since you're using keyboard.dismiss() . it'll surely work if you place it on a right place and in a right way. Once you follow the documentation right. It'll be solve. I have faced this issue while developing one of my app. And I resolved and saved the solution in may notes. I do this with every single error I faced.

I'm confident and feel myself a best contributor for this job. So , hoping for your response.

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

dukenv0307 commented 1 year ago

Proposal

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

Keyboard does not hide when switching between attachments with focused PDF password input field

What is the root cause of that problem?

There're total of 4 issues here as per the expanded scope:

  1. When the user navigates away from the PDF password form by clicking on the arrow buttons, dismiss the keyboard

The reason why clicking the Arrow Buttons do not dismiss the Keyboard is because of the keyboardShouldPersistTaps="handled" here https://github.com/Expensify/App/blob/98ae1aed1147d6d5875ed9b1bc06871d1a7f4b0f/src/pages/home/report/ReportActionsList.js#L183.

The right is children of the left: ReportActionsList -> ReportActionItem -> AttachmentModal -> arrows.

  1. When the user clicks on the eye icon while the keyboard is open, it should toggle immediately without dismissing keyboard

It's because the FlatList here doesn't have keyboardShouldPersistTaps="handled" and it's capturing all the tap (the arrows are not inside the FlatList so are not affected).

  1. When the user navigates away from the PDF password form to other attachments by swiping, dismiss the keyboard

Same as 1, but this only happens once we fix the 2.

  1. When the user navigates back to the PDF password form, focus the input and bring up the keyboard again

This is a new improvement

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

There're these changes we need to make, corresponding to the issues above:

  1. adding Keyboard.dismiss() to updatePage here (make sure to add on top of the updatePage block) https://github.com/Expensify/App/blob/af6fb0768b65e95f8a9feb5e36626ae9df5f2097/src/components/AttachmentCarousel/index.js#L210 will resolve both 1 and 3. That means we'll dismiss the keyboard whenever the viewable item in the attachment change (see here). This will work for all cases, meanwhile if we dismiss keyboard individually on each action (when clicking on the arrows or scrolling), if the attachment changes by other means (like by pressing <, > in keyboard), the issue will still happen.

We can optionally check if the keyboard is open and only dismiss if true, if keyboard is not open there's no need to dismiss it.

  1. We need to add keyboardShouldPersistTaps="handled" to the FlatList here.

  2. Same as 1

  3. We need to make sure when navigate back to the PDF password from, the input will be focused again, this is because the password field is the only input in the page, so it's important to bring up the Keyboard again if we navigate back to it, for the best UX.

What alternative solutions did you explore? (Optional)

NA

MariaHCD commented 1 year ago

Not overdue, waiting on @aimane-chnaif to review the proposals above

mvtglobally commented 1 year ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 1 year ago

@MariaHCD @sonialiap @aimane-chnaif 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!

aimane-chnaif commented 1 year ago

We need to make sure when navigate back to the PDF password from, the input will be focused again, this is because the password field is the only input in the page, so it's important to bring up the Keyboard again if we navigate back to it, for the best UX.

@MariaHCD I like this improvement. Can this be fixed here or out of scope?

aimane-chnaif commented 1 year ago

@ahmedGaber93 @dukenv0307 manually dismissing keyboard onPress will work but I noticed that after swiping left/right, keyboard dismisses automatically. Can you find out how it works and apply same to arrow buttons?

dukenv0307 commented 1 year ago

I noticed that after swiping left/right, keyboard dismisses automatically

@aimane-chnaif I believe the native behavior of Keyboard on iOS is that:

MariaHCD commented 1 year ago

I like this improvement. Can this be fixed here or out of scope?

@aimane-chnaif I like that improvement too! I think it's within the scope of this issue

aimane-chnaif commented 1 year ago

If we press on a pressable element (provided that it doesn't navigate the page), the Keyboard will not dismiss

@dukenv0307 I thought so from the code. But testing doesn't prove that. Keyboard dismiss on iOS when press 👁️ icon. Can you please figure out why?

https://github.com/Expensify/App/blob/9fa12f3cd6bc422d2b9723ac98d575c7ce48f05d/src/components/PDFView/PDFPasswordForm.js#L97-L98

dukenv0307 commented 1 year ago

Keyboard dismiss on iOS when press 👁️ icon

@aimane-chnaif that's another bug in itself rather than the norm, it's because the FlatList here doesn't have keyboardShouldPersistTaps="handled" and it's capturing all the tap (the arrows are not inside the FlatList so are not affected).

We need to add keyboardShouldPersistTaps="handled" to it.

dukenv0307 commented 1 year ago

^ Tested the above and confirmed, @aimane-chnaif are you ok to create a new bug report for it (the Confirm and Eyes button are not clickable when Keyboard is open)? I feel adding it here increases the issue scope too much since this issue already includes this extra improvement

dukenv0307 commented 1 year ago

@aimane-chnaif and the reason why clicking the Arrow Buttons do not dismiss the Keyboard is because of the keyboardShouldPersistTaps="handled" here https://github.com/Expensify/App/blob/98ae1aed1147d6d5875ed9b1bc06871d1a7f4b0f/src/pages/home/report/ReportActionsList.js#L183.

The right is children of the left: ReportActionsList -> ReportActionItem -> AttachmentModal -> arrows.

We cannot remove keyboardShouldPersistTaps="handled" there because if we do that, the arrows won't be clickable when the Keyboard is open (and a lot of other issues). I believe for most cases we always want buttons to be clickable when the Keyboard is opened. Addtionally dismissing the keyboard is what we can add in an ad-hoc basis so I believe Keyboard.dismiss() is the way to go here.

aimane-chnaif commented 1 year ago

are you ok to create a new bug report for it (the Confirm and Eyes button are not clickable when Keyboard is open)? I feel adding it here increases the issue scope too much since this issue already includes this extra improvement

I don't see any reason for making it out of scope. As you see, after adding keyboardShouldPersistTaps="handled" in horizontal FlatList, same bug happens while swipe. I believe we should fix that case here as well.

If you feel this requires more efforts as it fixes multiple bugs related to password protected pdf, we can increase bounty.

https://github.com/Expensify/App/assets/96077027/5f9dfaef-105d-4feb-bf12-242e21dc10e7

Thoughts @MariaHCD?

melvin-bot[bot] 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? 💸

MariaHCD commented 1 year ago

If you feel this requires more efforts as it fixes multiple bugs related to password protected pdf, we can increase bounty.

Agreed with @aimane-chnaif. I think it would be better to fix the bugs associated with password protected PDF attachments and the keyboard as part of this issue. We can definitely increase the bounty if there is more effort and complexity involved. What do you think, @sonialiap?

So currently, the scope of the solution includes:

  1. When the user navigates away from the PDF password form by clicking on the arrow buttons, dismiss the keyboard
  2. When the user clicks on the eye icon and navigates away from the form, dismiss the keyboard
  3. When the navigates back to the PDF password form, focus the input and bring up the keyboard again

Is there anything I'm missing?

aimane-chnaif commented 1 year ago

I think 2. is a bit unclear. It can be split like this:

Another edge case I concern:

melvin-bot[bot] commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

MariaHCD commented 1 year ago

Nice, thanks, @aimane-chnaif!

So the scope is:

  1. When the user navigates away from the PDF password form by clicking on the arrow buttons, dismiss the keyboard
  2. When the user clicks on the eye icon while the keyboard is open, it should toggle immediately without dismissing keyboard
  3. When the user navigates away from the PDF password form to other attachments by swiping, dismiss the keyboard
  4. When the user navigates back to the PDF password form, focus the input and bring up the keyboard again
MariaHCD commented 1 year ago

There might be keyboard glitch when navigate between continuous PDF password forms

Good point! I think this is something the contributor should test for when outlining their proposal. Hopefully, we can catch any other edge cases when testing the final solution PR.

dukenv0307 commented 1 year ago

@aimane-chnaif @MariaHCD adding Keyboard.dismiss() to updatePage here https://github.com/Expensify/App/blob/af6fb0768b65e95f8a9feb5e36626ae9df5f2097/src/components/AttachmentCarousel/index.js#L210 will resolve both 1 and 3. That means we'll dismiss the keyboard whenever the viewable item in the attachment change (see here).

There might be keyboard glitch when navigate between continuous PDF password forms

@aimane-chnaif @MariaHCD please see the following video, the first keyboard closes and the second opens gracefully, so I think it's ok

https://github.com/Expensify/App/assets/129500732/4ddecbf1-843d-441b-9bb2-1f19f5ed19f8

aimane-chnaif commented 1 year ago

please see the following video, the first keyboard closes and the second opens gracefully, so I think it's ok

iOS seems fine. did you also test android?

ahmedGaber93 commented 1 year ago

Proposal

Updated @aimane-chnaif cover all 4 points here .

dukenv0307 commented 1 year ago

iOS seems fine. did you also test android?

@aimane-chnaif Android looks fine to me as well

https://github.com/Expensify/App/assets/129500732/7e2dc3e1-d6a7-4f75-8d0b-98ee22d90117

dukenv0307 commented 1 year ago

@aimane-chnaif I've updated the proposal to include root causes and solutions for all 4 issues. These root causes and solutions are already posted first in comments when we make the decisions and scope increases through out the discussion.

MariaHCD commented 1 year ago

Not overdue, @aimane-chnaif will evaluate the proposals we have already.

melvin-bot[bot] commented 1 year ago

@MariaHCD @sonialiap @aimane-chnaif 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!

aimane-chnaif commented 1 year ago

The proposal is most likely to be accepted. I will retest on my side and approve.

melvin-bot[bot] 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? 💸

aimane-chnaif commented 1 year ago

After testing on all platforms, I noticed many issues.

https://github.com/Expensify/App/assets/96077027/7c7c20e8-47e2-4612-bbe5-b377c6d2972f

https://github.com/Expensify/App/assets/96077027/0a8a3df6-6728-4d98-8d80-688ba2250c60

This is known issue and that's why OptionsList is using platform-specific codes to dismiss keyboard when start dragging.

https://github.com/Expensify/App/assets/96077027/76d32911-03b3-42e5-bdde-cd6fee597616

We need another solution unless using the same approach.

On mSafari, cursor doesn't show even when focused and keyboard shown. This is known issue so can be out of scope.

https://github.com/Expensify/App/assets/96077027/a348d040-6741-4f4f-a286-876518627329

https://github.com/Expensify/App/assets/96077027/00eddfde-ab85-48f2-a3e5-09bc2972527f

aimane-chnaif commented 1 year ago

@sonialiap kindly bump on https://github.com/Expensify/App/issues/19156#issuecomment-1573237859

ahmedGaber93 commented 1 year ago

@aimane-chnaif

[mWeb] onScrollBeginDrag doesn't work This is known issue and that's why OptionsList is using platform-specific codes to dismiss keyboard when start dragging.

That's true, we can use keyboarddismissmode like OptionsList

// onScrollBeginDrag={Keyboard.dismiss}
keyboardDismissMode="on-drag" // this will support all platform

[iOS] auto-focus doesn't work when swipe between protected PDFs quickly

This because when you swipe quickly, swipe not complete to view the whole PDF. We can fix it by reduce itemVisiblePercentThreshold: 95 to 75 https://github.com/Expensify/App/blob/035783a7984e0214677b5364d9351c968887bf58/src/components/AttachmentCarousel/index.js#L53-L57

ahmedGaber93 commented 1 year ago

@aimane-chnaif

[iOS] keyboard glitch

This because keyboard not dismiss immediately when drag start, but after replace onScrollBeginDrag={Keyboard.dismiss} with keyboardDismissMode="on-drag", keyboard will dismiss immediately when swipe.

aimane-chnaif commented 1 year ago

yes, I know the root cause per each bug. Please try to find out solution to fix each one.

ahmedGaber93 commented 1 year ago

@aimane-chnaif Yes, I am doing that, I explain the both root cause and the solution. keyboardDismissMode="on-drag" will fix the glitch and support [mWeb] reduce itemVisiblePercentThreshold: 95 to 75 will fix quick swipe. And the other is out of scope, as you explain. And also I found re focus when navigates back to the PDF password form not work in ios safari, I fix this by adding delay before focus but it need more test, so I think we need increase bounty. (update: this issue not reproducible now, but I add solution for it if it repeated)

dukenv0307 commented 1 year ago

[iOS] auto-focus doesn't work when swipe between protected PDFs quickly

[mWeb] onScrollBeginDrag doesn't work

@aimane-chnaif hi, my proposal don't have the above issues, here's my branch in case you want to test it out.

[iOS] keyboard glitch

For this I think it's normal since we're blurring one input and focusing on another, also the glitch doesn't look too bad, from my branch the transition is smooth.

[mChrome] arrow buttons glitch

This is existing issue in main, same as the rest.