App icon indicating copy to clipboard operation
App copied to clipboard

Android - Attachments - Typing is not smooth when revealing password in protected PDF

Open lanitochka17 opened this issue 1 year ago • 3 comments

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?

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/e3809f66-d637-454a-a280-74b66f08b6bc

View all open jobs on GitHub

lanitochka17 avatar Dec 02 '24 16:12 lanitochka17

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.

melvin-bot[bot] avatar Dec 02 '24 16:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 06 '24 09:12 melvin-bot[bot]

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

johncschuster avatar Dec 06 '24 23:12 johncschuster

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

melvin-bot[bot] avatar Dec 09 '24 17:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 09 '24 17:12 melvin-bot[bot]

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: https://github.com/Expensify/App/blob/f481d64b7e894b6a5a98dcaec0e48876a42e60f9/src/components/TextInput/BaseTextInput/index.native.tsx#L370

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.

https://github.com/user-attachments/assets/4b24a7bd-9e3a-4c91-abc2-937e0e53ea01

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

https://github.com/user-attachments/assets/539d31ed-add4-4c6a-a771-109255cc0b7d

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/.

rohit9625 avatar Dec 10 '24 16:12 rohit9625

@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 https://github.com/Expensify/App/pull/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).

jjcoffee avatar Dec 11 '24 09:12 jjcoffee

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.

rohit9625 avatar Dec 11 '24 13:12 rohit9625

@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.

jjcoffee avatar Dec 11 '24 13:12 jjcoffee

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

rohit9625 avatar Dec 11 '24 13:12 rohit9625

I don't know why but the build failed when I pulled from the main branch recently. Do you have any idea about this failing task? I was building the app directly using the command ./gradlew buildDevelopmentDebug.

Task :react-native-key-command:generateCodegenSchemaFromJavaScript No modules to process in combine-js-to-schema-cli. If this is unexpected, please check if you set up your NativeComponent correctly. See combine-js-to-schema.js for how codegen finds modules.

Task :react-native:packages:react-native:ReactAndroid:hermes-engine:downloadHermes Download https://github.com/facebook/hermes/tarball/hermes-2024-08-15-RNv0.75.1-4b3bf912cc0f705b51b71ce1a5b8bd79b93a451b

Task :app:generateAutolinkingPackageList FAILED

Task :react-native:packages:react-native:ReactAndroid:downloadBoost UP-TO-DATE Download https://archives.boost.io/release/1.83.0/source/boost_1_83_0.tar.gz

Task :react-native:packages:react-native:ReactAndroid:downloadDoubleConversion UP-TO-DATE Download https://github.com/google/double-conversion/archive/v1.1.6.tar.gz

Task :react-native:packages:react-native:ReactAndroid:hermes-engine:downloadHermes UP-TO-DATE

Task :react-native:packages:react-native:ReactAndroid:downloadFmt UP-TO-DATE Download https://github.com/fmtlib/fmt/archive/9.1.0.tar.gz

Task :react-native:packages:react-native:ReactAndroid:downloadFolly UP-TO-DATE Download https://github.com/facebook/folly/archive/v2024.01.01.00.tar.gz

Task :react-native:packages:react-native:ReactAndroid:downloadGlog UP-TO-DATE Download https://github.com/google/glog/archive/v0.3.5.tar.gz

Task :react-native:packages:react-native:ReactAndroid:downloadGtest UP-TO-DATE Download https://github.com/google/googletest/archive/refs/tags/release-1.12.1.tar.gz

FAILURE: Build failed with an exception.

  • What went wrong: Execution failed for task ':app:generateAutolinkingPackageList'.

RNGP - Autolinking: Could not find project.android.packageName in react-native config output! Could not autolink packages without this field.

  • Try:

Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights. Get more help at https://help.gradle.org.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.8/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD FAILED in 1m 3s 262 actionable tasks: 157 executed, 105 up-to-date

rohit9625 avatar Dec 11 '24 16:12 rohit9625

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

https://github.com/user-attachments/assets/3ff2878b-c7e6-4942-9b68-56cbe5843190

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. https://github.com/Expensify/App/blob/20b4c7db47969f7d2209f1c0b8fd765ce883f92c/src/libs/getSecureEntryKeyboardType/index.android.ts#L8-L9

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

https://github.com/user-attachments/assets/0b912d33-1db0-4ad6-ad01-eba790916260

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.

https://github.com/user-attachments/assets/520a4eed-430c-41a9-b09e-47a43de55702

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}.

rohit9625 avatar Dec 11 '24 18:12 rohit9625

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 https://github.com/Expensify/App/pull/9593, but it's visually less disturbing than the keyboard changing whilst you type, I think.

jjcoffee avatar Dec 12 '24 15:12 jjcoffee

