[$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 https://github.com/Expensify/App/pull/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
• [x] 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

https://github.com/Expensify/App/assets/78819774/59bc8698-7c3b-421a-a795-620d982f80a7

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[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[s-alves10]

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 https://github.com/Expensify/App/blob/52a18912a42567fdc25cb6c64676516959a16781/src/components/TextInput/BaseTextInput.js#L348

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 https://github.com/Expensify/App/blob/776ea5dd2587dce9796c0233a074113ffeb01938/src/components/TextInput/BaseTextInput.js#L352

            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

https://github.com/Expensify/App/assets/126839717/6e75ec7e-f0f2-485a-9ba1-4fcdf4e2abc7

What alternative solutions did you explore? (Optional)

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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.

[s-alves10]

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[bot]]

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

[s-alves10]

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

[s-alves10]

@sobitneupane

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

[s-alves10]

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.

[s-alves10]

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.

[s-alves10]

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

[s-alves10]

Proposal

Updated

Note: space issue doesn't exist here

[sobitneupane]

Thanks for the update @s-alves10. But we want issue to be solved on all platforms.

I think we should wait for better proposal.

[s-alves10]

I think we should wait for better proposal.

Yes. I'm doing a research

[rain2o]

I think this bug will need to go on hold for #22183 as this toggle no longer works at all.

However, I think this specific issue might already be resolved. I tested out the proposal submitted for that new bug, and the cursor no longer moves to the start of the field once that fix is applied. Once that bug is resolved we can re-test to be sure.

[s-alves10]

@rain2o

The solution for https://github.com/Expensify/App/issues/22183 cannot solve this issue.

[rain2o]

@rain2o

The solution for #22183 cannot solve this issue.

Sorry if I wasn't clear @s-alves10 . I was saying that this issue is already resolved, however, we can't currently verify because the eye toggle isn't working at all in current staging & main. Once that bug is resolved though, we can see that this one has also been resolved already. I tested current main with the fix from that bug and this bug is no longer present.

[s-alves10]

https://github.com/Expensify/App/issues/22183 is a different issue.

Now, the PR was merged into main already and you can test it. You can see this was not solved at all.

[rain2o]

22183 is a different issue.

Now, the PR was merged into main already and you can test it. You can see this was not solved at all.

Thanks @s-alves10 , I've re-tested and the bug is still present. Not sure what happened when I couldn't reproduce earlier.

[melvin-bot[bot]]

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

[laurenreidexpensify]

@sobitneupane do you agree with @rain2o that this should be held on #22183 ?

[sobitneupane]

@laurenreidexpensify Nope. Not required. PR associated with https://github.com/Expensify/App/issues/22183 is already on production.

We are waiting for proposals.

[ShogunFire]

For now I think there is a problem when we click on all pdfs

[s-alves10]

Proposal

Updated

[johnmlee101]

Unassigning in the meantime. Please request approval once you have a new proposal you'd like to go with @sobitneupane

[s-alves10]

@sobitneupane

Please check my updated proposal. They work on all platforms

[melvin-bot[bot]]

@sobitneupane @laurenreidexpensify 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!

[sobitneupane]

Thanks for your proposal @s-alves10

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

why?

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

why?

In root cause analysis section, you are expected to explain where the issue lies and what's the reason behind the issue. Is this issue existent only in our app or is it an issue from some upstream package?

Your solution tends to handle each issue separately for different platforms. You have three different solutions. Where are you proposing those changes? Is it possible to use single solution in all platforms?

[s-alves10]

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

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

This code is for preventing input component losing focus.

TextInput cursor goes to the end when toggle secureTextEntry in iOS

This is a feature(or bug) of TextInput component in iOS. I can't explain more

In root cause analysis section, you are expected to explain where the issue lies and what's the reason behind the issue. Is this issue existent only in our app or is it an issue from some upstream package?

Yes. the issue is from upstream package.

TextInput works differently on various platforms. We need to have a general solution and have specific solutions for some platforms, I think

[laurenreidexpensify]

Seeing if anyone from CallStack can take this on for us

[s-alves10]

@sobitneupane @laurenreidexpensify

I think it's reasonable that platform specific issues can be solved by platform-specific code. I verified those issue clearly and provided the solution. I'm not sure why this solution can't be accepted.

[pac-guerreiro]

Hi, I'm Pedro Guerreiro from Callstack - expert contributor group - and I would like to take a look at this issue!

[melvin-bot[bot]]

📣 @pac-guerreiro! 📣 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. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[sobitneupane]

@laurenreidexpensify

@pac-guerreiro from CallStack is interested in the issue. I am not sure what is the next step here.

[melvin-bot[bot]]

@sobitneupane @laurenreidexpensify 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!

[s-alves10]

@pac-guerreiro

Can you let me know your thoughts on my proposal?

[melvin-bot[bot]]

📣 @sobitneupane Please request via NewDot manual requests for the Reviewer role ($1000)

[laurenreidexpensify]

@pac-guerreiro can you please post a proposal.

@sobitneupane once we've 👍 that proposal can you do the normal 🎀 comment for our team to get an engineer from Expensify to sign off? Thanks!

[pac-guerreiro]

From my understanding, the root cause of this issue isn't on the app side.

A similar issue was also reported on a library which also uses React - - which leads me to believe that this may be something on React side.

I've been trying different solutions for this one and it seems that the best that can be done here is either a workaround like @s-alves10 proposed, but I believe we may not need to use platform specific code here, or just blur the input when the user presses the password visibility button.

I tried adding onMouseUp event handler to the password visibility button but it's didn't make any difference on web.

I'll let you know when I come up with something that works on all platforms!

[pac-guerreiro]

I just tried applying a solution in TextInput.js from Expensify/react-native-web and it seems to do the trick on web:

https://github.com/Expensify/App/assets/48553277/0b440455-b7fb-473b-86be-7503c6a6e9c6

The changes are the following:

This could be made more generic to apply the fix every time the input type is changed.

Let me know your thought on this @s-alves10 😄

[s-alves10]

@pac-guerreiro

I have some questions regarding the above code.

1. Do we need to compare prevSecureTextEntry and secureTextEntry?
2. Is this working on ios safari? We want to remove platform-specific code and this is important
3. What does this code mean? setSelection(hostRef.current, prevSelection.current)