search

s-yadav / react-number-format

React component to format numbers in an input or as a text.
MIT License
3.87k stars 409 forks source link

fix: `setCaretTimeout` checks #814

Closed csantos1113 closed 8 months ago

csantos1113 commented 11 months ago

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 https://github.com/s-yadav/react-number-format/issues/811#issuecomment-1819527005 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:

https://github.com/s-yadav/react-number-format/assets/9648559/fb3c2791-9e43-40c8-b358-91d4a2cb85ac

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

https://github.com/s-yadav/react-number-format/assets/9648559/9ff9066c-de30-4c83-ba8f-a1b2bd6fb690

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 commented 11 months ago

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

csantos1113 commented 8 months ago

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

s-yadav commented 8 months ago

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