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

[DataGrid] Fix Alt+c being ignored on some systems #3660

Merged
merged 9 commits into from
Jan 26, 2022
Merged

[DataGrid] Fix Alt+c being ignored on some systems #3660

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3f520b2
[test] cover alt+c copy
cherniavskii Jan 18, 2022
a784117
[DataGrid] Fix Alt+c being ignored on some systems
cherniavskii Jan 18, 2022
a0fc376
[test] Do not skip clipboard tests in real browser
cherniavskii Jan 18, 2022
6490f5e
[docs] Update MacOS key assignment
cherniavskii Jan 19, 2022
b35b309
use keyCode instead of code
cherniavskii Jan 24, 2022
a957848
fix failing tests
cherniavskii Jan 24, 2022
8b8355c
Merge branch 'master' into fix-copy-to-clipboard
cherniavskii Jan 25, 2022
735192b
fix failing test
cherniavskii Jan 25, 2022
f589472
fix failing test
cherniavskii Jan 26, 2022
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
7 changes: 6 additions & 1 deletion packages/grid/_modules_/grid/hooks/features/clipboard/useGridClipboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,12 @@ export const useGridClipboard = (apiRef: GridApiRef): void => {
const handleKeydown = React.useCallback(
(event: KeyboardEvent) => {
const isModifierKeyPressed = event.ctrlKey || event.metaKey || event.altKey;
if (event.key.toLowerCase() !== 'c' || !isModifierKeyPressed) {
/**
* On some systems (e.g. MacOS) Alt+C might add accents to letters (like Alt+c gives ç)
* 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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

return;
}

Expand Down
33 changes: 26 additions & 7 deletions packages/grid/x-data-grid-pro/src/tests/clipboard.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,6 @@ describe('<DataGridPro /> - Clipboard', () => {
describe('copySelectedRowsToClipboard', () => {
let writeText;

before(function beforeHook() {
if (!isJSDOM) {
cherniavskii marked this conversation as resolved.
Show resolved Hide resolved
// Needs permission to read the clipboard
this.skip();
}
});

beforeEach(function beforeEachHook() {
writeText = stub().resolves();

Expand Down Expand Up @@ -94,5 +87,31 @@ describe('<DataGridPro /> - Clipboard', () => {
expect(writeText.firstCall.args[0]).to.equal(['0\tNike', '1\tAdidas'].join('\r\n'));
});
});

it(`should copy the selected rows and headers to the clipboard when Alt + C is pressed`, () => {
render(<Test />);
apiRef.current.selectRows([0, 1]);
const cell = getCell(0, 0);
cell.focus();
fireEvent.keyDown(cell, { key: 'c', altKey: true });
expect(writeText.callCount).to.equal(1, "writeText wasn't called");
expect(writeText.firstCall.args[0]).to.equal(
['id\tBrand', '0\tNike', '1\tAdidas'].join('\r\n'),
);
});

it(`should copy the selected rows and headers to the clipboard when Alt + C is pressed with modified key`, () => {
render(<Test />);
apiRef.current.selectRows([0, 1]);
const cell = getCell(0, 0);
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.

expect(writeText.callCount).to.equal(1, "writeText wasn't called");
expect(writeText.firstCall.args[0]).to.equal(
['id\tBrand', '0\tNike', '1\tAdidas'].join('\r\n'),
);
});
});
});