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

[$250] Android - Attachments - Typing is not smooth when revealing password in protected PDF #53394

Open
2 of 8 tasks
lanitochka17 opened this issue Dec 2, 2024 · 48 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Dec 2, 2024

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


Version Number: 9.0.69-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5284678&group_by=cases:section_id&group_order=asc&group_id=292107
Email or phone of affected tester (no customers): [email protected]*
Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the Expensify app
  2. Open any chat
  3. Tap on the "+" button and select "Add File"
  4. Select a PDF that is protected with a password
  5. Tap on "Enter the password"
  6. Tap on the eye icon to reveal the password
  7. Start typing the password in a fast manner
  8. Verify that each character is added smoothly

Expected Result:

When typing the PDF password after revealing it, each character should be added smotthly

Actual Result:

Typing is not smooth when typing PDF password after revealing it. When typing in a fast manner, not every character is added to password

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6680924_1733004068289.Pass_PDF.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021866173601902671628
  • Upwork Job ID: 1866173601902671628
  • Last Price Increase: 2025-01-06
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 2, 2024
Copy link

melvin-bot bot commented Dec 2, 2024

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Dec 6, 2024

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

@johncschuster
Copy link
Contributor

I couldn't action this today. I will check it out this weekend.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 6, 2024
@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Dec 9, 2024
@melvin-bot melvin-bot bot changed the title Android - Attachments - Typing is not smooth when revealing password in protected PDF [$250] Android - Attachments - Typing is not smooth when revealing password in protected PDF Dec 9, 2024
Copy link

melvin-bot bot commented Dec 9, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 9, 2024
Copy link

melvin-bot bot commented Dec 9, 2024

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

@rohit9625
Copy link

Proposal

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

Typing is not smooth when revealing password in protected PDF. On Android, the keyboard flickers when typing the password in the revealed state.

What is the root cause of that problem?

React Native’s TextInput automatically switches the keyboard type on Android based on the secureTextEntry prop. See this comment that shows the behavior for both IOS and Android when keyboardType=numeric.

We are using the getSecureEntryKeyboardType method to determine the keyboard here:

keyboardType={getSecureEntryKeyboardType(inputProps.keyboardType, inputProps.secureTextEntry ?? false, passwordHidden ?? false)}

This is the method responsible for determining keyboard type on Android devices:-
image

The keyboardType='visible-password' conflicts with the default behavior of switching keyboard type on Android. It results in the flickering keyboard that can be seen in this screencast below:-

Observe the emoji icon button and the mic button.

Issue_Repro.mp4

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

We should not use the native getSecureEntryKeyboardType method to determine keyboardType based on passwordHidden and secureTextEntry parameters on Android devices, instead we should use directly assign

keyboardType={inputProps.keyboardType}

It fixes the issue as seen in the screencast

Issue_Solution.mp4

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  1. We can add tests, if not already, for the native getSecureEntryKeyboardType method ensuring that it returns the passed keyboardType prop as it is, if prefer refactoring similar to the method in src/libs/getSecureEntryKeyboardType/index.ts file.

  2. Otherwise, just get rid of that index.android.ts file and write tests, if not already, for the method in src/libs/getSecureEntryKeyboardType/index.ts.

What alternative solutions did you explore? (Optional)

  1. Alternatively, we can also pass the inputMethod prop as text to the TextInput component in PDFPasswordForm.tsx
    Because the inputMethod prop has precedence over the keyboardType prop and hence will ignore the keyboardType=visible-password. See this ref

  2. However, I would suggest removing the usage of the native getSecureEntryKeyboardType method because this is getting called on each keystroke(verified by log statements). We already have the same method that directly returns the passed keyboardType prop which is being used for Web/Desktop/IOS/.

@jjcoffee
Copy link
Contributor

@rohit9625 Thanks for the proposal! What version of Android are you able to reproduce the issue on? For me on Android 14 (API 34) I don't see the issue.

Your solution would introduce a regression from the fix implemented in #9593, which was added to prevent the keyboard from visibly switching when toggling the password visibility (which you can see happens in your solution video).

@rohit9625
Copy link

What version of Android are you able to reproduce the issue on? For me on Android 14 (API 34) I don't see the issue.

I tested this issue on my Samsung A14 Device with Android 14(API 34).

Your solution would introduce a regression from the fix implemented in #9593, which was added to prevent the keyboard from visibly switching when toggling the password visibility (which you can see happens in your solution video).

I'm looking into it. How can I access that password input screen while login? For now, I'm being redirected to the Magic Code screen.

@jjcoffee
Copy link
Contributor