@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[bot] avatar Dec 16 '24 09:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 16 '24 09:12 melvin-bot[bot]

@johncschuster Friendly bump on this question :pray:

jjcoffee avatar Dec 16 '24 10:12 jjcoffee

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

melvin-bot[bot] avatar Dec 16 '24 16:12 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-12-18 16:28:23 UTC.

Proposal

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

Android - Attachments - Typing is not smooth when revealing password in a protected PDF

What is the root cause of that problem?

When the password is visible, we set secureTextEntry to false and keyboardType to visible-password. When the password is hidden, we set secureTextEntry to true and keyboardType to undefined.

https://github.com/Expensify/App/blob/4b0f250c71d4cc214107ff84797b24365964a5d0/src/components/TextInput/BaseTextInput/index.native.tsx#L367-L370

The issue lies in the native setSecureTextEntry method here

@ReactProp(name = "secureTextEntry", defaultBoolean = false)
public void setSecureTextEntry(ReactEditText view, boolean password) {
  updateStagedInputTypeFlag(
      view,
      password
          ? InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD
          : InputType.TYPE_NUMBER_VARIATION_PASSWORD | InputType.TYPE_TEXT_VARIATION_PASSWORD,
      password ? InputType.TYPE_TEXT_VARIATION_PASSWORD : 0);
  checkPasswordType(view);
}

When secureTextEntry is set to true, it sets the input type to TYPE_TEXT_VARIATION_PASSWORD flag and removes TYPE_TEXT_VARIATION_VISIBLE_PASSWORD.

When secureTextEntry is set to false, it does not set a new input type to anything but removes TYPE_NUMBER_VARIATION_PASSWORD and TYPE_TEXT_VARIATION_PASSWORD. Because it didn't set a new type, it defaults to the default (which is not visible-password that we expect).

Note: On every render the order in which these methods are called is reversed (I think it's by design to avoid relying on the order):

setSecureTextEntry(false)
setKeyboardType(visible-password)

setKeyboardType(visible-password)
setSecureTextEntry(false)

setSecureTextEntry(false)
setKeyboardType(visible-password)

setKeyboardType(visible-password)
setSecureTextEntry(false)

When setSecureTextEntry(false) is called second, it removes TYPE_TEXT_VARIATION_PASSWORD (as described above) that was added by setKeyboardType here.

This causes the keyboard to switch to a different one on every keystroke: visible-password -> default -> visible-password -> default.

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

Patch this line to set the input type back to TYPE_TEXT_VARIATION_VISIBLE_PASSWORD, and possibly make an upstream PR

 @ReactProp(name = "secureTextEntry", defaultBoolean = false)
 public void setSecureTextEntry(ReactEditText view, boolean password) {
   updateStagedInputTypeFlag(
       view,
       password
           ? InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD
           : InputType.TYPE_NUMBER_VARIATION_PASSWORD | InputType.TYPE_TEXT_VARIATION_PASSWORD,
-      password ? InputType.TYPE_TEXT_VARIATION_PASSWORD : 0);
+      password
+          ? InputType.TYPE_TEXT_VARIATION_PASSWORD
+          : (view.getStagedInputType() & InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD) == InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD
+            ? InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD
+            : 0);
   checkPasswordType(view);
 }

Note: this is not thoroughly tested so it would be good to hear the RN team's opinion first.

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

Our tests won't be able to catch this as they only run against the web platform and this is an Android issue.

andriivitiv avatar Dec 17 '24 22:12 andriivitiv

Thanks for the extra testing @rohit9625! I think I'm not quite convinced by the RCA in your https://github.com/Expensify/App/issues/53394#issuecomment-2532202683, 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 https://github.com/Expensify/App/pull/9593, but it's visually less disturbing than the keyboard changing whilst you type, I think.

@jjcoffee sorry I missed your message above! I'm generally not a fan of regressing, but before I weigh in, I think I need a bit more clarity to make sure I'm following along.

Can you help me understand how the keyboard would change once we switch between visible/invisible modes? Is it a visual change, or is this simply switching the keyboard to secureTextEntry={true}, which prevents autoCorrect and autoComplete?

johncschuster avatar Dec 18 '24 21:12 johncschuster

