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] Correctly handle events with a complete value insertion #9896

Merged
merged 7 commits into from
Aug 3, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Aug 3, 2023

This fix resolves the issue mentioned here: #9165 (comment)

Calling fill('04/11/2022') (in playwright and possibly other e2e frameworks) on the pickers field does nothing because we explicitly check only the event.target.value.
In such case event.target.value will equal 04/11/2022/MM/YYYY, which is not valid as per our code and nothing happens.

With the change done here, we'd use the event.nativeEvent.data, which would contain the whole 04/11/2022 and update the value using it, instead of the regular section updating process.

When using regular editing by typing characters with the keyboard results in event.nativeEvent.data having only the changed character.

@LukasTy LukasTy added test component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Keyboard editing Related to the pickers keyboard edition labels Aug 3, 2023
@LukasTy LukasTy self-assigned this Aug 3, 2023
@mui-bot
Copy link

mui-bot commented Aug 3, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9896--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 321.8 486 392.6 386.48 58.689
Sort 100k rows ms 666.3 1,428.4 1,379.1 1,178.5 270.962
Select 100k rows ms 644.9 761.7 696.7 706.46 43.434
Deselect 100k rows ms 116.2 223.4 147 166.7 39.772

Generated by 🚫 dangerJS against 1efd153

const eventData = (event.nativeEvent as InputEvent).data;
// Calling `.fill(04/11/2022)` in playwright will trigger a change event with the requested content to insert in `event.nativeEvent.data`
// usual changes have only the currently typed character in the `event.nativeEvent.data`
const useEventData = eventData && eventData.length > 1;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure it can not be more than one character? I'm thinking about special keyboards or some auto-completion

I don't see issues, it's just to share a point of attention. I first thought about #9465 but it's not the same problem since IME keyboard calls multiple times the onChange

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we sure it can not be more than one character? I'm thinking about special keyboards or some auto-completion

I'm definitely not sure that there wouldn't be any edge cases, but autocompletion is turned off, although, if any extension hacks it's way into this, I'd imagine that this change could only fix it and not break because it would probably use a similar API of emitting a change event with the full value string. 🤔

test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Flavien DELANGLE <[email protected]>
Signed-off-by: Lukas <[email protected]>
@LukasTy LukasTy merged commit bc9cdb5 into mui:master Aug 3, 2023
@LukasTy LukasTy deleted the fix-fields-behavior-in-e2e branch August 3, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Keyboard editing Related to the pickers keyboard edition test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants