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

[DataGrid] Fix copy paste cells with localization keyboard #14220

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

rotembarsela
Copy link
Contributor

Closes #14219

@mui-bot
Copy link

mui-bot commented Aug 15, 2024

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

Generated by 🚫 dangerJS against 58bd2b7

@rotembarsela rotembarsela changed the title [DataGrid] fix copy paste cells with localization keyboard [DataGridPremium] fix copy paste cells with localization keyboard Aug 15, 2024
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Aug 15, 2024
@@ -57,11 +57,23 @@ export const isHideMenuKey = (key: React.KeyboardEvent['key']) => isTabKey(key)
export function isPasteShortcut(event: React.KeyboardEvent) {
if (
(event.ctrlKey || event.metaKey) &&
event.key.toLowerCase() === 'v' &&
event.code === 'KeyV' &&
Copy link
Member

@oliviertassinari oliviertassinari Aug 16, 2024

Choose a reason for hiding this comment

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

This can't work, e.g. event.code assumes a QWERTY keyboard layout which would be wrong with a Dvorak keyboard. We faced this in https://github.com/mui/mui-x/pull/3660/files#r793084149.

This property is useful when you want to handle keys based on their physical positions on the input device rather than the characters associated with those keys

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code

I can reproduce the bug on macOS with:

SCR-20240817-ugnn SCR-20240817-ufoh

It seems that I broke it with https://github.com/mui/mui-x/pull/9220/files#r1216831312, and that we can only event.keyCode use to detect the shortcut correctly. String.fromCharCode(event.keyCode) is the same but with a nicer DX. https://stackoverflow.com/questions/2903991/how-to-detect-ctrlv-ctrlc-using-javascript seems to confirm it.

Suggested change
event.code === 'KeyV' &&
String.fromCharCode(event.keyCode) === 'V' &&

Signed-off-by: Olivier Tassinari <[email protected]>
@oliviertassinari oliviertassinari changed the title [DataGridPremium] fix copy paste cells with localization keyboard [DataGridPremium] Fix copy paste cells with localization keyboard Aug 16, 2024
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work regression A bug, but worse labels Aug 16, 2024
…idAggregationInterfaces.ts

Signed-off-by: Olivier Tassinari <[email protected]>
scripts/README.md Outdated Show resolved Hide resolved
Signed-off-by: Olivier Tassinari <[email protected]>
Signed-off-by: Olivier Tassinari <[email protected]>
@oliviertassinari oliviertassinari dismissed their stale review August 18, 2024 12:17

Initial concern solved

@oliviertassinari oliviertassinari changed the title [DataGridPremium] Fix copy paste cells with localization keyboard [DataGrid] Fix copy paste cells with localization keyboard Aug 18, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 18, 2024

I have added some comments to help in the future, we have had so many iterations of this logic, so to make sure we build up. This looks good 👍, I have unsubscribed to notifications.

For those curious about pushing it even further: w3c/uievents#377.

@oliviertassinari
Copy link
Member

The date picker has the same problem:

@rotembarsela
Copy link
Contributor Author

rotembarsela commented Aug 19, 2024

The date picker has the same problem:

isPasteShortcut func has the same problem when trying to paste after the CTRL+C event,
https://github.com/mui/mui-x/blob/master/packages/x-data-grid/src/utils/keyboardUtils.ts

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: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Copy & Paste on Cells using localize keyboard
6 participants