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

Cursor doesn't keep up with typing on some Samsung Android phones #75

Closed
richardhills opened this issue Sep 13, 2017 · 38 comments
Closed

Comments

@richardhills
Copy link

This reproducible on my Huawei running Android 6.0, and on Samsung S8+ phones. It happens with and without type="tel" in the <Phone> declaration, so I don't think it's related to this issue.

Set the country to "UK", and type in "077915". The input will contain "07791 5" (with a space) and the cursor will be positioned before the 5. Typing "67890" after this results in "07791 678905" instead of "07791 567890".

It appears that on these devices that doing a complete onChange -> setState -> render cycle results in the cursor being positioned incorrectly. Calling setState in a setTimeout callback seems to fix it.

Here's a pull request that seems to resolve the issue on my device: #74

@catamphetamine
Copy link
Owner

catamphetamine commented Sep 13, 2017

Did you install the latest version by the way.
You could have an ancient version with type="tel" being the default.

@catamphetamine
Copy link
Owner

You can check that in the browser: if you open DevTools and select the <input/> it should not have type="tel".
Then you can test it on the phone.

@Gedzis
Copy link

Gedzis commented Sep 13, 2017

I have same issue with latest version and Samsung device.
If I type LT phone "+370 6|" cursor is moved before 6, so it looks like "+370 |6"

@catamphetamine
Copy link
Owner

@Gedzis is type="tel" absent from the <input/> tag on your webpage?

@catamphetamine
Copy link
Owner

The pull request relies on setTimeout which is magic and I'm not willing to magically fix things preferring durable and clear solutions instead.
The UK use case is too simple to verify the workaround too because it's the simplest one and is most likely a false positive:

Use case:

Select UK country

0|
07|
077|
0779|
07791|
07791 |5
07791 6|5
07791 67|5
07791 678|5
07791 6789|5
07791 67890|5

So, from the use case it seems like the same issue as this one:
catamphetamine/input-format#2
It seems like the caret position isn't controllable at all.

When a timeout is introduced the <input/> value goes out of sync and React just resets the caret positioning it in the end of the <input/> value.
This results in falsy confidence of the bug being "fixed".
Try that trick on the complex ones, having lots of braces and spaces in between, inserting digits somewhere in the middle.

@Gedzis
Copy link

Gedzis commented Sep 13, 2017

@catamphetamine I agree that setting timeout isn't best solution.
Issue that I am having: https://ibb.co/cNtbWv

@richardhills
Copy link
Author

Hi @catamphetamine , yes it's reproducible on the latest version. It happens with and without type="tel" in the <Phone> declaration.

I'm just trying to identify exactly when it goes wrong in the onChange -> render cycle.

@catamphetamine
Copy link
Owner

If you want my directions then provided this simulation it's the caret positioning code:

0|
07|
077|
0779|
07791|
07791 |5
07791 6|5
07791 67|5
07791 678|5
07791 6789|5
07791 67890|5

i.e. the caret always moves 1 step forward and is uncontrollable through input-format's setCaretPosition() for some reason.
ppl said "Google's keyboard" worked whatever that means.

@richardhills
Copy link
Author

Hi @catamphetamine, I've realised it's more complex. I get:

0|
07|
077|
0779|
07791|
07791 |5
0779165|
07791 65|7
07791 658|7
07791 6589|7
07791 65890|7

I've stepped through the code, and setCaretPosition() is called correctly. At some point after render() is called, the caret is placed in the incorrect position.

@richardhills
Copy link
Author

Also, the cursor is updated correctly by setCaretPosition(), but it reverts to the incorrect place.

@catamphetamine
Copy link
Owner

My feeling is like on Androids the keyboard is stubborn and forces itself through any programmatic cursor positioning.
Not sure if something like setCaretPosition(input, 123) would work when hooked to a button tap or executed in a timeout.
If it does, then a manual input.addEventListener('change', () => setCaretPosition(...)) could be tried, i.e. without React at all, to see if it allows messing with caret position at all inside the 'change' event handler.
Guesses all around.

@catamphetamine
Copy link
Owner

And if it was React resetting the caret position then AFAIK it always resets to the end, which is not the case in your case.

@richardhills
Copy link
Author

Calling setCaretPosition(input, 123) works as expected. The keyboard appears to respect the cursor position, but it's being reverted afterwards. I'm not sure why react would necessarily reset it to the end, but I'll continue investigating.

