slate icon indicating copy to clipboard operation
slate copied to clipboard

Fix Copy/pasting void elements is not working

Open laufeyrut opened this issue 1 year ago • 5 comments

Description This PR makes it possible to copy/cut void elements.

Issue Fixes: https://github.com/ianstormtaylor/slate/issues/4808 and https://github.com/ianstormtaylor/slate/issues/4806

Example Before Kapture 2022-09-13 at 11 50 06 After: Kapture 2022-09-13 at 12 46 02

Context Created a new function hasSelectableTarget that checks if hasEditableTarget and isTargetInsideNonReadonlyVoid. Using this function instead of hasEditableTarget so we are not excluding nonReadonlyVoids fx onCopy and onCut and onPaste

I did not add Cypress tests for the new logic since Cypress does not support copy/paste and the only solution I found was to use some package like https://www.npmjs.com/package/clipboardy. If we want to include the package or if there is a better solution for it I'm happy to add the tests.

Checks

  • [x] The new code matches the existing patterns and styles.
  • [x] The tests pass with yarn test.
  • [x] The linter passes with yarn lint. (Fix errors with yarn fix.)
  • [x] The relevant examples still work. (Run examples with yarn start.)
  • [x] You've added a changeset if changing functionality. (Add one with yarn changeset add.)

laufeyrut avatar Sep 13 '22 12:09 laufeyrut

🦋 Changeset detected

Latest commit: 79933152e7b9b7656af294e4a6d6f00cc8cb3e11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Sep 13 '22 12:09 changeset-bot[bot]

@dylans After reading your comment I did some more testing and found one bug in my approach regarding editing the editable voids, I fixed that and added a cypress test for it.

But I see the issue with copy-pasting inside the editable voids and I'm not sure how it is best to solve that 😞 Do you have any ideas?

laufeyrut avatar Sep 20 '22 11:09 laufeyrut

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'.

I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

SmilinBrian avatar Sep 20 '22 17:09 SmilinBrian

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'.

I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

dylans avatar Sep 20 '22 21:09 dylans

I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

SmilinBrian avatar Sep 20 '22 23:09 SmilinBrian

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'. I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

Hi @dylans. Have you had some time to think about this?

evasteingrims avatar Oct 06 '22 09:10 evasteingrims

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'. I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

Hi @dylans. Have you had some time to think about this?

I have but I don't have a good answer yet. Need a few more days to try out other examples and see the potential consequences of this.

dylans avatar Oct 07 '22 17:10 dylans

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.

The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

zbeyens avatar Oct 07 '22 18:10 zbeyens

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.

The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

This is one way I was roughly thinking could work. What do you think @laufeyrut?

dylans avatar Oct 07 '22 18:10 dylans

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable. The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

This is one way I was roughly thinking could work. What do you think @laufeyrut?

Yes, I think this could work! I don't know of a better idea. @dylans I'm not sure how that would be implemented though

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.

The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

I think being able to copy the void nodes should be the default behavior 🙏

laufeyrut avatar Oct 10 '22 12:10 laufeyrut