3.0 issue75 global copy paste #87
Conversation
| /*#if TEST_COPY_PASTE*/ | ||
| Clipboard.value = value; | ||
| /*#else*/ | ||
| _ev.clipboardData.setData('text/plain', value); | ||
| _ev.preventDefault(); |
There was a problem hiding this comment.
Set our value and prevent the normal execution from overriding our value
There was a problem hiding this comment.
Because of the test vs normal code, sometimes the event variable is not used.. prepending with '_' disables the usage check.. a bit lame but somewhat cleaner than an if..endif for the variable IMO
| onPaste={this.onPaste} | ||
| tabIndex={-1} | ||
| > | ||
| (<table tabIndex={-1}> |
There was a problem hiding this comment.
Pushing onPaste handling to the top component element
| return; | ||
| } | ||
| /*#endif*/ |
There was a problem hiding this comment.
The test now need to override both copy and paste keyboard scenarios
There was a problem hiding this comment.
This adds the #75 feature (browser menu copy/paste). However, it appears to introduce a regression: Ctrl-V on a focused cell no longer works as expected. This is slightly confusing because #79 selects all input text upon a single click, so in this state copy/paste will of course work natively with either the menu or with keyboard shortcuts. So to be clear, I am referring to the behaviour on a focused, active cell (with a blinking cursor) — on master Ctrl-V will paste the contents of the clipboard on such a cell.
Regarding the code, the issue above aside, removing the document.execCommand('copy') method in favor of this code seems cleaner to me. Please see below comment for clarification re: the implementation here.
| document.getSelection().removeAllRanges(); | ||
| // Restore the original selection | ||
| document.getSelection().addRange(selected); | ||
| } |
There was a problem hiding this comment.
(for my own understanding)
Can you explain briefly how this implementation using document.execCommand fit in with the method using the native onCopy event handler together with TableClipboardHelper.toClipboard()?
There was a problem hiding this comment.
The document.execCommand was not used with native onCopy, it was always being called by a onkeyDown listener deciding that a combination of keys meant we wanted to copy or paste. This method is typically used to put something in the clipboard when there is not a user interaction/trigger for it. It's also why the behavior was incorrect when other valid ways of initiating copy/paste were used.
So instead, the code now listens for copy/paste events and it gets funneled to whoever has focused (the table's focus is a bit fuzzy at times, but the last user interaction that sets focus would focus on the latest table, hopefully.. I have my doubts but won't really know what happens until we test scenarios with multiple tables visible at the same time -- anyway setting the listener at the top element, the event will eventually propagate there)
Now that we listen to native copy/paste events (all sources), we need to make sure the copy contains the data we want, hence the listener on the copy event calling the helper, setting the value in the event and preventing further default processing that would override what we've just set in the event.
Hopefully this all makes sense.
I'll look at the regression tmr!
There was a problem hiding this comment.
OK, I think I understand. So listening to all copypaste event sources now eliminates the need for this block with execCommand. I think the code is cleaner without this (the HackerNoon tutorial method), so once that regression is fixed I'm all for it.
There was a problem hiding this comment.
Exactly. Listening to all, prevent defaults where needed (forgot one) and overriding with our values.
| e.preventDefault(); | ||
| } | ||
| private handlePaste = (e: ClipboardEvent) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
This was causing the regression -- input should not react to the paste event and let the table handle it
There was a problem hiding this comment.
Thinking out loud — can we have a test, or debugging function, that listens for unexpected event triggers and/or default events? If it's the latter it could be removed in production.
There was a problem hiding this comment.
We could get all object members that start with 'on' (or just a static list) for a given element and do
el.addEventListener(objMember.substring(2), () => { /* do something */ }) for each of those -- we could test if 'defaultPrevented' is true, not much can be done for stopPropagation.. once that in place I'm not sure how to build a useful test from there but it should be possible.
| row_deletable, | ||
| row_selectable, | ||
| setProps, | ||
| sorting_settings, | ||
| virtual_dataframe_indices | ||
| } = this.props; | ||
|
|
||
| if (is_focused || !editable) { | ||
| if (!editable) { |
There was a problem hiding this comment.
The paste event is no longer global, if this method gets called it comes from a child input and we know we have focus.
| ).join('\r\n'); | ||
| ); | ||
|
|
||
| const value = SheetClip.prototype.stringify(df); |
There was a problem hiding this comment.
Using SheetClip's way of stringifying instead of our own gets rid of the phantom \r and guarantees we are in=line with what it expects when parsing
| @@ -7,22 +7,57 @@ describe('copy paste', () => { | |||
| cy.visit('http://localhost:8082'); | |||
| }); | |||
|
|
|||
| it('can do BE roundtrip on cell modification', () => { | |||
| it('can copy multiple rows', () => { | |||
There was a problem hiding this comment.
Additional test to see if we can copy multiple rows
| } | ||
| }); | ||
|
|
||
| it('can copy multiple rows and columns', () => { |
There was a problem hiding this comment.
Additional test to see if we can copy multiple rows and columns
There was a problem hiding this comment.
These tests should protect us better from the regression @wbrgss pointed out
|
@wbrgss Please try and give this another go :) |
| e.preventDefault(); | ||
| } | ||
| private handlePaste = (e: ClipboardEvent) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Thinking out loud — can we have a test, or debugging function, that listens for unexpected event triggers and/or default events? If it's the latter it could be removed in production.
|
sorry-- manually testing some things look incorrect when copying with command c can we wait on merging this and test manually once again |
|
@cldougl Sure. This can wait. Can you describe the problematic scenario? |
|
@Marc-Andre-Rivet basically I'm sorting and copying and pasting and just making sure everything is working as expected |
|
💃 |
Copy/paste that involved the browser menu (edit -> copy or paste) was not working correctly because the clipboard value was only updated when the keyboard combinations Ctrl+C, Ctrl+V were used.
Instead, listen to copy and paste events on the top component of the table, this picks up keyboard, mouse and menu driven copy/paste scenarios, apply our resolution logic for copy content and prevent default execution.