@catamphetamine
Copy link
Owner

catamphetamine commented Sep 14, 2017

So, your point is that the cursor position only resets in the React case and that in a simple event hook setCaretPosition() results aren't reverted?
React is known to reset caret position to the end when input.value changes externally (which is what this library does). It's the reason input-format manually does input.value = xxx before calling onChange(xxx) – it's kind of a workaround to prevent React from resetting the caret (moving it to the end).

@richardhills
Copy link
Author

@catamphetamine , that's right. I'm trying to breakpoint in places where react might update the carot position with no luck.

@catamphetamine
Copy link
Owner

Yeah, that's a tricky one.
I imagine it could take up to infinity time to debug this one so make sure your head doesn't blow ha ha.

@shivgarg5676
Copy link

Is there any update on this bug. I am facing same problem on ios as well

@catamphetamine
Copy link
Owner

@shivgarg5676 This bug is for Android only.
If you're having a bug on iOS then open a separate issue and post reproduction instructions.

@shivgarg5676
Copy link

Yes, this is on android only and that too some limited devices. But are we able to address this

@catamphetamine
Copy link
Owner

@shivgarg5676 My guess is that this bug is unbeatable.

@shivgarg5676
Copy link

Is there any way we can disable this spacing and formatting feature. This will automatically disable this bug.

@catamphetamine
Copy link
Owner

@shivgarg5676 Use libphonenumber-js parse() if you don't want the formatting feature.

@jcollum
Copy link

jcollum commented Mar 8, 2018

We're seeing it too. It happens on Samsung 7.0 devices but does not happen on:

  • LG 7.0 device

  • AVD emulator 5.0 device

  • Chrome emulating Galaxy S5

@catamphetamine
Copy link
Owner

@jcollum I know on iPhones there's a debugging feature: one can hook up an iPhone to a macOS machine and then observe any console.log()s or even use the DevTools debugger.
Is there something like that on Android phones?

@jcollum
Copy link

jcollum commented Mar 8, 2018

Yes, pretty sure you can do that with Chrome debugging tools: chrome://inspect/#devices. I'll post more here when I get a real Samsung device to test with.

@catamphetamine
Copy link
Owner

For anyone interested, copy-pasting from the PR: #74 (comment)

If anyone wants they can send a phone to my US mailbox this week (a window of opportunity) so that I could take a look at the issue when the package arrives from US to Russia (I'm purchasing something this week).

@JsGvDev
Copy link

JsGvDev commented Apr 18, 2018

Hi @catamphetamine , yes you can debug on physical mobile phone on android. You can use chrome, firefox, even with safari. After you give at your phone developer permissions and you can set up debugging with usb, you can use that features.

Firefox --> Firefox WebIDE
Chrome --> chrome://inspect/#devices

Even if you can use adb or android studio for that purpose and into console view get different logs.

Only I leave this message because I saw your before answer. I didn't try yet this component, but I was reading different threads about this issues. I don't have to much time time but if possible I'd try to see this issue for myself.

But for my experience a delay something, it's not usually good solution. Because of ent lop work in javascript

https://developer.mozilla.org/en-US/docs/Web/JavaScript/EventLoop

If you need to do something after setstate is called, or you use callback param or lifecycle into react components like https://reactjs.org/docs/react-component.html#componentdidupdate to do something with DOM.

Thanks ;)

@brianjoseff
Copy link

Maybe if there's an elegant way to remove AsYouType as a dependency from input-control.js. I think the AsYouType formatter is the source of these issues (well Samsung, et al is...but anyway.)

Create like an "autoFormat" prop, and if false, then that flag is carried into input-control.js and it only passes the parsed input into the formatter if autoFormat flag is true (default)

@catamphetamine
Copy link
Owner

@brianjoseff

I think the AsYouType formatter is the source of these issues (well Samsung, et al is...but anyway.)

No, it is not.
I already told here that the issue is inside input-format
catamphetamine/input-format#2
So one can as well debug input-format directly instead of debugging this library.

input-format (<ReactInput/>) could be made into an optional thing controlled via a property, something like smartCaret={false}: if smartCaret is false then a simple <input {...rest} value={formatPhoneNumber(value)} onChange={...}/> is rendered instead of <ReactInput/> inside source/Input.js's .render().