@rohit9625 Huh, strange that I can't reproduce it on the same API version. Does it happen for you in an emulator too?

How can I access that password input screen while login?

That no longer exists in the app, but the behaviour is the same in the PDF password input since it's the same base component.

@rohit9625
Copy link

@jjcoffee, I haven't tried it on an emulator yet. I'll try and let you know.

@rohit9625

This comment was marked as resolved.

@rohit9625
Copy link

Hey @jjcoffee, I tried running the app on the emulator as well with API 34 and still facing the same issue. See the screencast below:-

Screencast.from.2024-12-11.23-23-34.mp4

I think that toggling secureTextEntry automatically switches the keyboardType as I mentioned in my proposal. Currently, we are forcing keyboardType='visible-password' when the password is not hidden which is redundant. Please correct me if I'm wrong.

const getSecureEntryKeyboardType: GetSecureEntryKeyboardType = (keyboardType, secureTextEntry, passwordHidden) =>
secureTextEntry && !passwordHidden ? CONST.KEYBOARD_TYPE.VISIBLE_PASSWORD : keyboardType;

I guess the behavior in my solution video is obvious. The same behavior can be seen for Facebook Android app below:-

Screen_recording_20241211_232756.mp4

This is the screencast from the emulator after the fix I mentioned. The behavior in the screencast was the issue that this #9593 has solved.

Screencast.from.2024-12-11.23-46-45.mp4

I think we cannot prevent this keyboard switching because this is the library's issue. However, when secureTextEntry={true} the keyboard prevents autoCorrect and autoComplete but it allows that when secureTextEntry={false}.

@jjcoffee
Copy link
Contributor

Thanks for the extra testing @rohit9625! I think I'm not quite convinced by the RCA in your proposal, as the behaviour is that the keyboard is switching whilst typing, which doesn't seem to be explained by setting keyboardType='visible-password' (unless this is some sort of known bug).

@johncschuster Do you think it's an acceptable fix if the keyboard changes once you switch between visible/invisible modes? It's technically a regression from #9593, but it's visually less disturbing than the keyboard changing whilst you type, I think.

Copy link

melvin-bot bot commented Dec 16, 2024

@johncschuster @jjcoffee 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!

@melvin-bot melvin-bot bot added the Overdue label Dec 16, 2024
Copy link

melvin-bot bot commented Dec 16, 2024

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

@jjcoffee
Copy link
Contributor

@johncschuster Friendly bump on this question 🙏

@melvin-bot melvin-bot bot removed the Overdue label Dec 16, 2024
@melvin-bot melvin-bot bot removed the Overdue label Dec 27, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

@rafecolton @johncschuster @jjcoffee this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2024
@jjcoffee
Copy link
Contributor

Not overdue, just waiting for @rafecolton to return from OOO.

@melvin-bot melvin-bot bot removed the Overdue label Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

Copy link

melvin-bot bot commented Jan 3, 2025

@rafecolton, @johncschuster, @jjcoffee Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Jan 3, 2025
@jjcoffee
Copy link
Contributor

jjcoffee commented Jan 6, 2025

Friendly bump @rafecolton 🙇

@melvin-bot melvin-bot bot removed the Overdue label Jan 6, 2025
Copy link

melvin-bot bot commented Jan 6, 2025

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

@rafecolton
Copy link
Member

Thanks for the bump. My understanding is that @rohit9625's proposal will remove the flickering but that the keyboard will change when you click the eye. And that we have agreed this is the preferred outcome even though it is technically a "regression." Is that right? If so, I will assign @rohit9625 😊

@melvin-bot melvin-bot bot added the Overdue label Jan 8, 2025
@rafecolton
Copy link
Member

Not overdue melvin, I just commented two hours ago 😄

@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 2025
@jjcoffee
Copy link
Contributor

jjcoffee commented Jan 9, 2025

@rafecolton That's right!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 9, 2025
Copy link

melvin-bot bot commented Jan 9, 2025

📣 @rohit9625 You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@rohit9625
Copy link

Should I write the same proposal for applying to the Upwork job?
@rafecolton

@rafecolton
Copy link
Member

I don't known. @johncschuster?

@rohit9625
Copy link

I've applied for the Upwork job and you can expect the PR on 10th Jan 2025.

@rohit9625
Copy link

I didn't received a offer at upwork yet. Should I wait or proceed with PR?

@rafecolton
Copy link
Member

bump @johncschuster

@jjcoffee
Copy link
Contributor

@rohit9625 Please proceed with the PR. Being assigned here guarantees that you've got the job - there is just sometimes a delay before you get hired on Upwork.

@rohit9625

This comment was marked as resolved.

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests

7 participants