Hey @johncschuster, please refer to this comment to see the visual changes in the keyboard on Emulator, before and after the fix. The keyboard might be flickering because we simultaneously have secureTextEntry={true} and keyboardType="visible-password" in our code. But, setting keyboardType="visible-password" supersedes that secureTextEntry={false}, according to the library here:-

  @ReactProp(name = "keyboardType")
  public void setKeyboardType(ReactEditText view, @Nullable String keyboardType) {
    int flagsToSet = InputType.TYPE_CLASS_TEXT;
    if (KEYBOARD_TYPE_NUMERIC.equalsIgnoreCase(keyboardType)) {
      flagsToSet = INPUT_TYPE_KEYBOARD_NUMBERED;
    } else if (KEYBOARD_TYPE_NUMBER_PAD.equalsIgnoreCase(keyboardType)) {
      flagsToSet = INPUT_TYPE_KEYBOARD_NUMBER_PAD;
    } else if (KEYBOARD_TYPE_DECIMAL_PAD.equalsIgnoreCase(keyboardType)) {
      flagsToSet = INPUT_TYPE_KEYBOARD_DECIMAL_PAD;
    } else if (KEYBOARD_TYPE_EMAIL_ADDRESS.equalsIgnoreCase(keyboardType)) {
      flagsToSet = InputType.TYPE_TEXT_VARIATION_EMAIL_ADDRESS | InputType.TYPE_CLASS_TEXT;

      // Set cursor's visibility to False to fix a crash on some Xiaomi devices with Android Q. This
      // crash happens when focusing on a email EditText, during which a prompt will be triggered
      // but
      // the system fail to locate it properly. Here is an example post discussing about this
      // issue: https://github.com/facebook/react-native/issues/27204
      if (shouldHideCursorForEmailTextInput()) {
        view.setCursorVisible(false);
      }
    } else if (KEYBOARD_TYPE_PHONE_PAD.equalsIgnoreCase(keyboardType)) {
      flagsToSet = InputType.TYPE_CLASS_PHONE;
    } else if (KEYBOARD_TYPE_VISIBLE_PASSWORD.equalsIgnoreCase(keyboardType)) {
      // This will supersede secureTextEntry={false}. If it doesn't, due to the way
      //  the flags work out, the underlying field will end up a URI-type field.
      flagsToSet = InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD;
    } else if (KEYBOARD_TYPE_URI.equalsIgnoreCase(keyboardType)) {
      flagsToSet = InputType.TYPE_TEXT_VARIATION_URI;
    }

    updateStagedInputTypeFlag(view, InputType.TYPE_MASK_CLASS, flagsToSet);
    checkPasswordType(view);
  }

Hence, results in conflict: secureTextEntry=false by setting keyboardType="visible-password" and secureTextEntry=true by passing this value as true from our App's code.

rohit9625 avatar Dec 19 '24 07:12 rohit9625

But, setting keyboardType="visible-password" supersedes that secureTextEntry={false}, according to the library here:-

This is true, but it depends on the order in which these values are set. It will only supersede if setSecureTextEntry was called before setKeyboardType, not the other way around. My proposal suggests a fix to setSecureTextEntry to account for cases when it is called after setKeyboardType.

andriivitiv avatar Dec 19 '24 09:12 andriivitiv

Can you help me understand how the keyboard would change once we switch between visible/invisible modes? Is it a visual change, or is this simply switching the keyboard to secureTextEntry={true}, which prevents autoCorrect and autoComplete?

@johncschuster Just to add in text form, yes this is purely a visual change. Namely the emoji picker on the keyboard becomes visible as well as the number bar at the top of the keyboard being replaced by the auto-correct/complete suggestions.

jjcoffee avatar Dec 19 '24 10:12 jjcoffee

@CyberAndrii Thanks for your proposal! Normally I think fixing upstream would be the best way forward, but I'm unsure if this is something they'd actually want to support (since us setting the keyboardType directly is a bit of a workaround anyway).

I think it makes sense to go forwards with @rohit9625's proposal, with the acceptance that the keyboard will switch when tapping the visibility icon (this is normal behaviour in other apps as pointed out here). Open to discussion though, if the assigned engineer disagrees!

:ribbon::eyes::ribbon: C+ reviewed

jjcoffee avatar Dec 19 '24 10:12 jjcoffee

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

melvin-bot[bot] avatar Dec 19 '24 10:12 melvin-bot[bot]

I'm not sure that When user tapped on the eye icon in password field on Android, the keyboard changes was actually a bug, it sounds like a feature to me. As a user, I see this kind of thing when I tap into an email field, for example, so I think it's a pattern that will be understood by users. A flickering or unresponsive keyboard seems like a much bigger problem.

@tgolen I'm curious what you think, since you reviewed https://github.com/Expensify/App/pull/9593.

rafecolton avatar Dec 19 '24 22:12 rafecolton

I think the keyboard switching sounds fine.

tgolen avatar Dec 20 '24 17:12 tgolen

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

melvin-bot[bot] avatar Dec 23 '24 09:12 melvin-bot[bot]

Not overdue, @rafecolton is OOO, so we'll get back to this next year I guess :partying_face:

jjcoffee avatar Dec 23 '24 09:12 jjcoffee

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

melvin-bot[bot] avatar Dec 23 '24 16:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 27 '24 09:12 melvin-bot[bot]