I guess this can be implemented without any difficulties.
I'll give it a try.

@catamphetamine
Copy link
Owner

catamphetamine commented Apr 19, 2018

So, I implemented the feature.
See the linked commit for reference.

react-phone-number-input@latest

Usage:

import Phone from 'react-phone-number-input'

<Phone
  smartCaret={false}
  value={this.state.value}
  onChange={value => this.setState(value)}/>

smartCaret={false} caret is not as "smart" as the default one but still works good enough.
When erasing or inserting digits in the middle of a phone number the caret usually jumps to the end: this is the expected behaviour and it's the workaround for this Samsung Galaxy bug.

Internally smartCaret={false} sets inputComponent to <BasicInput/> instead of input-format's <ReactInput/>. Use it instead of passing inputComponent={BasicInput} directly.

The demo has also been updated with an example of using the smartCaret={false} property.

@catamphetamine catamphetamine changed the title Cursor doesn't keep up with typing on android phones [See readme for the workaround] Cursor doesn't keep up with typing on some Samsung Android phones Apr 19, 2018
@catamphetamine catamphetamine changed the title [See readme for the workaround] Cursor doesn't keep up with typing on some Samsung Android phones (see readme for the workaround) Cursor doesn't keep up with typing on some Samsung Android phones Apr 19, 2018
@JsGvDev
Copy link

JsGvDev commented Apr 19, 2018

Great fix @catamphetamine, I have been testing today from my device Samsung Galaxy S7 edge, everything is working fine. I tried to reproduce different issues commented above, and they work well, also I've tried other possibilities and it works fine.

@catamphetamine
Copy link
Owner

catamphetamine commented Apr 19, 2018

I also rewrote <BasicInput/>'s fix for React caret jumping today.
It was using direct ReactDOM.findDOMNode(this.ref).value = event.target.value in 1.0.8.
In the new react-phone-number-input@1.0.9 this ReactDOM.findDOMNode approach was replaced with storing value in this.state (React complained about using ReactDOM.findDOMNode inside .render()).
Because I don't have a Galaxy phone I don't know if 1.0.9 still works.
We'll see what different people say.

@JsGvDev
Copy link

JsGvDev commented Apr 19, 2018

Hey @catamphetamine, I did test right now with new version and still working fine in my samsung phone.

I've tested to different browsers from Samsung Galaxy S7 Edge:

Chrome ✅
Firefox ✅
Samsung Internet Browser

Even I have simulated with Xcode iPhone X (Safari Browser ✅) is working fine.

But for main mobile browser (Chrome and Safari) is working fine.

The problem only persist with that Samsung browser but this one is a minor issue, I haven't tried other browsers. You have more here.

Also I leave a codePen. I did quickly test in there. Everyone can check it for their self.

@catamphetamine
Copy link
Owner

catamphetamine commented Apr 19, 2018

@Xopsy wow, thanks for thorough testing 👍
Ok then, it works.

Samsung seems to be a really big company that can afford writing its own everything: their own Android shell, their own Bixby, their own web browser... Looks like another Microsoft to me.

I added a smartCaret={false} property which sets inputComponent to <BasicInput/>. Use it instead of passing inputComponent={BasicInput} directly.

The demo has also been updated with an example of using the smartCaret={false} property.

@catamphetamine
Copy link
Owner

Update: I released version 2.x which defaults to smartCaret={false}.

@catamphetamine
Copy link
Owner

Since smartCaret is now false by default I'll close this issue until someone reports otherwise.
Btw, I released version 2.1 which now only supports React >= 16.3

@catamphetamine catamphetamine changed the title (see readme for the workaround) Cursor doesn't keep up with typing on some Samsung Android phones Cursor doesn't keep up with typing on some Samsung Android phones Jul 26, 2018
@catamphetamine
Copy link
Owner

FYI: released version react-phone-number-input@2.5.0 where smartCaret is true by default again. The reason is that I searched for popular mask input libraries and found text-mask to be one of the most popular. I searched its issues and found the exact same one. And I just copied their solution: wrapping onChange in setTimeout(0). We'll see how users report if it works on weird Androids now.
Updated the demo.

@EzanAsif
Copy link

@catamphetamine I was getting same bug, it worked after setting smartCart = {false}

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

No branches or pull requests

8 participants