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

[fields] Fix section clearing behavior on Android #13652

Merged

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Jun 28, 2024

Fixes #13620

Related to #10399.

The present behavior and test for section clearing on Android seemed incorrect.
It doesn't work like that on Desktop.
The change aligns the section clearing behavior between Desktop and Android.

There is still an issue on Chrome on Android with syncing the focus on a section, which has a different length placeholder than the value (i.e. full length month).

screen-20240628-160431.2.mp4

Maybe because it requires an extra repaint/resize cycle. 🤷
If you have ideas on how to solve it—I'm all ears.
P.S. Wrapping the syncSelectionToDOM call in a setTimeout fixes the issue, but introduces a delay and causes the caret to be momentarily visible.
P.P.S. The situation with the MS Swiftkey keyboard is even worse... All the section selections are no syncked after section removal. 🙈

This fix seems to avoid the problem and works stable: 73ed8e9 (albeit with a slight chance of endless recursion in an unforeseen case).

In any case, I'd be happy to ship this fix as is, because the current state of Chrome on Android with Gboard is pretty bad-unusable when deleting. 🙈

@LukasTy LukasTy added bug 🐛 Something doesn't work regression A bug, but worse component: pickers This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition labels Jun 28, 2024
@LukasTy LukasTy self-assigned this Jun 28, 2024
@mui-bot
Copy link

mui-bot commented Jun 28, 2024

Deploy preview: https://deploy-preview-13652--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 73ed8e9

@LukasTy LukasTy requested review from flaviendelangle and a team June 28, 2024 13:11
@LukasTy LukasTy marked this pull request as ready for review June 28, 2024 13:11
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's improves the current behavior, I think we can ship it

Is the behavior better / worse in v7 approach?

(inputRef.current.selectionStart !== selectionStart ||
inputRef.current.selectionEnd !== selectionEnd)
) {
interactions.syncSelectionToDOM();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid of a possible endless recursion here. 🤔
WDYT, do we need an additional safeguard against it here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a clearer idea of a scenario when this recursion might happen?
Tbh I don't really know how to add safeguards here 😬

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any. 🤔

@LukasTy
Copy link
Member Author

LukasTy commented Jun 28, 2024

Is the behavior better / worse in v7 approach?

It's better on enableAccessibleDOMStructure.
It needs additional testing on various keyboards, but overall works great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fields] on Android: deleting the date template is possible and breaks the component
3 participants