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

fix(web)[TextInput]: cursor jumps to the start when secureTextEntry i… #21

Conversation

pac-guerreiro
Copy link

@pac-guerreiro pac-guerreiro commented Jul 28, 2023

This PR fixes Expensify/App#21727

Problem

On web, the cursor jumps to the start of the input when secureTextEntry is toggled.

Solution

Preserve and restore previous selection after secureTextEntry is toggled.

The proposal and demo are available in this comment: Expensify/App#21727 (comment)

Co-authored-by: Thiago Brezinski <thiagobrez@gmail.com>
@pac-guerreiro pac-guerreiro marked this pull request as ready for review July 31, 2023 11:06
Copy link

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pac-guerreiro In IOS/Safari, I am having following issue

Screen.Recording.2023-08-01.at.18.24.32.mov

@pac-guerreiro
Copy link
Author

pac-guerreiro commented Aug 1, 2023

Uploading Screen Recording 2023-08-01 at 18.46.43.mov…

@sobitneupane

You sure you're running my code? I just tested my changes on iOS/safari and they work 😅

@sobitneupane
Copy link

@pac-guerreiro Looks like the screen recording didn't get uploaded in previous message

Screenshot 2023-08-02 at 13 30 18

@pac-guerreiro
Copy link
Author

@sobitneupane

My bad 😅

Screen.Recording.2023-08-01.at.18.46.43.mov

@sobitneupane
Copy link

Thanks @pac-guerreiro.

We are good to merge this.

cc: @NikkiWines

@NikkiWines
Copy link

Looks like there are some formatting and unit test errors

@pac-guerreiro
Copy link
Author

@NikkiWines

My bad 😅 I just pushed fixes for those issues!

@NikkiWines
Copy link

No worries @pac-guerreiro - looks like there's still one failure

@pac-guerreiro
Copy link
Author

@NikkiWines second time's the charm 😅

Copy link

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@NikkiWines NikkiWines merged commit 04c6cbd into Expensify:master Aug 4, 2023
5 checks passed
@roryabraham
Copy link

Hello everyone 👋🏼 few PSA's here:

  • Generally we should not review or merge any PRs in any of our forked dependencies if there is not an equivalent PR in the upstream. Same goes for any PR that introduces a patch file
  • I think we are going to stop using this fork and move to patch files instead for all the same reasons we ditched the react-native fork. This repo is easier to build and publish, but still it's an extra step we don't want to deal with and we invested in an awesome new feature in patch-package that allows us to have multiple patch files for a single dependency applied in sequence.

So in conclusion, I think we need to:

  • create an issue in the upstream react-native-web repo with a minimal reproduction of the bug
  • create an upstream PR in the react-native-web repo for this change
  • Only once that's completed and we have an upstream PR to reference should we proceed with this code in a patch file in E/App instead of in this fork
  • To prevent any duplicate work, we should wait until [$3500] Upgrade to react-native-web v0.19.6 or greater App#16660 is complete before introducing the new patch including this code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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