Allow copying highlighted text and prevent editing when pressing ctrl+input key #3431
Conversation
jj49411
changed the title
src/DataGrid.tsx
Outdated
| const cKey = 67; | ||
| const vKey = 86; | ||
|
|
||
| if (isCtrlKeyHeldDown(event) && keyCode === cKey && window.getSelection()?.toString() !== '') { |
There was a problem hiding this comment.
Can/should we check that the selection starts/ends in the cell?
https://developer.mozilla.org/en-US/docs/Web/API/Selection#instance_properties
I wonder if we can do something like cell.contains(selection.anchorNode) for example.
How should we handle the isRowEvent case?
There was a problem hiding this comment.
Can/should we check that the selection starts/ends in the cell?
Why do we need to check this? When the text selection is across multiple cells, it doesn't open the editor and performs the browser behaviour.
There was a problem hiding this comment.
I can click on a cell, select text elsewhere without losing focus on the cell, then press ctrl+c, what should happen then? Right now we open the selected cell and copy the selected cell's text.
I can also select text in a cell, tab to the next cell, and the selection will remain on the previous cell. Same issue as above.
@amanmahajan7 any ideas?
There was a problem hiding this comment.
For both scenarios, it doesn't open the editor and copies the selected text, is it not desirable?
There was a problem hiding this comment.
Mmh, yes that seems correct, maybe I'm confusing myself.
There was a problem hiding this comment.
We don't have special handling for isRowEvent and it copies text as expected, is there anything we need to consider?
Co-authored-by: Nicolas Stepien <[email protected]>
…ata-grid into jc-handle-ctrl-c
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3431 +/- ##
==========================================
- Coverage 98.26% 98.25% -0.02%
==========================================
Files 47 47
Lines 5082 5093 +11
Branches 725 730 +5
==========================================
+ Hits 4994 5004 +10
- Misses 88 89 +1
|
| // event.key may differ by keyboard input language, so we use event.keyCode instead | ||
| // event.nativeEvent.code cannot be used either as it would break copy/paste for the DVORAK layout | ||
| const cKey = 67; | ||
| const vKey = 86; | ||
| if (keyCode === cKey) { | ||
| handleCopy(); |
There was a problem hiding this comment.
I wonder if we should check window.selection before calling handleCopy. Any benefit of checking it outside? Also, should we call onCopy prop regardless of the selection?
There was a problem hiding this comment.
If I understand correctly, selectedCellIsWithinViewportBounds already checks if a cell is selected/focused, why do we need to check window.selection?
There was a problem hiding this comment.
If I understand correctly, selectedCellIsWithinViewportBounds already checks if a cell is selected/focused
selectedCellIsWithinViewportBounds checks cell selection which is a state inside RDG and it is different from window.getSelection(). I was thinking of doing this
if (keyCode === cKey) {
const selection = window.getSelection();
if (selection !== null && selection.toString() !== '') return;
handleCopy();
}
There was a problem hiding this comment.
In the above condition, if ctrl+c is pressed and window.getSelection()?.isCollapsed === false, it will return so won't go through this condition though
…+input key (adazzle#3431) * Allow copying highlighted text * Prevent editing when pressing ctrl+input key * Add test * Refactor conditions * Revert changes * Update test/copyPaste.test.tsx Co-authored-by: Nicolas Stepien <[email protected]> * Move comments * Check selection and add comment * Refactor getSelection * Allow ctrl+space * Check control v inside isDefaultCellInput * Refactor copying text --------- Co-authored-by: Cheng <[email protected]> Co-authored-by: Nicolas Stepien <[email protected]> Co-authored-by: Aman Mahajan <[email protected]>
It should prevent editing when we press
Before
After
