-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[fields] Support Backspace key on Android #7842
Conversation
554ca1e
to
44032c6
Compare
@@ -242,6 +242,7 @@ export const useFieldState = < | |||
return setState((prevState) => ({ | |||
...prevState, | |||
sections: newSections, | |||
tempValueStrAndroid: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sure that cleanActiveSection
resets the tempValueStrAndroid
We could probably replace all the setState
in this file with a wrapper which removes tempValueStrAndroid
to avoid having to define this null
value each time.
These are the results for the performance tests:
|
// Then `onChange` has only been called once, which means the user pressed `Backspace` to reset the section. | ||
// This causes a small flickering on Android, | ||
// But we can't use `useEnhancedEffect` which is always called the two `onChange` calls and would cause false positives. | ||
React.useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my tests, the 2nd onChange
is always called before the React.useEffect
, but I don't have the proof that it's always the case, which is not great.
I could add some delay, but the longer I add, the more the flickering will be visible.
If you have an idea to reliably react to the lack of 2nd onChange
instead of saying "nothing happened so there will be no 2nd onChange
", that would of course be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised that between the "delete" onChange and the "replace" onChange JS has the time to do a render, but it' make sens because JS calls event one after the other according to the "Event loops" section of this article
I made two test to experiment.
From what I understand, when for example you click on the button (which triggers focus and click). The browser add two message in the pool to handle: 'focus' and 'mouse down'.
When JS has some time available, it take the first one, run the code, and enter in the main thread loop (which let React do his full cicle)
Then it takes the second one (click) and do a new main trhead cycle.
I added a long loop (for (var i = 0; i < 1000000000; i++)
) to add delay in the event handler which is not async. And the result is the same, so except if another event is triggered between the "delete" and "replace" onChange
, the useEfect should always be called at the correct moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your article is not specifically about Android.
I don't know which is the abstraction level which decided to call onChange
twice when pressing a key on Android
.
But I don't think that the event loop is the place deciding when this 2nd call is done.
I suspect that those two calls are always synchronous (or at least at the micro-task level), otherwise all the websites on Android would have flickering when typing.
And if that's the case, then indeed, the useEffect
will always occur after the 2nd onChange
since it's is applied after paining the new UI (which occur after the micro-tasks).
So basically, I have no proof of when this 2nd onChange
is called by Android.
But it would be very weird if it was called too late for the useEffect
to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your article is not specifically about Android.
Why would it be? the JS cycle is defined by the V8 engine.
Are we sure it's an android bug. This double onChange
only occurs with the Microsoft keyboard.
In the input change demo, when using GBoard to select a range and override it, the onChange
is called only once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be? the JS cycle is defined by the V8 engine.
I don't think the execution cycle differs that between different JS engines, so, it's not just V8 🤔
Are we sure it's an android bug. This double
onChange
only occurs with the Microsoft keyboard.
It probably is. I've read through possibly related annoying issue still present in react-native
. I've done quite a bit of testing between Firefox and Chrome. It seems that even with GBoard
the same issue happens when removing the full month section (April) in the same demo.
On another note: Spectrum has the same issue and sometimes the component fails miserably It must have been a fluke or they managed to notice and fix it in a few hours 🙈 , while Telerik approach seems the most robust and lacks any flickering, even though, they are also using an input as we currently do.
Side note: our field component experience on Firefox mobile is really clunky, due to how the text is selected and also marked, but Telerik does also suffer from this issue.
All in all, I'm not the biggest fan of the current solution, especially due to the flickering. If we can't think of anything better, maybe that could be a happy middle ground to at least be on par with other libraries. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be? the JS cycle is defined by the V8 engine.
But for me it's not a JS cycle topic.
Android (or the keyboard) has a weird way of updating the value in the input. It could very well fire the 2nd onChange
asynchronously in some scenario, it's not the engine who decides that.
Are we sure it's an android bug. This double onChange only occurs with the Microsoft keyboard.
I took for granted that it was Android but if it's only one keyboard (or at least not all), then it's probably even worse because it mean different keyboard can have different behaviors 😬
But it would also mean that most Android user could have the best experience without those hacks.
It's probably worth testing more to see exactly who is the "Android" hack for.
Telerik approach seems the most robust and lacks any flickering
Did you successfully used their DateInput
on Android ?
Last time I tested it did not work at all on Swiftkey (Microsoft Keyboard), not just the Backspace
but also all the rest of the editing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you successfully used their
DateInput
on Android ?
Last time I tested it did not work at all on Swiftkey (Microsoft Keyboard), not just theBackspace
but also all the rest of the editing.
Ah, sorry to misrepresent Telerik
. This time I did only check the deleting behaviour. The editing is totally broken with SwiftKey 😢 But works fine with GBoard
. Later today I'll try to also test native Xiaomi and Samsung keyboards to see, how they behave with our, Spectrum and Telerik solutions. 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting back after some more testing.
Samsung keyboard is working great as does the Gboard
. Even better actually, because it does not clear April
the same way SwiftKey
clears all other sections.
As for Xiaomi, sorry, but it does not seem to have it's own keyboard.
I also tried a few popular keyboards on my own device:
- Fonts (works the best, like Samsung, doesn't clear "April")
- Go Keyboard Lite (same level/issues as SwiftKey)
- Grammarly (same level/issues as SwiftKey)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you say that we have 2 type of Android keyboards:
- the one with a "PC-like" approach, they fire
onChange
only once so they work just like on a PC. - the one with a "Swiftkey-like" approach, they first fire on
onChange
with a cleaned selected value then a secondonChange
with the key pressed as the selected value
In which case, supporting Swiftkey as much as possible should hopefully be enough to cover the whole space.
Fixes #7835