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: setCaretTimeout checks #814

Merged
merged 1 commit into from
Feb 25, 2024
Merged

fix: setCaretTimeout checks #814

merged 1 commit into from
Feb 25, 2024

Conversation

csantos1113
Copy link
Contributor

Describe the change

Fixes #811

I'm opening this PR as an attempt to resolve jest + testing-library issue reported in #811.

I have an open question on that issue #811 (comment) where I wondered why el.selectionStart !== el.selectionEnd existed; but after a lot of thinking I cannot see a reason for it

My thinking

selectionStart and selectionEnd will always be the same when the caret is placed and nothing is selected:

Screen.Recording.2023-11-21.at.9.32.13.PM.mov

so el.selectionStart !== el.selectionEnd will only happen if the user have text selected:

Screen.Recording.2023-11-21.at.9.32.38.PM.mov

which differs from the intention described by the inline comment that is in the code:

    /* setting caret position within timeout of 0ms is required for mobile chrome,
    otherwise browser resets the caret position after we set it
    We are also setting it without timeout so that in normal browser we don't see the flickering */

But please let me know if I've missed something else on my analysis 🙏

@csantos1113 csantos1113 changed the title fix: setCaretTimeout check fix: setCaretTimeout checks Nov 22, 2023
@csantos1113
Copy link
Contributor Author

Hey @s-yadav - I'm gently pinging you for when you get a chance to review this PR. Thank you!

@csantos1113
Copy link
Contributor Author

@s-yadav 👋 when you get a chance, this would be much appreciated!

@s-yadav
Copy link
Owner

s-yadav commented Feb 25, 2024

Yes, the selectionStart !== selectionEnd mostly looks unintentional. the check with caretPos, is correct.
I will see if it can be accompanied by a test as this was mostly a side-effect of not having a test.

Update:
Added the test from your codesandbox. Thanks

@s-yadav s-yadav merged commit 4450eea into s-yadav:master Feb 25, 2024
s-yadav added a commit that referenced this pull request Feb 25, 2024
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.

suffix not working properly in jest + testing-library
2 participants