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 Alt+c being ignored on some systems #3660

Merged
merged 9 commits into from
Jan 26, 2022

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jan 18, 2022

cell.focus();
// On some systems (e.g. MacOS) Alt+C might add accents to letters (like Alt+c gives ç)
// depending on keyboard layout and language
fireEvent.keyDown(cell, { key: 'ç', altKey: true, code: 'KeyC' });
Copy link
Member Author

Choose a reason for hiding this comment

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

Not ideal, but that's the best I could do to cover this test case.
Tried playwright, but it's not possible to use clipboard in headless mode.

@mui-bot
Copy link

mui-bot commented Jan 18, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 145.5 242.4 223.6 200.58 41.964
Sort 100k rows ms 280.8 599.8 503.4 463.36 106.344
Select 100k rows ms 136.6 224 171 174.38 32.864
Deselect 100k rows ms 85.6 218.4 218.4 154.74 55.205

Generated by 🚫 dangerJS against f589472

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

I can't test it since I'm on Windows. Maybe @DanailH could take a look.

@cherniavskii
Copy link
Member Author

@m4theushw can you confirm the issue not reproducible on Windows?

@DanailH
Copy link
Member

DanailH commented Jan 18, 2022

I can't test it since I'm on Windows. Maybe @DanailH could take a look.

Just tested it. The problem seems to be fixed on Mac.

@m4theushw
Copy link
Member

On Windows, if I have a keyboard without ç, I must use AltGr+,. See https://superuser.com/questions/1075992/cedilla-under-c-%C3%A7-in-us-international-with-dead-keys-keyboard-layout-in-linu. That way, event.key is not "c".

@cherniavskii
Copy link
Member Author

On Windows, if I have a keyboard without ç, I must use AltGr+,.

@m4theushw but what happens if you press Alt + c on your default keyboard layout? Does it copy rows with headers on mui.com (before this fix)?

@cherniavskii cherniavskii added the component: data grid This is the name of the generic UI component, not the React module! label Jan 18, 2022
@m4theushw
Copy link
Member

but what happens if you press Alt + c on your default keyboard layout? Does it copy rows with headers on mui.com (before this fix)?

It copies including headers. That could be a problem if the browser also used this shortcut, but it's not used (IE uses but we partially support it).

* depending on keyboard layout and language.
* In this case `event.code` is used to check the actual key pressed.
*/
if ((event.key.toLowerCase() !== 'c' && event.code !== 'KeyC') || !isModifierKeyPressed) {
Copy link
Member

@oliviertassinari oliviertassinari Jan 18, 2022

Choose a reason for hiding this comment

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

Would it be enough to check the physical keyboard?

Suggested change
if ((event.key.toLowerCase() !== 'c' && event.code !== 'KeyC') || !isModifierKeyPressed) {
if (event.code !== 'KeyC' || !isModifierKeyPressed) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I'm just being cautious here.

There are different layouts, and for example KeyC will be returned for C key on QWERTY and J key on Dvorak.
If we remove event.key, Alt+C on Dvorak layout won't work, which is a bad UX.
So I think it's better to keep both.

Here's some context on this https://ux.stackexchange.com/questions/30666/keyboard-shortcuts-on-non-qwerty-keyboard-layouts

Copy link
Member

@oliviertassinari oliviertassinari Jan 22, 2022

Choose a reason for hiding this comment

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

The Dvorak keyboard case is interesting, I didn't know that it would have an influence here. It's the time I see it relevant in a discussion on MUI 😁. But for a macOS user with a Dvorak keyboard, would it mean that a user would need to press Option + J?

In this case maybe?

Suggested change
if ((event.key.toLowerCase() !== 'c' && event.code !== 'KeyC') || !isModifierKeyPressed) {
// event.key === 'c' is not enough as alt+c can lead to ©, ç, or other characters on macOS.
// event.code === 'KeyC' is not enough as event.code assume a QWERTY keyboard layout which would be wrong with a Dvorak keyboard (as if pressing J).
if (event.keyCode !== 67 || !isModifierKeyPressed) {

Anyway, it's a detail. I might not have the time to check your answer, feel free to move forward with what you think of best.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, that's the only solution that would work consistently on every layout (Alt/Option + C, where C is the key that is marked as C on system keyboard layout)

The problem with keyCode is that it's deprecated in favor of key and code.
keyCode is still supported by browsers, since a lot of apps use it.


This made me think if Alt/Option + C is a good idea for keyboard shortcut. Because it has direct impact on another feature of DataGrid - Editing.

The problem

When user presses any printable key, grid cell enters edit mode (as per https://mui.com/components/data-grid/editing/#start-editing)
As we have seen here, on MacOS Option is a modifier key, and Option+C on most layouts results in printable key (e.g. ç on British/U.S. layout) that IMHO should start edit mode. Currently, it doesn't and it's something we should fix.

Proposal

Since Alt+C is the only shortcut that uses Alt (see https://mui.com/components/data-grid/accessibility/#keyboard-navigation) and it's not a widespread shortcut (as opposed to Ctrl+C/Ctrl+V), I propose to change it to something that doesn't involve Alt key:

Ctrl+Shift+C

I think it makes more sense.
We need to add event.preventDefault() though, since this shortcut opens DevTools on MacOS 🙃

Doubts

  • Should we consider this a breaking change?
    Technically it's a breaking change, but the functionality doesn't work on MacOS.
    We can leave current implementation for backward compatibility though.
    What do you think?

Action points

  • change Alt+C shortcut to Ctrl+Shift+C
  • start editing cell when user presses printable key modified with Alt/Option - this might be a tricky one, since I'm not sure yet how we define "printable key". This can be explored in a separate issue.

Would love some input from @mui-org/x members

Copy link
Member

@oliviertassinari oliviertassinari Jan 24, 2022

Choose a reason for hiding this comment

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

+1 to:

  • fix quick and dirty now (event.keyCode)
  • cover in [discussion] Preparing v6 #3287 that we might be able to improve this with a breaking change in v6. Hopefully, we will have a better product knowledge at this point to decide if it's worth a change, and what change.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Jan 18, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022
@cherniavskii cherniavskii merged commit 4e99fa4 into mui:master Jan 26, 2022
@cherniavskii cherniavskii deleted the fix-copy-to-clipboard branch January 26, 2022 19:18
// event.key === 'c' is not enough as alt+c can lead to ©, ç, or other characters on macOS.
// event.code === 'KeyC' is not enough as event.code assume a QWERTY keyboard layout which would
// be wrong with a Dvorak keyboard (as if pressing J).
if (String.fromCharCode(event.keyCode) !== 'C' || !isModifierKeyPressed) {
Copy link
Member

@oliviertassinari oliviertassinari Jan 26, 2022

Choose a reason for hiding this comment

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

I have learned something new here. I didn't know about String.fromCharCode, nice trick 👍

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] ALT+C keyboard shortcut doesn't work
5 participants