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.

Sign up for GitHub

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

Jump to bottom

[fields] Support Backspace key on Android #7842

Merged
merged 5 commits into from
Feb 10, 2023
Merged

[fields] Support Backspace key on Android #7842

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
44032c6
[fields] Support Backspace key on Android
flaviendelangle Feb 6, 2023
af084ea
Merge branch 'next' into backspace-android
flaviendelangle Feb 8, 2023
84f38eb
Fix
flaviendelangle Feb 8, 2023
6abde04
Code review: Alex
flaviendelangle Feb 8, 2023
7ae71a5
Fix tests
flaviendelangle Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/x-date-pickers/src/internals/hooks/useField/useField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,16 @@ export const useField = <
return () => window.clearTimeout(focusTimeoutRef.current);
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// If `state.tempValueStrAndroid` is still defined when running `useEffect`,
// 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.
LukasTy marked this conversation as resolved.
Show resolved Hide resolved
React.useEffect(() => {
Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

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. 🤔

Copy link
Member Author

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.

Copy link
Member

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 the Backspace 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. 👌

Copy link
Member

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)

Copy link
Member Author

@flaviendelangle flaviendelangle Feb 8, 2023 •
edited
Loading

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 second onChange 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.

if (state.tempValueStrAndroid != null && selectedSectionIndexes != null) {
clearActiveSection();
}
}, [state.tempValueStrAndroid]); // eslint-disable-line react-hooks/exhaustive-deps

const valueStr = React.useMemo(
() => state.tempValueStrAndroid ?? fieldValueManager.getValueStrFromSections(state.sections),
[state.sections, fieldValueManager, state.tempValueStrAndroid],
Expand Down
1 change: 1 addition & 0 deletions packages/x-date-pickers/src/internals/hooks/useField/useFieldState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ export const useFieldState = <
return setState((prevState) => ({
...prevState,
sections: newSections,
tempValueStrAndroid: null,
Copy link
Member Author

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.

...newValue,
}));
};
Expand Down