[$1000] Chat - Tapping eye icon for protected pdf file moves cursor to the start of the field

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR #21367

Action Performed:

1. Login to NewDot
2. Open chat
3. Tap password protected pdf
4. Tap enter password
5. Type several characters
6. Tap Eye icon

Expected Result:

Cursor should stay where it was

Actual Result:

Cursor moves to the start fo the field

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.33.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6108545_video_33__1_.mp4

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/~01f00fa406bef36c56
• Upwork Job ID: 1674042523612561408
• Last Price Increase: 2023-06-28

Issue Owner

Current Issue Owner: @laurenreidexpensify

[melvin-bot]

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

[melvin-bot]

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

[dummy-1111]

Proposal

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

Tapping eye icon for protected pdf file moves cursor to the start

What is the root cause of that problem?

Please check the code below

App/src/components/TextInput/BaseTextInput.js

Line 348 in 52a1891

onMouseDown={(e) => e.preventDefault()}

I'm not sure what does this code do but preventing default action on mousedown event makes TextInput works weird.

In addition, TextInput cursor goes to the end when toggle secureTextEntry in iOS native as you can see below

TextInput cursor goes to the beginning when toggle secureTextEntry in iOS safari.

These are the root cause of the issue.

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

Prevent default action for mouseup event. Add onMouseUp props to the Checkbox and add the following code here

App/src/components/TextInput/BaseTextInput.js

Line 352 in 776ea5d

onMouseDown={(e) => e.preventDefault()}

            onMouseUp={(e) => e.preventDefault()}

In iOS safari and iOS native, we need to control selection manually.

Add the selection state

    const [selection, setSelection] = useState(props.selection);

For iOS safari, get selection from the input element when toggle

    const togglePasswordVisibility = useCallback(() => {
        if (getPlatform() === CONST.PLATFORM.WEB) {
            setSelection({start: input.current.selectionStart, end: input.current.selectionEnd});
        }
        setPasswordHidden((prevState) => !prevState);
    }, []);

For iOS native, add onSelectionChange to get selection, and set selection manually when secureTextEntry toggles because control selection doesn't work well.

    useEffect(() => {
        if (getPlatform() === CONST.PLATFORM.IOS) {
            input.current.setNativeProps({selection});
        }
    }, [passwordHidden])

    onSelectionChange={getPlatform() === CONST.PLATFORM.IOS ? (e) => setSelection(e.nativeEvent.selection) : undefined}

Result

21727_mac_chrome.mp4

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[rain2o]

I think an important question is, what is the expected behavior? As @s-alves10 mentioned in his proposal, we have onMouseDown={(e) => e.preventDefault()} on the checkbox surrounding the eye icon. This prevents the checkbox from gaining focus when we click on it and keeps focus on the text input.

The solution he proposed "fixes" it, in that it allows the checkbox to gain focus when you click it. However, this could cause unexpected behavior for users, if they click it, then continue to type because focus will no longer be in the textbox. But, from an accessibility standpoint, I think it does make sense for it to behave this way, otherwise we'd have different behavior for click vs tab+space.

The decision on how this is expected to behave could determine if a different change is required or not.

[dummy-1111]

Please check the alternative as well

[laurenreidexpensify]

@sobitneupane please review and confirm if any of the proposals work. thanks!

[sobitneupane]

Alternative solution from @s-alves10's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

Turn out that the solution won't work for IOS safari.

[melvin-bot]

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

[dummy-1111]

@sobitneupane
Here are my thoughts.
I think removing the onMouseDown and onMouseUp would be a better solution. Preventing default action may raise other issues. For instance, the solution doesn't work on ios safari.

Proposal

Updated

[sobitneupane]

@s-alves10 If we remove onMouseDown, then the input will lose focus which is not expected. If the proposal won't work for IOS/safari, then we will wait for better proposal which works on all devices and have the text input focused.

Will your updated proposal yield the expected output?

[dummy-1111]

@sobitneupane

That's why I added the below code segment in the proposal

[dummy-1111]

Will your updated proposal yield the expected output?

Yes. the proposal works on all platforms.
In ios native, when the cursor is in the middle of the text, cursor moves to the last position. But this is its original feature, not a bug.

[rain2o]

@sobitneupane

That's why I added the below code segment in the proposal

This would however break a11y behavior. I don't know how much focus we put into that for this app.
When using tab to focus on the checkbox, and usingspace to toggle it, with the above changes the toggle does trigger the change of visibility, but when focus is put back into the input, it also adds a space to the end of the current value. Additionally, we shouldn't change the current focus as a side-effect of pressing something for accessibility, it would cause unexpected behavior and could cause the user's password to be read by a screen reader if they accidentally hit the eye checkbox.

[dummy-1111]

passwordHidden state won't be changed unless user clicks the eye button.

[rain2o]

passwordHidden state won't be changed unless user clicks the eye button.

If a user tabs focus to the eye checkbox and hits space, it also triggers the behavior (as expected) to toggle passwordHidden. This is necessary for a11y.

[dummy-1111]

@rain2o
I don't think your concern isn't a problem for my solution

[sobitneupane]

@s-alves10 You might have to update your proposal. There are some recent changes(migration from class -> functional component) in BaseTextInput.

but when focus is put back into the input, it also adds a space to the end of the current value

Also please do consider above statements from @rain2o. We would definitely not want to add an extra space to the input.

[pac-guerreiro]

The original issue should be resolved now, but the iOS fix is still pending -> facebook/react-native#38678

Also, I think it would be a good idea to merge the react-native-web fix to upstream 😄

[NikkiWines]

@pac-guerreiro, sounds good - can you make a PR to update the @expensify/react-native-web dependency here?

[lanitochka17]

Issue reproducible in Build 1.3.52-3.
Windows 10/Chrome

Bug6159478_bandicam_2023-08-09_18-13-44-267.mp4

[roryabraham]

Thanks for your work on this issue so far everyone, and thanks @pac-guerreiro for creating an upstream PR for the react-native portion of this. Can you also create an upstream issue and PR for the react-native-web portion as well?

Posted more detailed thoughts here.

@pac-guerreiro, sounds good - can you make a PR to update the @expensify/react-native-web dependency here?

@NikkiWines normally we have to manually publish the react-native-web fork, but as stated in here we're moving away from forks and to https://github.com/ds300/patch-package because it's less internal work to manage.

Thanks everyone!

[dummy-1111]

Is this issue waiting for proposals?

[pac-guerreiro]

@roryabraham

Thank you for the input!

I'm opening a PR to react-native-web with my fix.

Meanwhile, if there was no bump to the current version of Expensify/react-native-web in the last app release, then the fix will not be present.

Please, confirm this @lanitochka17 and retest it after the version is bumped to see if the fix worked.

@s-alves10 regarding the iOS fix, it's still waiting for approval on react-native repo 😄

[pac-guerreiro]

Sorry for the delay, I had a two week vacation.

I just opened a PR for necolas/react-native-web with my fix.

The PR with the iOS fix is still pending review.

[laurenreidexpensify]

@pac-guerreiro how is the PR going?

[laurenreidexpensify]

@pac-guerreiro is there a PR up for this still?

[pac-guerreiro]

@laurenreidexpensify

Sorry for the delay, the PR for react-native-web was closed but my fix was added manually by necolas - necolas/react-native-web@5e41208 - and it will be available in react-native-web@0.19.9

Regarding the react-native PR, there are no updates from Meta side. I guess I'll need to mention more reviewers.

[laurenreidexpensify]

What are the next steps on this one?

[laurenreidexpensify]

@pac-guerreiro bump

[laurenreidexpensify]

@pac-guerreiro @sobitneupane is there any latest here?

[pac-guerreiro]

@laurenreidexpensify I'm still waiting on updates from React Native PR

[pac-guerreiro]

Still no updates on React Native PR

[pac-guerreiro]

Still no updates on React Native PR

[pac-guerreiro]

When the app starts using RN 0.73.1, I'll check if the new version fixes the underlying issue 😄

[laurenreidexpensify]

Still holding on #31381

[laurenreidexpensify]

@pac-guerreiro looks like #31381 is live - we have Upgraded react-native to 0.73+ so can we pick this one up again?

[pac-guerreiro]

@laurenreidexpensify Thanks for the heads up! I'll test this asap!

[pac-guerreiro]

Screen.Recording.2024-02-09.at.19.07.42.mp4

@laurenreidexpensify The issue still persists on iOS. I reported this on facebook/react-native#38676

Meanwhile, I could apply this fix through a patch if you want, otherwise we'll have to wait on meta team response.

[laurenreidexpensify]

@pac-guerreiro have Meta responded yet?

[pac-guerreiro]

@laurenreidexpensify Nothing so far but I just bumped them 😄

[laurenreidexpensify]

I am going to close this. It feels like an edge case, and we can reopen if this is ever reported as an issue by live customers.